Details

      Description

      In 14, add subqueries to permit e.g. select from Employee where this.salary > (select avg(salary) from Employee)

      1. subquery-tck.patch
        60 kB
        Michael Bouschen
      2. subquery-api.patch
        10 kB
        Michael Bouschen

        Activity

        Hide
        Craig L Russell added a comment -

        Please review this patch.

        This patch updates the Query interface by adding the method to support subqueries
        void addSubquery
        (Query sub, String variableDeclaration, String candidateCollectionExpression);

        Show
        Craig L Russell added a comment - Please review this patch. This patch updates the Query interface by adding the method to support subqueries void addSubquery (Query sub, String variableDeclaration, String candidateCollectionExpression);
        Hide
        Craig L Russell added a comment -

        svn commit -m "JDO-446 Add addSubquery api to Query" api20/src/java/javax/jdo/Query.java
        Sending api20/src/java/javax/jdo/Query.java
        Transmitting file data .
        Committed revision 509811.

        Show
        Craig L Russell added a comment - svn commit -m " JDO-446 Add addSubquery api to Query" api20/src/java/javax/jdo/Query.java Sending api20/src/java/javax/jdo/Query.java Transmitting file data . Committed revision 509811.
        Hide
        Craig L Russell added a comment -

        We now need tck test cases for this feature.

        Show
        Craig L Russell added a comment - We now need tck test cases for this feature.
        Hide
        Michael Bouschen added a comment -

        Please review the patch subquery-api.patch. It adds the missing addSubquery methods to the JDO Query interface.

        Show
        Michael Bouschen added a comment - Please review the patch subquery-api.patch. It adds the missing addSubquery methods to the JDO Query interface.
        Hide
        Michael Bouschen added a comment -

        Added an updated version of the patch (subquery-api.patch).

        Show
        Michael Bouschen added a comment - Added an updated version of the patch (subquery-api.patch).
        Hide
        Craig L Russell added a comment -

        Hi Michael,

        As I was reading the patch, I thought that it might be better if we eliminated most of the javadoc that is duplicated in the four addSubquery methods. This is because some of the wording used in the original patch has been changed in the specification to make it clearer. Specifically, the specification now says
        This method adds a subquery to this Query. A subquery is composed as a Query and subsequently attached to a different Query (the outer Query) by calling this method. The String parameters are trimmed of white space.
        The Query parameter instance is unmodified as a result of the addSubquery or subsequent execution of the outer Query. Only some of the parameter query parts are copied for use as the subquery. The parts copied include the candidate class, filter, parameter declarations, variable declarations, imports, ordering specification, uniqueness, result specification, and grouping specification. The association with a PersistenceManager, the candidate collection or extent, result class, and range limits are not used.
        The variableDeclaration parameter is the name of the variable containing the results of the subquery execution. If the same value of variableDeclaration is used to add multpile subqueries, the subquery replaces the previous subquery for the same named variable. If the subquery parameter is null, the variable is unset, effectively making the variable named in the variableDeclaration unbound. If the trimmed value is the empty String, or the parameter is null, then JDOUserException is thrown.
        The candidateCollectionExpression is the expression from the outer query that represent the candidates over which the subquery is evaluated. If the trimmed value is the empty String, or the parameter is null, then the candidate collection is the extent of the candidate class.
        When comments on the spec wording are all resolved, we should put them back into the javadoc. So I'm thinking that maybe we should avoid copying and pasting into four different methods.

        What I don't understand well is the impact on javadoc for code completion. If we have the full javadoc only on the first method, and the paragraph on the parameter handling is mostly in the single string version, do we hurt the usability for users depending on javadoc for code completion? I'm pretty sure that javadoc for regular users will not suffer if there is a @see reference that points to the full description in the other method.

        Any comments from code completion fans?

        Show
        Craig L Russell added a comment - Hi Michael, As I was reading the patch, I thought that it might be better if we eliminated most of the javadoc that is duplicated in the four addSubquery methods. This is because some of the wording used in the original patch has been changed in the specification to make it clearer. Specifically, the specification now says This method adds a subquery to this Query. A subquery is composed as a Query and subsequently attached to a different Query (the outer Query) by calling this method. The String parameters are trimmed of white space. The Query parameter instance is unmodified as a result of the addSubquery or subsequent execution of the outer Query. Only some of the parameter query parts are copied for use as the subquery. The parts copied include the candidate class, filter, parameter declarations, variable declarations, imports, ordering specification, uniqueness, result specification, and grouping specification. The association with a PersistenceManager, the candidate collection or extent, result class, and range limits are not used. The variableDeclaration parameter is the name of the variable containing the results of the subquery execution. If the same value of variableDeclaration is used to add multpile subqueries, the subquery replaces the previous subquery for the same named variable. If the subquery parameter is null, the variable is unset, effectively making the variable named in the variableDeclaration unbound. If the trimmed value is the empty String, or the parameter is null, then JDOUserException is thrown. The candidateCollectionExpression is the expression from the outer query that represent the candidates over which the subquery is evaluated. If the trimmed value is the empty String, or the parameter is null, then the candidate collection is the extent of the candidate class. When comments on the spec wording are all resolved, we should put them back into the javadoc. So I'm thinking that maybe we should avoid copying and pasting into four different methods. What I don't understand well is the impact on javadoc for code completion. If we have the full javadoc only on the first method, and the paragraph on the parameter handling is mostly in the single string version, do we hurt the usability for users depending on javadoc for code completion? I'm pretty sure that javadoc for regular users will not suffer if there is a @see reference that points to the full description in the other method. Any comments from code completion fans?
        Hide
        Craig L Russell added a comment -

        Here's what the spec currently says about Map method:
        The Map version of the method treats the key of each map entry as the name of the parameter in the subquery, with or without the leading ":", and the value as the name of the expression in the outer query. If the trimmed expression is the empty String for either the parameter or the value of the String[], or for any map key or value, that expression is ignored.

        And for the Object... method:
        The String[] version of the method binds the named expressions in turn to parameters in the order in which they are declared in the subquery, or in the order they are found in the filter if not explicitly declared in the subquery.

        Show
        Craig L Russell added a comment - Here's what the spec currently says about Map method: The Map version of the method treats the key of each map entry as the name of the parameter in the subquery, with or without the leading ":", and the value as the name of the expression in the outer query. If the trimmed expression is the empty String for either the parameter or the value of the String[], or for any map key or value, that expression is ignored. And for the Object... method: The String[] version of the method binds the named expressions in turn to parameters in the order in which they are declared in the subquery, or in the order they are found in the filter if not explicitly declared in the subquery.
        Hide
        Michael Bouschen added a comment -

        I agree there is no need to duplicate the javadoc, especially as we do no duplicate the javadoc description in other places of the JDO API (e.g. overloaded PersistenceManager methods).

        I updated the patch, you find attached for review. Now the String... version of the method has the full javadoc and all the other addSubquery methods refer to this.

        Show
        Michael Bouschen added a comment - I agree there is no need to duplicate the javadoc, especially as we do no duplicate the javadoc description in other places of the JDO API (e.g. overloaded PersistenceManager methods). I updated the patch, you find attached for review. Now the String... version of the method has the full javadoc and all the other addSubquery methods refer to this.
        Hide
        Craig L Russell added a comment -

        Two questions.

        1. The javadoc for param parameter[s] say
        + * @param parameters the actual values of the subquery parameters

        But it isn't the actual values, it's the name of the expression in the outer query to bind the parameter. The description is ok, but the @param should probably say

        • @param parameter the expression from the outer query to bind the parameter in the subquery

        2. Should we consider one more method, with two arguments, that has the candidate collection be the extent of the candidate class of the subquery? This would avoid the need for passing null as the candidateCollectionExpression. Might be too much of a corner case.

        Show
        Craig L Russell added a comment - Two questions. 1. The javadoc for param parameter [s] say + * @param parameters the actual values of the subquery parameters But it isn't the actual values, it's the name of the expression in the outer query to bind the parameter. The description is ok, but the @param should probably say @param parameter the expression from the outer query to bind the parameter in the subquery 2. Should we consider one more method, with two arguments, that has the candidate collection be the extent of the candidate class of the subquery? This would avoid the need for passing null as the candidateCollectionExpression. Might be too much of a corner case.
        Hide
        Michael Bouschen added a comment -

        Hi Craig,

        1. Good catch. I will change the @param javadoc as proposed before I check in the patch.

        2. I think there are usecases where the subquery uses the extent as the candidate collection. However, I'm not sure whether it is worth adding a new method. Moreover, I think we would need to add 4 new methods: the one you proposed plus 3 more handling subquery parameters.

        Regards Michael

        Show
        Michael Bouschen added a comment - Hi Craig, 1. Good catch. I will change the @param javadoc as proposed before I check in the patch. 2. I think there are usecases where the subquery uses the extent as the candidate collection. However, I'm not sure whether it is worth adding a new method. Moreover, I think we would need to add 4 new methods: the one you proposed plus 3 more handling subquery parameters. Regards Michael
        Hide
        Michael Bouschen added a comment -

        Checked in the subquery API patch (see revision 591836).

        Show
        Michael Bouschen added a comment - Checked in the subquery API patch (see revision 591836).
        Hide
        Michael Bouschen added a comment -

        Attached you find the first version of the tck test cases (subquery-tck.patch).
        I added a new package org.apache.jdo.tck.query.jdoql.subqueries and a new test class per subquery assertion. The test data file companyForSubqueriesTests.xml defines the test data for the subquery tests.

        Show
        Michael Bouschen added a comment - Attached you find the first version of the tck test cases (subquery-tck.patch). I added a new package org.apache.jdo.tck.query.jdoql.subqueries and a new test class per subquery assertion. The test data file companyForSubqueriesTests.xml defines the test data for the subquery tests.
        Hide
        Craig L Russell added a comment -

        Looks good. Just a few comments:

        1. + void runTestSuqueriesXX(PersistenceManager pm) {
        probably should be
        + void runTestSubqueriesXX(PersistenceManager pm) {

        2. Just a thought. Should we run queries against memory model instances as well?

        3. The new/updated classes apply to both tck2 and tck2-legacy, right?

        Show
        Craig L Russell added a comment - Looks good. Just a few comments: 1. + void runTestSuqueriesXX(PersistenceManager pm) { probably should be + void runTestSubqueriesXX(PersistenceManager pm) { 2. Just a thought. Should we run queries against memory model instances as well? 3. The new/updated classes apply to both tck2 and tck2-legacy, right?
        Hide
        Michael Bouschen added a comment -

        Attached you find an updated version of the patch subquery-tck.patch:

        • I fixed the typo in the method name (should be runTestSubqueriesXX)
        • I added tests against the a memory collection containing all employees to three of the new test classes.

        Yes, the changes apply to both tck2 and tck2-legacy.

        Show
        Michael Bouschen added a comment - Attached you find an updated version of the patch subquery-tck.patch: I fixed the typo in the method name (should be runTestSubqueriesXX) I added tests against the a memory collection containing all employees to three of the new test classes. Yes, the changes apply to both tck2 and tck2-legacy.
        Hide
        Craig L Russell added a comment -

        Looks good.

        Craig

        Show
        Craig L Russell added a comment - Looks good. Craig
        Hide
        Michael Bouschen added a comment -

        Checked in the patch (see revision 609405).

        The test cases currently fail because JPOX does not yet supports subqueries (see JDO-568).

        Show
        Michael Bouschen added a comment - Checked in the patch (see revision 609405). The test cases currently fail because JPOX does not yet supports subqueries (see JDO-568 ).
        Hide
        Andy Jefferson added a comment -

        Minor point : the queries seem to assume a field "firstName" in Person yet it is "firstname"

        Show
        Andy Jefferson added a comment - Minor point : the queries seem to assume a field "firstName" in Person yet it is "firstname"
        Hide
        Michael Bouschen added a comment -

        Good catch! I fixed the name of the field (see revision 611476).

        Show
        Michael Bouschen added a comment - Good catch! I fixed the name of the field (see revision 611476).

          People

          • Assignee:
            Michael Bouschen
            Reporter:
            Michelle Caisse
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development