History | Log In     View a printable version of the current page.  
Issue Details (XML | Word | Printable)

Key: CASTOR-871
Type: Improvement Improvement
Status: Closed Closed
Resolution: Fixed
Priority: Minor Minor
Assignee: Werner Guttmann
Reporter: Martin Fuchs
Votes: 0
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
castor

Implementation of LIMIT and OFFSET for Oracle

Created: 02/Sep/04 01:54 PM   Updated: 19/Apr/05 10:52 AM
Component/s: JDO
Affects Version/s: 0.9.5.3
Fix Version/s: None

Time Tracking:
Not Specified

File Attachments: 1. Text File castor-1732-postProcessParamInfo.patch (26 kb)
2. Text File castor-1732.patch (12 kb)
3. Text File castor-1732.patch (12 kb)
4. Text File castor-1732.patch (14 kb)
5. Text File patch.2 (4 kb)
6. Text File patch.3 (7 kb)
7. Text File patch.4 (7 kb)
8. Text File patch.complete (17 kb)
9. Text File patch.final (10 kb)
10. Text File patch.txt (2 kb)
11. Text File TestLimitClause.patch (3 kb)

Environment:
Operating System: All
Platform: All

Bugzilla Id: 1732


 Description  « Hide
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



 All   Comments   Work Log   Change History      Sort Order: Ascending order - Click to sort in descending order
Werner Guttmann - 03/Sep/04 03:46 AM
Created an attachment (id=639)
Initial (partial) patch.

Werner Guttmann - 03/Sep/04 03:48 AM
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 ?


Werner Guttmann - 10/Sep/04 03:09 AM
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 ?


Werner Guttmann - 10/Sep/04 03:11 AM
Pending a reply to my question above, here's an initial patch for review

Werner Guttmann - 10/Sep/04 03:12 AM
Created an attachment (id=647)
Initial patch.

Martin Fuchs - 10/Sep/04 11:27 AM
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?


Martin Fuchs - 10/Sep/04 12:42 PM
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 ( ");


Werner Guttmann - 10/Sep/04 01:06 PM
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.

Werner Guttmann - 11/Sep/04 01:28 PM
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.


Martin Fuchs - 12/Sep/04 02:35 PM
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.

Werner Guttmann - 13/Sep/04 12:18 PM
Created an attachment (id=654)
New patch

Werner Guttmann - 13/Sep/04 12:19 PM
Above patch is a complete patch, changing OracleQueryExpression as diuscussed
above, and addíng throws SyntaxNotSupportedException to
JDBCQueryExpression.getStatement().

Martin Fuchs - 13/Sep/04 01:30 PM
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 ...".


Werner Guttmann - 13/Sep/04 01:59 PM
Martin, can you please confirm that your OQL looks similar to

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


Martin Fuchs - 13/Sep/04 02:26 PM
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.


Werner Guttmann - 14/Sep/04 01:05 AM
Created an attachment (id=656)
Revised patch as per Martin's advice.

Martin Fuchs - 14/Sep/04 02:31 AM
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);
    }

Werner Guttmann - 14/Sep/04 03:00 AM
Just wondering whether the ':1 syntax' is common across al RDBMS ?

Martin Fuchs - 14/Sep/04 03:40 AM
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...


Martin Fuchs - 14/Sep/04 04:07 AM
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);
    }

Martin Fuchs - 14/Sep/04 04:23 AM
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.


Werner Guttmann - 17/Sep/04 02:29 AM
Created an attachment (id=671)
Final patch for review

Werner Guttmann - 17/Sep/04 02:31 AM
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.


Martin Fuchs - 17/Sep/04 05:23 AM
Created an attachment (id=672)
revised patch

Martin Fuchs - 17/Sep/04 05:24 AM
Created an attachment (id=673)
revised patch without superfluous parts

Martin Fuchs - 17/Sep/04 05:33 AM
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.


Werner Guttmann - 17/Sep/04 07:37 AM
> 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.

Werner Guttmann - 17/Sep/04 07:37 AM
Do your patches build upon mine ?

Martin Fuchs - 17/Sep/04 09:45 AM
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.


Werner Guttmann - 17/Sep/04 10:55 AM
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 .. .


Martin Fuchs - 17/Sep/04 04:59 PM
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++) {


Martin Fuchs - 18/Sep/04 03:53 AM
Created an attachment (id=677)
Fix for TC62

Martin Fuchs - 18/Sep/04 04:01 AM
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>


Martin Fuchs - 18/Sep/04 10:01 AM
Created an attachment (id=678)
last version of the patch?

Martin Fuchs - 18/Sep/04 10:11 AM
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);

Werner Guttmann - 23/Sep/04 01:57 PM
> 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.

Werner Guttmann - 23/Sep/04 02:27 PM
> - 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.

Werner Guttmann - 23/Sep/04 02:28 PM
> - Using LIMIT and OFFSET together still requires an database specific
> order of bind variables.
I honestly cannot think of any solution here.

Werner Guttmann - 23/Sep/04 02:36 PM
Created an attachment (id=688)
Complete patch (for review)

Werner Guttmann - 23/Sep/04 02:36 PM
Complete patch as discussed above.

Martin Fuchs - 24/Sep/04 07:09 AM
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.


Werner Guttmann - 24/Sep/04 12:35 PM
I didn't mean to check this in, to be honest, but at 1:30 a.m. things
unfortunately happen .. . Will revert this code.

Werner Guttmann - 24/Sep/04 12:36 PM
Well, forget my last comment ... I desperately need some sleep.

Martin Fuchs - 26/Sep/04 05:06 AM
>> - 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.


Martin Fuchs - 27/Sep/04 04:03 AM
Created an attachment (id=695)
DB type independend patch

Martin Fuchs - 27/Sep/04 04:06 AM
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.

Werner Guttmann - 27/Sep/04 01:46 PM
Committed as discussed above.

Werner Guttmann - 27/Sep/04 01:48 PM
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 .. .

Martin Fuchs - 27/Sep/04 02:19 PM
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.


Werner Guttmann - 27/Sep/04 02:30 PM
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.


Werner Guttmann - 27/Sep/04 02:32 PM
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.

Martin Fuchs - 27/Sep/04 03:16 PM
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?


Martin Fuchs - 27/Sep/04 03:38 PM
> And do not forget to close this bug once you've verified its correctness .. ;-
).

OK, correctness tested - verified - certificate given.


Werner Guttmann - 28/Sep/04 01:07 AM
> 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 ?

Werner Guttmann - 28/Sep/04 01:08 AM
> 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) ?

Martin Fuchs - 28/Sep/04 01:22 AM
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


Martin Fuchs - 28/Sep/04 01:30 AM
Found, what's wrong with HSQL.
The syntax should be:

select limit 0 3 *
from Customer

and:

select top 5 *
from Customer


Werner Guttmann - 28/Sep/04 02:41 AM
Can you please supply me with unified patches ?

Martin Fuchs - 28/Sep/04 03:03 AM
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.


Werner Guttmann - 28/Sep/04 04:03 AM
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 ... .


Werner Guttmann - 05/Oct/04 01:38 PM
      • Bug 1199 has been marked as a duplicate of this bug. ***