Bugzilla Version 2.16.7
Bugzilla Bug 1732
  Implementation of LIMIT and OFFSET for Oracle
     Query page      Enter new bug
Bug#: 1732   Platform:   Reporter: martin-fuchs at gmx dot net (Martin Fuchs)
Product:   OS:   Add CC:
Component:   Version:   CC:
Remove selected CCs
Status: CLOSED   Priority:  
Resolution: FIXED   Severity:  
Assigned To: werner dot guttmann at gmx dot net (Werner Guttmann)   Target Milestone:  
URL:
Summary:

Attachment Type Modified Status Actions
Initial (partial) patch. patch 2004-09-03 03:46 none Edit
Initial patch. patch 2004-09-10 03:12 none Edit
New patch patch 2004-09-13 12:18 none Edit
Revised patch as per Martin's advice. patch 2004-09-14 01:05 none Edit
Final patch for review patch 2004-09-17 02:29 none Edit
revised patch patch 2004-09-17 05:23 none Edit
revised patch without superfluous parts patch 2004-09-17 05:24 none Edit
Fix for TC62 patch 2004-09-18 03:53 none Edit
last version of the patch? patch 2004-09-18 10:01 none Edit
Complete patch (for review) patch 2004-09-23 14:36 none Edit
DB type independend patch patch 2004-09-27 04:03 none Edit
Create a New Attachment (proposed patch, testcase, etc.) View All

Bug 1732 depends on: Show dependency tree
Show dependency graph
Bug 1732 blocks: 1733
Votes: 0    Show votes for this bug    Vote for this bug

Additional Comments:


Leave as CLOSED FIXED
Reopen bug

View Bug Activity   |   Format For Printing

Description: Opened: 2004-09-02 13:54

If you are using an Oracle DB of version 8.1.6 or above, you can use
the 'rank()' function to implement full support for LIMIT and OFFSET.

For example to query all rows between number 3 and 8 you can use:

select *
from (
    select  empno, ename, job, sal,
            rank() over (order by sal) rnk
    from    emp
)
where   rnk between 3 and 8


So this is how OQL queries should be transformed:


"SELECT o FROM Person o ORDER BY personId LIMIT $1 OFFSET $2"

-> 

select *
from (
    select  ... ,
            rank() over (order by ...) rnk
    from    ...
)
where   rnk between :2 and :2 + :1



"SELECT o FROM Person o ORDER BY personId LIMIT $1"

->

select *
from (
    select  ... ,
            rank() over (order by ...) rnk
    from    ...
)
where   rnk <= :1


It may be possible to optimize the simple case with 'LIMIT', but 
without 'OFFSET' and without an 'order by' clause.
For this simple case you can use the 'rownum' in a statement like.
This is also compatible to older Oracle DBs like 8.1.5 or 7.3.4:

select	*
from	emp
where	rownum <= :1

------- Additional Comment #1 From Werner Guttmann 2004-09-03 03:46 -------
Created an attachment (id=639)
Initial (partial) patch.

------- Additional Comment #2 From Werner Guttmann 2004-09-03 03:48 -------
Above patch is only a partial solution for the feature request described above. 

As it is, it adds the outer select statement incl. the WHERE rnk ... clause.

With regards to the insertion of the 

  rank() over (order by ...) rnk

expression, I am a bit torn between inserting this into the SQL code already
generated at this time through StringBuffer.manipulation (e.g. before the FROM
identifier) or a more structured approach. Hmm ?

------- Additional Comment #3 From Werner Guttmann 2004-09-10 03:09 -------
Martin, looking at your decription above and at the Oracle manuals at
http://download-west.oracle.com/docs/cd/B14117_01/server.101/b10759/functions108.htm#sthref1577,
it looks like the rank() function requires an ORDER BY clause to be in place. 

Question; what to do if one does not want to specify such a clause ?

------- Additional Comment #4 From Werner Guttmann 2004-09-10 03:11 -------
Pending a reply to my question above, here's an initial patch for review

------- Additional Comment #5 From Werner Guttmann 2004-09-10 03:12 -------
Created an attachment (id=647)
Initial patch.

------- Additional Comment #6 From Martin Fuchs 2004-09-10 11:27 -------
Yes, the rank functions needs a sort order to be able to generate the ranking 
of datasets. So you will always have to add an order by clause.

There might be two ways to handle the case, if the Castor user doesn't 
explicitly specify one:

