|
|
|
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 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 ? Pending a reply to my question above, here's an initial patch for review
Created an attachment (id=647)
Initial patch. 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 a.) The first one, which I would prefer, is to issue an exception. This is b.) Alternatively one might force a predictable (but somehow arbitrarily) sort select * If we decided to use solution a.) {throwing expceptions}, I could also live What do you think on this ideas? By the way: Hi Werner,
experimenting a bit with your patch I noticed my alternative b.) is not quite Btw. there is currently missing a coma in the SQL syntax. sql.append (", rank() over ( "); 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. 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 In addition, one would need to change the method signature in 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. Above patch is a complete patch, changing OracleQueryExpression as diuscussed
above, and addíng throws SyntaxNotSupportedException to JDBCQueryExpression.getStatement(). 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 select * from ( But there are only two bind variables, e.g.: oql.bind(limit); The generated SQL should better counted place holders: Just another minor fix. The exception message should be "To use a LIMIT ..." Martin, can you please confirm that your OQL looks similar to
"SELECT o FROM Person o ORDER BY personId LIMIT $1 OFFSET $2" Yes, this is a snippet of my test class:
// Query DEPT datasets oql.bind(2); QueryResults results = oql.execute(Database.ReadOnly); try { If you need a full test case, I can post it tomorrow. Created an attachment (id=656)
Revised patch as per Martin's advice. 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
Just wondering whether the ':1 syntax' is common across al RDBMS ?
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" http://www.oracle.com/technology/sample_code/tech/java/codesnippet/jdbc/namedpar So what to do? I think it would be the best to move the code, which generates the SQL bind 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 + /** Index: src/main/org/exolab/castor/jdo/drivers/JDBCQueryExpression.java -} +} Index: src/main/org/exolab/castor/jdo/drivers/OracleQueryExpression.java + /** +}
One more additional change for OracleQueryExpression:
// add LIMIT/OFFSET clause - part 2 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 Created an attachment (id=671)
Final patch for review 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. Created an attachment (id=672)
revised patch Created an attachment (id=673)
revised patch without superfluous parts Hi Werner,
attached you find my revised version of the patch. (sorry for the first version I resolved my TODO comment in the first version, and changed three things:
I already run the CTF test cases for Oracle: No problems other than that of > 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. 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 Is see this even with the unpatched sources. 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 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" 2. It tries to execute: assertEquals (LIMIT, results.size()); 3. There are still problems with the :1 / :2 bind variables, so using LIMIT 4. I don't think the testLimitWithOffset() test is correct: OQLQuery query = _db.getOQLQuery("select t from jdo.TestObject t order by Shouldn't the loop better be: for (int i = 1 + OFFSET; i <= OFFSET+LIMIT; i++) { OK, let's first fix TC62.
This is what I changed:
After applying the fix for TestLimitClause.java above, you can activate TC62 Index: tests.xml </category> Created an attachment (id=678)
last version of the patch? 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 Known little problems are:
> 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. > - The problem described at the bottom of
> http://brainopolis.dnsalias.com/castorwiki/Wiki.jsp?page=OQLLimit ![]() > 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. > - Using LIMIT and OFFSET together still requires an database specific
> order of bind variables. I honestly cannot think of any solution here. Created an attachment (id=688)
Complete patch (for review) 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 You should revert this to enable successfull passing of TC62 for MySQL and I didn't mean to check this in, to be honest, but at 1:30 a.m. things
unfortunately happen .. ![]() Well, forget my last comment ... I desperately need some sleep.
>> - The problem described at the bottom of
>> http://brainopolis.dnsalias.com/castorwiki/Wiki.jsp?page=OQLLimit ![]() >> 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 Well, the order of bind values in OQLQueryImpl._bindValues or Created an attachment (id=695)
DB type independend patch 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. 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 .. ![]() 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 Btw: Looking for "JOIN" in the SQL you would not find much - Oracle would Sure, _joins.isEmpty() makes for a better candidate than looking for a substring
'JOIN' .. ![]() occur literally. Let's try to get bug 1733 out of our way, so that I can focus (again) on support 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 ... ![]() us listening in without an appreciation of cents, that makes (almost) a pence. 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 + } I can't test it, may be some one else has a MS SQL server running and can look Another thing... Class HsqlQueryExpression also tries to generate SQL including a LIMIT > And do not forget to close this bug once you've verified its correctness .. ;-
). OK, correctness tested - verified - certificate given. > 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 ? > 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) ? For SQL Server there is used the syntax "TOP n":
if ( _limit != null ) I just issued simple queries using the HSQL query tool, for example: select * And I get an "unexpected token: LIMIT in statement [limit]" error message. The following prints "Unexpected token: 5 in statement [5]": select * Found, what's wrong with HSQL.
The syntax should be: select limit 0 3 * and: select top 5 * Can you please supply me with unified patches ?
lol I don't yet have patches.
May be we should move this discussion and re-open bug 1221 for the HSQL patch. 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
|
Initial (partial) patch.