|
Please review this patch.
This patch adds DISTINCT to the SELECT clause for the JDOQL statements. I still cannot pass this test, now because of the DISTINCT / ORDER BY combination.
DISTINCT is eventually a specific case of GROUP BY, and it should restrict the valid ORDER BY expressions, similarly to the GROUP BY description. For instance, if two persons have the same first name / last name, which of their IDs should we use to sort the results? The query orders by personid, which is not specified in the select expression. It's assumed that if the underlying query language (e.g. SQL) requires all order by terms to be selected, then the term is automatically added by the implementation.
In this case, it would result in SELECT DISTINCT personid, firstname, lastname which might be unexpected since personid is not used in the user-visible query result. The results might include multiple instances with different personid but the same firstname and lastname. Since this case is not adequately discussed in the specification, the test case should be changed. This patch changes the ordering expression to use selected fields and no unselected fields . Thanks, the patch looks good.
By the way, I read that such a DISTINCT / ORDER BY combination causes an error in Oracle, where MySQL chooses the first (arbitrary) personId for sort. Adding the order expression to the SELECT is a probably another (third) dialect. svn commit -m "
Sending tck2/src/java/org/apache/jdo/tck/query/api/ChangeQuery.java Transmitting file data . Committed revision 577401. Missed the tck2-legacy in the above commit.
svn commit -m " Sending tck2-legacy/src/java/org/apache/jdo/tck/query/api/ChangeQuery.java Transmitting file data . Committed revision 577406. This query now has the ordering constraint of
firstname, lastname ASCENDING If I read the spec 14.6.6 it says "The ordering statement is a String containing one or more ordering declarations separated by commas. Each ordering declaration is a Java expression of an orderable type: • primitives (boolean is non-portable); • wrappers (Boolean is non-portable); • BigDecimal; • BigInteger; • String; • Date followed by one of the following words: "ascending", "descending","asc", or "desc"." So shouldn't the ordering constraint should be firstname ASCENDING, lastname ASCENDING ??? "firstname ASCENDING" is an "ordering declaration" using spec verbage, as is "lastname ASCENDING". Unless we're having inbuilt default of ASCENDING, but I didn't read that from the spec. I agree with what Andy said. We need to add the keyword ASCENDING such that the ordering constraint becomes: firstname ASCENDING, lastname ASCENDING
Attached you find a patch ( I checked in the patch (see revision 578069).
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Test
Resolved
Major
ChangeQuery - DISTINCT is expected even though it is not specified
The query is
"SELECT firstname, lastname INTO FullName FROM FullTimeEmployee " +
"WHERE salary > 1000 & projects.contains(p) & " +
"p.budget > limit " +
"VARIABLES Project p PARAMETERS BigDecimal limit " +
"ORDER BY personid ASCENDING RANGE 0, 5";
Evaluation is done by 14.6.9:
The candidate tuples are
the cartesian product of the candidate class and all variables used in the result. The result tuples are
the tuples of the candidate class and all variables used in the result that satisfy the filter. The result
is the collection of result expressions projected from the result tuples.
The candidate tuples (the cartesian product of the three full time employees (1, 2, 5) with the three projects (1, 2, 3)) has nine tuples.
Of these nine, the result tuples are only {(emp1, proj1), (emp2, proj1), (emp2, proj2), (emp5, proj3)}, that satisfy the condition.
The projection should have four result objects.