Issue Details (XML | Word | Printable)

Key: JDO-529
Type: Test Test
Status: Resolved Resolved
Resolution: Fixed
Priority: Major Major
Assignee: Craig Russell
Reporter: Ilan Kirsh
Votes: 0
Watchers: 0
Operations

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

ChangeQuery - DISTINCT is expected even though it is not specified

Created: 16/Sep/07 06:06 PM   Updated: 03/Nov/07 05:42 AM
Return to search
Component/s: tck2
Affects Version/s: None
Fix Version/s: JDO 2 maintenance release 1

Time Tracking:
Not Specified

File Attachments:
  Size
Text File Licensed for inclusion in ASF works JDO-529-addAscending.patch 2007-09-20 12:18 PM Michael Bouschen 2 kB
Text File Licensed for inclusion in ASF works jdo-529.patch 2007-09-18 06:16 PM Craig Russell 3 kB

Resolution Date: 19/Sep/07 05:52 PM


 Description  « Hide
Test org.apache.jdo.tck.query.api.ChangeQuery expects 3 result objects:

List expectedResult = Arrays.asList(new Object[] {
            new FullName("emp1First", "emp1Last"),
            new FullName("emp2First", "emp2Last"),
            new FullName("emp5First", "emp5Last")});

But actually there should be 4 result objects:

List expectedResult = Arrays.asList(new Object[] {
            new FullName("emp1First", "emp1Last"),
            new FullName("emp2First", "emp2Last"),
            new FullName("emp2First", "emp2Last"),
            new FullName("emp5First", "emp5Last")});

because the result is not specified as DISTINCT.


 All   Comments   Work Log   Change History   Subversion Commits      Sort Order: Ascending order - Click to sort in descending order
Craig Russell added a comment - 16/Sep/07 08:02 PM
Right.

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.

Craig Russell added a comment - 18/Sep/07 05:23 PM
Please review this patch.

This patch adds DISTINCT to the SELECT clause for the JDOQL statements.

Craig Russell made changes - 18/Sep/07 05:23 PM
Field Original Value New Value
Attachment jdo-529.patch [ 12366105 ]
Ilan Kirsh added a comment - 18/Sep/07 05:38 PM
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?

Craig Russell added a comment - 18/Sep/07 06:16 PM
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 .

Craig Russell made changes - 18/Sep/07 06:16 PM
Attachment jdo-529.patch [ 12366111 ]
Craig Russell made changes - 18/Sep/07 06:16 PM
Attachment jdo-529.patch [ 12366105 ]
Ilan Kirsh added a comment - 18/Sep/07 06:30 PM
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.

Repository Revision Date User Message
ASF #577401 Wed Sep 19 17:51:41 UTC 2007 clr JDO-529 Add DISTINCT to query in ChangeQuery
Files Changed
MODIFY /db/jdo/trunk/tck2/src/java/org/apache/jdo/tck/query/api/ChangeQuery.java

Craig Russell added a comment - 19/Sep/07 05:52 PM
svn commit -m "JDO-529 Add DISTINCT to query in ChangeQuery" tck2/src/java/org/apache/jdo/tck/query/api/ChangeQuery.java tck2/src/java/org/apache/jdo/tck/query/api/ChangeQuery.java
Sending tck2/src/java/org/apache/jdo/tck/query/api/ChangeQuery.java
Transmitting file data .
Committed revision 577401.

Craig Russell made changes - 19/Sep/07 05:52 PM
Resolution Fixed [ 1 ]
Fix Version/s JDO 2 maintenance release 1 [ 12310923 ]
Status Open [ 1 ] Resolved [ 5 ]
Assignee Craig Russell [ clr ]
Repository Revision Date User Message
ASF #577406 Wed Sep 19 18:04:14 UTC 2007 clr JDO-529 Add DISTINCT to query in ChangeQuery
Files Changed
MODIFY /db/jdo/trunk/tck2-legacy/src/java/org/apache/jdo/tck/query/api/ChangeQuery.java

Craig Russell added a comment - 19/Sep/07 06:05 PM
Missed the tck2-legacy in the above commit.

svn commit -m "JDO-529 Add DISTINCT to query in ChangeQuery" tck2-legacy/src/java/org/apache/jdo/tck/query/api/ChangeQuery.java
Sending tck2-legacy/src/java/org/apache/jdo/tck/query/api/ChangeQuery.java
Transmitting file data .
Committed revision 577406.

Andy Jefferson added a comment - 20/Sep/07 11:29 AM
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.

Michael Bouschen added a comment - 20/Sep/07 12:18 PM
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 (JDO-529-addAscending.patch) that adds the keyword ASCENDING.

Michael Bouschen made changes - 20/Sep/07 12:18 PM
Attachment JDO-529-addAscending.patch [ 12366279 ]
Craig Russell added a comment - 20/Sep/07 02:36 PM
I agree.

Repository Revision Date User Message
ASF #578069 Fri Sep 21 11:26:26 UTC 2007 mbo JDO-529: add keyword ASCENDING to the ordering constraint
Files Changed
MODIFY /db/jdo/trunk/tck2/src/java/org/apache/jdo/tck/query/api/ChangeQuery.java
MODIFY /db/jdo/trunk/tck2-legacy/src/java/org/apache/jdo/tck/query/api/ChangeQuery.java

Michael Bouschen added a comment - 21/Sep/07 11:27 AM
I checked in the patch (see revision 578069).

Ilan Kirsh added a comment - 03/Nov/07 05:42 AM
Hi Craig,

Thanks for the prompt response. I will appreciate if you can check also
JDO-513 and JDO-514 when you have the time.

Regards, Ilan