a.) The first one, which I would prefer, is to issue an exception. This is 
justifiable because it doesn't make much sense to try windowing a query result 
set to some range [from .. to] without an defined order. The results would not 
be predictable.

b.) Alternatively one might force a predictable (but somehow arbitrarily) sort 
order by automatically generating a ORDER BY clause containing all SELECT 
columns. To use the example from above, the query would become

select *
from (
    select  empno, ename, job, sal,
            rank() over (ORDER BY EMPNO, ENAME, JOB, SAL) rnk
    from    emp
)
where   rnk between 3 and 8

If we decided to use solution a.) {throwing expceptions}, I could also live 
with creating a ORDER BY in the special case, if there exists a LIMIT clause 
but no OFFSET clause in the OQL. This would be some kind of middle way between 
a.) and b.) .

What do you think on this ideas?


By the way:
How do other database systems than Oracle handle the case of LIMIT/OFFSET 
queries without any ORDER criteria?

------- Additional Comment #7 From Martin Fuchs 2004-09-10 12:42 -------
Hi Werner,

experimenting a bit with your patch I noticed my alternative b.) is not quite 
correct. One should use only generate an ORDER BY using the identity columns of 
the queried table, not all columns.

Btw. there is currently missing a coma in the SQL syntax.
It should be:

   sql.append (", rank() over ( ");
instead of
   sql.append (" rank() over ( ");

------- Additional Comment #8 From Werner Guttmann 2004-09-10 13:06 -------
Martin, thanks for pointing out the incorrect syntax wrt adding a rank()
function to the generated SQL statement. I have already changed the relevant
code section, and will provide a new patch once I have considered your
statements re: the use of an ORDER BY clause.

------- Additional Comment #9 From Werner Guttmann 2004-09-11 13:28 -------
Martin, I thin I am actually in favour of option a), i.e. throwing an exception.
Just looking at OracleQueryExpression.getStatement(), it looks like the method
signature has to be changed to include a throws clause, or more specifically

