JDO
  1. JDO
  2. JDO-529

ChangeQuery - DISTINCT is expected even though it is not specified

    Details

      Description

      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.

      1. jdo-529.patch
        3 kB
        Craig L Russell
      2. JDO-529-addAscending.patch
        2 kB
        Michael Bouschen

        Activity

        Hide
        Craig L Russell added a comment -

        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.

        Show
        Craig L Russell added a comment - 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.
        Hide
        Craig L Russell added a comment -

        Please review this patch.

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

        Show
        Craig L Russell added a comment - Please review this patch. This patch adds DISTINCT to the SELECT clause for the JDOQL statements.
        Hide
        Ilan Kirsh added a comment -

        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?

        Show
        Ilan Kirsh added a comment - 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?
        Hide
        Craig L Russell added a comment -

        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 .

        Show
        Craig L Russell added a comment - 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 .
        Hide
        Ilan Kirsh added a comment -

        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.

        Show
        Ilan Kirsh added a comment - 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.
        Hide
        Craig L Russell added a comment -

        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.

        Show
        Craig L Russell added a comment - 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.
        Hide
        Craig L Russell added a comment -

        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.

        Show
        Craig L Russell added a comment - 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.
        Hide
        Andy Jefferson added a comment -

        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.

        Show
        Andy Jefferson added a comment - 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.
        Hide
        Michael Bouschen added a comment -

        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.

        Show
        Michael Bouschen added a comment - 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.
        Hide
        Craig L Russell added a comment -

        I agree.

        Show
        Craig L Russell added a comment - I agree.
        Hide
        Michael Bouschen added a comment -

        I checked in the patch (see revision 578069).

        Show
        Michael Bouschen added a comment - I checked in the patch (see revision 578069).
        Hide
        Ilan Kirsh added a comment -

        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

        Show
        Ilan Kirsh added a comment - 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

          People

          • Assignee:
            Craig L Russell
            Reporter:
            Ilan Kirsh
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development