public String getStatement (boolean lock) throws XXXException {

I am not 100% sure yet whether to throw a (more general) OQlQueryException or a
SyntaxNotSupportedException, though this would be a bit misleading.

In addition, one would need to change the method signature in
JDBCQueryExpression as well.

------- Additional Comment #10 From Martin Fuchs 2004-09-12 14:35 -------
Well, I think a SyntaxNotSupportedException would be the right exception to 
throw. The description "Exception thrown to indicate that a feature is not 
supported by a particular database." matches the problem we are faced here. To 
make it clear you can use a message like "LIMIT without ORDER BY" as argument 
for the exception constructor.

------- Additional Comment #11 From Werner Guttmann 2004-09-13 12:18 -------
Created an attachment (id=654)
New patch

------- Additional Comment #12 From Werner Guttmann 2004-09-13 12:19 -------
Above patch is a complete patch, changing OracleQueryExpression as diuscussed
above, and addíng throws SyntaxNotSupportedException to
JDBCQueryExpression.getStatement().

------- Additional Comment #13 From Martin Fuchs 2004-09-13 13:30 -------
OK, this works for me when using the LIMIT keyword.


But when trying to use the OFFSET keyword there is a problem with the generated 
SQL, for example:

select * from ( 
SELECT "EMP"."EMPNO","EMP"."ENAME","EMP"."JOB","EMP"."MGR","EMP"."HIREDATE","EMP
"."SAL","EMP"."COMM","EMP"."DEPTNO" , rank() over (  ORDER BY "EMP"."EMPNO"  ) 
rnk  FROM "EMP" ) where rnk between ? and ? + ?

But there are only two bind variables, e.g.:

oql.bind(limit);
oql.bind(offset);


The generated SQL should better counted place holders:
... where rnk between :2 and :1 + :2


Just another minor fix. The exception message should be "To use a LIMIT ..." 
instead of "To use a LIMT ...".

------- Additional Comment #14 From Werner Guttmann 2004-09-13 13:59 -------
Martin, can you please confirm that your OQL looks similar to 

"SELECT o FROM Person o ORDER BY personId LIMIT $1 OFFSET $2"

------- Additional Comment #15 From Martin Fuchs 2004-09-13 14:26 -------
Yes, this is a snippet of my test class:

// Query DEPT datasets 
OQLQuery oql = db.getOQLQuery(
        "SELECT	x\n"+
        "FROM	scottJdo.Emp x\n"+
        "ORDER BY EMPNO LIMIT $1 OFFSET $2"
);

oql.bind(2);
oql.bind(1);

QueryResults results = oql.execute(Database.ReadOnly);

try {
	while(results.hasMore()) {
		Emp emp = (Emp) results.next();

		System.out.println("EMP.EMPNO="+emp.EMPNO);
				System.out.println("");
	}
} finally {
	results.close();
	oql.close();
}


If you need a full test case, I can post it tomorrow.

------- Additional Comment #16 From Werner Guttmann 2004-09-14 01:05 -------
Created an attachment (id=656)
Revised patch as per Martin's advice.

------- Additional Comment #17 From Martin Fuchs 2004-09-14 02:31 -------
The hardcoded bind variable positions of course work only when using no other 
bind variables than LIMIT and OFFSET. Why not fix it for all cases in 
ParseTreeWalker:

Index: ParseTreeWalker.java
===================================================================
RCS 
file: /cvs/castor/castor/src/main/org/exolab/castor/jdo/oql/ParseTreeWalker.java
,v
retrieving revision 1.5
diff -u -p -d -w -b -r1.5 ParseTreeWalker.java
--- a/ParseTreeWalker.java      8 Sep 2004 09:12:25 -0000       1.5
+++ b/ParseTreeWalker.java      14 Sep 2004 09:28:55 -0000
 at  at  -1130,7 +1130,7  at  at  public class ParseTreeWalker implements
         paramNumber = new Integer(sqlExpr.substring(pos + 1));
       ParamInfo paramInfo = (ParamInfo) _paramInfo.get(paramNumber);
       paramInfo.mapToSQLParam( SQLParamIndex++ );
-      sb.append( sqlExpr.substring( startPos, pos+1 ) );
+      sb.append( ":" + sqlExpr.substring( startPos+1, pos+2 ) );
       startPos = endPos < 0 ? sqlExpr.length() : endPos;
       pos = sqlExpr.indexOf("?", startPos);
     }
 at  at  -1166,7 +1166,7  at  at  private void addOffsetClause(ParseTreeNo
         paramNumber = new Integer(sqlExpr.substring(pos + 1));
       ParamInfo paramInfo = (ParamInfo) _paramInfo.get(paramNumber);
       paramInfo.mapToSQLParam( SQLParamIndex++ );
-      sb.append( sqlExpr.substring( startPos, pos+1 ) );
+      sb.append( ":" + sqlExpr.substring( startPos+1, pos+2 ) );
       startPos = endPos < 0 ? sqlExpr.length() : endPos;
       pos = sqlExpr.indexOf("?", startPos);
     }

------- Additional Comment #18 From Werner Guttmann 2004-09-14 03:00 -------
Just wondering whether the ':1 syntax' is common across al RDBMS ?

------- Additional Comment #19 From Martin Fuchs 2004-09-14 03:40 -------
This is a good question. I justed searched the web and found, it is a feature 
of the Oracle JDBC driver:
http://radio.weblogs.com/0118231/stories/2003/03/05/clarifyingSomeBindVariableCo
nfusion.html

I'm very Oracle biased.  ;-)

With Oracle version 10 you can even use named bind variables like ":EMPNO" 
or ":SAL":

http://www.oracle.com/technology/sample_code/tech/java/codesnippet/jdbc/namedpar
am/UsingOracleNamedParameterAPIs.html


So what to do?
The current bug here is only to iomplement LIMIT/OFFSET for Oracle connections. 
Because of this the ":1" syntax can be used in this place. But ParseTreeWalker 
is a common class. 

I think it would be the best to move the code, which generates the SQL bind 
syntax into the database specific classes, in this case OracleQueryExpression. 
Is as side effect this should also enhance the performance for Oracle queries a 
bit. I will work on a better proposal...

------- Additional Comment #20 From Martin Fuchs 2004-09-14 04:07 -------
OK, here is the new approach introducing a new function getBindSql() in 
QueryExpression to be able to use the Oracle specific bind variable syntax:


Index: src/main/org/exolab/castor/persist/spi/QueryExpression.java
===================================================================
RCS 
file: /cvs/castor/castor/src/main/org/exolab/castor/persist/spi/QueryExpression.
java,v
retrieving revision 1.3
diff -u -p -d -r1.3 QueryExpression.java
--- a/src/main/org/exolab/castor/persist/spi/QueryExpression.java	8 Sep 
2004 09:09:00 -0000	1.3
+++ b/src/main/org/exolab/castor/persist/spi/QueryExpression.java	14 Sep 
2004 10:57:36 -0000
 at  at  -354,6 +354,11  at  at  public interface QueryExpression
      */
     public boolean isOffsetClauseSupported();
 
+    /**
+     * create SQL bind string from OQL bind string 
+     */
+    public String getBindSql(String bind_oql);
+
 }
 
 
Index: src/main/org/exolab/castor/jdo/drivers/JDBCQueryExpression.java
===================================================================
RCS 
file: /cvs/castor/castor/src/main/org/exolab/castor/jdo/drivers/JDBCQueryExpress
ion.java,v
retrieving revision 1.4
diff -u -p -d -r1.4 JDBCQueryExpression.java
--- a/src/main/org/exolab/castor/jdo/drivers/JDBCQueryExpression.java	8 Sep 
2004 09:22:13 -0000	1.4
+++ b/src/main/org/exolab/castor/jdo/drivers/JDBCQueryExpression.java	14 Sep 
2004 10:57:34 -0000
 at  at  -611,6 +617,13  at  at  public class JDBCQueryExpression
 	}
 
 
-}
+    /**
+     * create SQL bind string from OQL bind string 
+     */
+    public String getBindSql(String bind_oql)
+    {
+        return bind_oql.substring(0, 1);	/// at todo why not just 
return "?" ?
+    }
 
 
+}

Index: src/main/org/exolab/castor/jdo/drivers/OracleQueryExpression.java
===================================================================
RCS 
file: /cvs/castor/castor/src/main/org/exolab/castor/jdo/drivers/OracleQueryExpre
ssion.java,v
retrieving revision 1.2
diff -u -p -d -r1.2 OracleQueryExpression.java
--- a/src/main/org/exolab/castor/jdo/drivers/OracleQueryExpression.java	19 Jan 
2004 20:01:46 -0000	1.2
+++ b/src/main/org/exolab/castor/jdo/drivers/OracleQueryExpression.java	14 Sep 
2004 10:57:34 -0000
 at  at  -130,17 +160,86  at  at  public final class OracleQueryExpression
 
+    /**
+     * create SQL bind string from OQL bind string
+     * using the Oracle specific ":1" syntax with positioned bind variables 
+     */
+    public String getBindSql(String bind_oql)
+    {
+        return ":" + bind_oql.substring(1);
+    }
 
+}
Index: src/main/org/exolab/castor/jdo/oql/ParseTreeWalker.java
===================================================================
RCS 
file: /cvs/castor/castor/src/main/org/exolab/castor/jdo/oql/ParseTreeWalker.java
,v
retrieving revision 1.5
diff -u -p -d -r1.5 ParseTreeWalker.java
--- a/src/main/org/exolab/castor/jdo/oql/ParseTreeWalker.java	8 Sep 2004 
09:12:25 -0000	1.5
+++ b/src/main/org/exolab/castor/jdo/oql/ParseTreeWalker.java	14 Sep 2004 
10:57:36 -0000
 at  at  -1130,7 +1130,8  at  at  public class ParseTreeWalker implements 
         paramNumber = new Integer(sqlExpr.substring(pos + 1));
       ParamInfo paramInfo = (ParamInfo) _paramInfo.get(paramNumber);
       paramInfo.mapToSQLParam( SQLParamIndex++ );
-      sb.append( sqlExpr.substring( startPos, pos+1 ) );
+      String bind_sql = _queryExpr.getBindSql(sqlExpr.substring(startPos, 
pos+2));
+      sb.append(bind_sql);
       startPos = endPos < 0 ? sqlExpr.length() : endPos;
       pos = sqlExpr.indexOf("?", startPos);
     }
 at  at  -1166,7 +1167,8  at  at  private void addOffsetClause(ParseTreeNo
         paramNumber = new Integer(sqlExpr.substring(pos + 1));
       ParamInfo paramInfo = (ParamInfo) _paramInfo.get(paramNumber);
       paramInfo.mapToSQLParam( SQLParamIndex++ );
-      sb.append( sqlExpr.substring( startPos, pos+1 ) );
+      String bind_sql = _queryExpr.getBindSql(sqlExpr.substring(startPos, 
pos+2));
+      sb.append(bind_sql);
       startPos = endPos < 0 ? sqlExpr.length() : endPos;
       pos = sqlExpr.indexOf("?", startPos);
     }

------- Additional Comment #21 From Martin Fuchs 2004-09-14 04:23 -------
One more additional change for OracleQueryExpression:

        // add LIMIT/OFFSET clause - part 2
        if ( _limit != null ) {
            sql.insert (0, " select * from ( ");

        	if ( _offset != null ) {
        		sql.append (" ) where rnk between " + _offset + " and " 
+ _limit + " + " + _offset + " - 1 ");
        	} else {
        		sql.append (" ) where rnk <= " + _limit + " ");
        	}
        }

There should be am additional "-1" in the "between" expression, but this has 
been my fault at the very first beginning.

------- Additional Comment #22 From Werner Guttmann 2004-09-17 02:29 -------
Created an attachment (id=671)
Final patch for review

------- Additional Comment #23 From Werner Guttmann 2004-09-17 02:31 -------
Above patch is the total of all patches discussed above. Martin, can you please
review. I've been able to run

'test castor.mysql.TC62'

successfully, so I think I am ready to commit this change.

------- Additional Comment #24 From Martin Fuchs 2004-09-17 05:23 -------
Created an attachment (id=672)
revised patch

------- Additional Comment #25 From Martin Fuchs 2004-09-17 05:24 -------
Created an attachment (id=673)
revised patch without superfluous parts

------- Additional Comment #26 From Martin Fuchs 2004-09-17 05:33 -------
Hi Werner,

attached you find my revised version of the patch. (sorry for the first version 
with changes in oracle.xml)

I resolved my TODO comment in the first version, and changed three things:

- getBindSql() gets now only the OQL parameter string without the leading "?". 
So we can in fact just return "?" for in JDBCQueryExpression.getBindSql().

- reworked the changes in addLimitClause() and addOffsetClause

- Now I'm using getBindSql() also in addWhereClause().

I already run the CTF test cases for Oracle: No problems other than that of 
TC47 and TC66.

------- Additional Comment #27 From Werner Guttmann 2004-09-17 07:37 -------
> I already run the CTF test cases for Oracle: No problems other than that 
> of TC47 and TC66.
I've already committed a separate patch for TC66.. Please run the tests again,
and you should notice only one error.

------- Additional Comment #28 From Werner Guttmann 2004-09-17 07:37 -------
Do your patches build upon mine ? 

------- Additional Comment #29 From Martin Fuchs 2004-09-17 09:45 -------
Sure, I took your last patch, applied it to the CVS code, and changed it as 
described.

Mhh, TC66 doesn't pass yet. Instead there is another problem with TC20:

junit.framework.AssertionFailedError: (is) bool value was not set
        at jdo.TypeHandling.testBooleanIsMethod(TypeHandling.java:305)
        at jdo.TypeHandling.runTest(TypeHandling.java:421)
        at harness.CastorTestCase.run(CastorTestCase.java:154)
        at harness.TestHarness.run(TestHarness.java:140)
        at harness.TestHarness.run(TestHarness.java:140)
        at Main.run(Main.java:152)
        at Main.main(Main.java:226)

Is see this even with the unpatched sources.

------- Additional Comment #30 From Werner Guttmann 2004-09-17 10:55 -------
Martin, can I read this as TC62 passes successfully. And the problem with TC20
is known, and can be 'investigated' by reading bug 1689.

As it happened, I checked in a change to a CTF test case assuming that it
executed successfully after a new testXXX() method has been added. Turned out
that this test case had a runTest() method where the new testXXX() method had
not been added .. :-(.

------- Additional Comment #31 From Martin Fuchs 2004-09-17 16:59 -------
No, TC62 doesn't pass. It is not part of the standard 'castor.oracle' test 
suite, so I didn't run it till now. So I inserted it into the Oracle test case 
list, and had to see some problems:

1. The TestLimitClause test class needs to be extended by "order by id" 
expressions to be able to be used for Oracle.

2. It tries to execute: assertEquals (LIMIT, results.size());
This doesn't work for Oracle. (see TC66)

3. There are still problems with the :1 / :2 bind variables, so using LIMIT 
with OFFSET doesn't work. However the testLimit() passes when replacing the 
results.size() assertion by a assertTrue(!results.hasMore()) after the fetch 
loop.

4. I don't think the testLimitWithOffset() test is correct:

      OQLQuery query = _db.getOQLQuery("select t from jdo.TestObject t order by 
id limit $1 offset $2");
        query.bind(LIMIT);
        query.bind(OFFSET);
        QueryResults results = query.execute();
        assertNotNull (results);
//      assertEquals (5, results.size());
        for (int i = 1 + OFFSET; i <= 5; i++) {
            testObject = (TestObject) results.next();
            assertEquals(i, testObject.getId());
        }
        assertTrue(!results.hasMore());


Shouldn't the loop better be:

        for (int i = 1 + OFFSET; i <= OFFSET+LIMIT; i++) {

------- Additional Comment #32 From Martin Fuchs 2004-09-18 03:53 -------
Created an attachment (id=677)
Fix for TC62

------- Additional Comment #33 From Martin Fuchs 2004-09-18 04:01 -------
OK, let's first fix TC62.

This is what I changed:

- Change the test OFFSET to 2 instead of the previous value 5. The previous 
value was not very good, because you can't see probblems with the parameter 
order of LIMIT and OFFSET, if LIMIT and OFFSET are equal.

- Don't use size() calls to try counting result objects, because this isn't 
portable, and can't be used with Oracle DB connections. Instead just test 
with 'assertTrue(!results.hasMore());' after fetching all rows in the loop.

- I removed all hardcoded '5' values and replaced them with the LIMIT or OFFSET 
constants.

- Fixed the wrong loop in testLimitWithOffset() as described above.

- Inserted a comment explaining the problems with the current DB-dependent 
implementation of LIMIT/OFFSET bind variable order.


After applying the fix for TestLimitClause.java above, you can activate TC62 
for the Oracle suite:

Index: tests.xml
===================================================================
RCS file: /cvs/castor/castor/src/tests/tests.xml,v
retrieving revision 1.7
diff -u -p -d -r1.7 tests.xml
--- a/tests.xml 12 Sep 2004 20:06:55 -0000      1.7
+++ b/tests.xml 18 Sep 2004 10:54:13 -0000
 at  at  -104,6 +104,7  at  at 
     <case class="jdo.TimeStampMismatched" />
     <case class="jdo.Size" />
     <case class="jdo.Absolute" />
+    <case class="jdo.TestLimitClause" id="TC62" />

   </category>

------- Additional Comment #34 From Martin Fuchs 2004-09-18 10:01 -------
Created an attachment (id=678)
last version of the patch?

------- Additional Comment #35 From Martin Fuchs 2004-09-18 10:11 -------
Here is my last version of this patch. It passes now the the Oracle and MySQL 
test cases including TC62. Of course the following test cases fail: 02(MySQL), 
47(Oracle), 20(Oracle+MySQL), 66(Oracle). But that are other problems.

I removed the Oracle specific :1/:2 bind parameters again, because they did not 
work as expected. Instead I'm know using an expression like "... where rnk - 
OFFSET between 1 and LIMIT".

Known little problems are:

- Using LIMIT and OFFSET together still requires an database specific order of 
bind variables. For Oracle and MySQL you have to "limit $2 offset $1", and for 
PostgreSQL you have to use "limit $1 offset $2", if afer that calling:
query.bind(LIMIT);
query.bind(OFFSET);

- The problem described at the bottom of 
http://brainopolis.dnsalias.com/castorwiki/Wiki.jsp?page=OQLLimit still exists 
(for all databases): You should use LIMIT only for simple queries, which don't 
generate joined SQL queries.

------- Additional Comment #36 From Werner Guttmann 2004-09-23 13:57 -------
> Here is my last version of this patch. It passes now the the Oracle and 
> MySQL test cases including TC62. Of course the following test cases 
> fail: 02(MySQL), 47(Oracle), 20(Oracle+MySQL), 66(Oracle).
Using all the latest and greatest source code from CVS plus the total of all the
patches discussed here, for mySQL it's only TC02 failing (which afair is a known
issue). Let me post a complete patch, and I'd appreciate if you could run it
against Oracle.

------- Additional Comment #37 From Werner Guttmann 2004-09-23 14:27 -------
> - The problem described at the bottom of 
> http://brainopolis.dnsalias.com/castorwiki/Wiki.jsp?page=OQLLimit still 
> exists (for all databases): You should use LIMIT only for simple 
>queries, which don't generate joined SQL queries.
How about throwing an exception if a LIMIT clause is used with more complex
queries, e.g. including a join. This should be achievable.

------- Additional Comment #38 From Werner Guttmann 2004-09-23 14:28 -------
> - Using LIMIT and OFFSET together still requires an database specific 
> order of bind variables. 
I honestly cannot think of any solution here.

------- Additional Comment #39 From Werner Guttmann 2004-09-23 14:36 -------
Created an attachment (id=688)
Complete patch (for review)

------- Additional Comment #40 From Werner Guttmann 2004-09-23 14:36 -------
Complete patch as discussed above.

------- Additional Comment #41 From Martin Fuchs 2004-09-24 07:09 -------
The Castor patch itself is OK, but you in changed the bind variable order 
from "limit $2 offset $1" into "limit $1 offset $2" at the following line in 
TestLimitClause.java:

+        OQLQuery query = _db.getOQLQuery("select t from jdo.TestObject t order 
by id limit $1 offset $2");
+        //OQLQuery query = _db.getOQLQuery("select t from jdo.TestObject t 
order by id limit $1 offset $2");


You should revert this to enable successfull passing of TC62 for MySQL and 
Oracle.

------- Additional Comment #42 From Werner Guttmann 2004-09-24 12:35 -------
I didn't mean to check this in, to be honest, but at 1:30 a.m. things
unfortunately happen .. :-(. Will revert this code.

------- Additional Comment #43 From Werner Guttmann 2004-09-24 12:36 -------
Well, forget my last comment ... I desperately need some sleep.

------- Additional Comment #44 From Martin Fuchs 2004-09-26 05:06 -------
>> - The problem described at the bottom of 
>> http://brainopolis.dnsalias.com/castorwiki/Wiki.jsp?page=OQLLimit still 
>> exists (for all databases): You should use LIMIT only for simple 
>>queries, which don't generate joined SQL queries.
>How about throwing an exception if a LIMIT clause is used with more complex
>queries, e.g. including a join. This should be achievable.

Yes, would be a possibility. This way at least no one gets wrong result data.

>> - Using LIMIT and OFFSET together still requires an database specific 
>> order of bind variables. 
>I honestly cannot think of any solution here.

Well, the order of bind values in OQLQueryImpl._bindValues or 
OQLQueryImpl._paramInfo would have to be changed depending on the database type.

------- Additional Comment #45 From Martin Fuchs 2004-09-27 04:03 -------
Created an attachment (id=695)
DB type independend patch

------- Additional Comment #46 From Martin Fuchs 2004-09-27 04:06 -------
The new patch above now handles the order of LIMIT/OFFSET bind variable in a 
database type independend manner by introducing a new function 
postProcessParamInfo() in the interface QueryExpression. This may not be the 
cleanest solution, but I think it solves the problem. I tested against Oracle 
and MySQL.

------- Additional Comment #47 From Werner Guttmann 2004-09-27 13:46 -------
Committed as discussed above.

------- Additional Comment #48 From Werner Guttmann 2004-09-27 13:48 -------
I think we should still throw an exception when the LIMIT clause is used with
'complex' SQL statements in OracleQueryExpression. We could e.g. search the
generated SQL String for occurrences of JOIN, etc. Very simplistic, but hey,
this whole class is one single hack .. ;-).

------- Additional Comment #49 From Martin Fuchs 2004-09-27 14:19 -------
Thanks for the commit. The discussion about this patch got a bit long. Now I 
can update the 1733-patch.

I think it would be enough to query '_joins.isEmpty()' to decide, if the query 
if a simply one.

Btw: Looking for "JOIN" in the SQL you would not find much - Oracle would 
support it since version 9, but Castor uses only the old syntax.  ;-)

------- Additional Comment #50 From Werner Guttmann 2004-09-27 14:30 -------
Sure, _joins.isEmpty() makes for a better candidate than looking for a substring
'JOIN' .. ;-). But what I had in mind was OUTER or/and LEFT JOINS, as they do
occur literally.

Let's try to get bug 1733 out of our way, so that I can focus (again) on support
for lazy-loading 1:1 relations (bug 1676), deal with a bit of a problem with
read-only support for 1:1 relations (bug 1660) and eventually get my head around
support for FK as part of a PK.

------- Additional Comment #51 From Werner Guttmann 2004-09-27 14:32 -------
And do not forget to close this bug once you've verified its correctness .. ;-).
I think I shall ask a cent for every time I mention this ... ;-). For those of
us listening in without an appreciation of cents, that makes (almost) a pence.

------- Additional Comment #52 From Martin Fuchs 2004-09-27 15:16 -------
I know, this does not really belong here...
But I just saw there is missing a isLimitClauseSupported() function in 
SQLServerQueryExpression.java, as it claims to be able to process LIMIT queries:

Index: src/main/org/exolab/castor/jdo/drivers/SQLServerQueryExpression.java
===================================================================
RCS 
file: /cvs/castor/castor/src/main/org/exolab/castor/jdo/drivers/SQLServerQueryEx
pression.java,v
retrieving revision 1.2
diff -u -p -d -r1.2 SQLServerQueryExpression.java
--- a/src/main/org/exolab/castor/jdo/drivers/SQLServerQueryExpression.java      
19 Jan 2004 20:01:46 -0000      1.2
+++ b/src/main/org/exolab/castor/jdo/drivers/SQLServerQueryExpression.java      
27 Sep 2004 22:09:36 -0000
 at  at  -140,7 +140,9  at  at  public final class SQLServerQueryExpress
         return sql.toString();
     }

+
+    public boolean isLimitClauseSupported () {
+       return true;
+    }

 }
-
-


I can't test it, may be some one else has a MS SQL server running and can look 
into this?


Another thing...

Class HsqlQueryExpression also tries to generate SQL including a LIMIT 
expression. But my HSQL test DB (version 1.7.2.4) doesn't recognise this. Has 
this been tested by some one?

------- Additional Comment #53 From Martin Fuchs 2004-09-27 15:38 -------
> And do not forget to close this bug once you've verified its correctness .. ;-
).

OK, correctness tested - verified - certificate given.  :)

------- Additional Comment #54 From Werner Guttmann 2004-09-28 01:07 -------
> But I just saw there is missing a isLimitClauseSupported() function 
> in SQLServerQueryExpression.java, as it claims to be able to process 
> LIMIT queries:
If that's the case, what syntax would MS SQL Server be using for a LIMIT clause ?

------- Additional Comment #55 From Werner Guttmann 2004-09-28 01:08 -------
> Class HsqlQueryExpression also tries to generate SQL including a LIMIT 
> expression. But my HSQL test DB (version 1.7.2.4) doesn't recognise 
> this. Has this been tested by some one?
As far as I know, the person submitting the original enhancement request was
using HSQL. Anyhow, what problems are you seeing ? Can you post the stack trace
(or at least the error message) ?

------- Additional Comment #56 From Martin Fuchs 2004-09-28 01:22 -------
For SQL Server there is used the syntax "TOP n":

        if ( _limit != null )
          if ( _limit.equals("") == false )
            sql.append("TOP ").append(_limit).append(" ");


I just issued simple queries using the HSQL query tool, for example:

select *
from Customer
where 1=1
limit 6 2

And I get an "unexpected token: LIMIT in statement [limit]" error message.

The following prints "Unexpected token: 5 in statement [5]":

select *
from Customer
top 5

------- Additional Comment #57 From Martin Fuchs 2004-09-28 01:30 -------
Found, what's wrong with HSQL.
The syntax should be:

select limit 0 3 *
from Customer

and:

select top 5 *
from Customer

------- Additional Comment #58 From Werner Guttmann 2004-09-28 02:41 -------
Can you please supply me with unified patches ?

------- Additional Comment #59 From Martin Fuchs 2004-09-28 03:03 -------
*lol* I don't yet have patches.

May be we should move this discussion and re-open bug 1221 for the HSQL patch. 
It will also require some reordering of bind parameters. It's even more 
complicated than on Oracle, because you have to move the LIMIT and OFFSET 
paramaters at the first position - even bevore any parameters used in the WHERE 
expressions.

------- Additional Comment #60 From Werner Guttmann 2004-09-28 04:03 -------
Agreed. And let's move the HSQL and MSSQL parts of this to separate (new) bugs.
Let's not reopen bug 1221, please.

And how come you cannot supply me with patches yet ? It's more than 60mins since
you've posted this ... ;-).

------- Additional Comment #61 From Werner Guttmann 2004-10-05 13:38 -------
*** Bug 1199 has been marked as a duplicate of this bug. ***

     Query page      Enter new bug
This is Bugzilla: the Mozilla bug system. For more information about what Bugzilla is and what it can do, see bugzilla.org.
Actions: New | Query | bug # | Reports   New Account | Log In