Eliminate the parser's rewriting of the abstract syntax tree for queries with GROUP BY and/or HAVING clauses

• Key: DERBY-681
• Type: Improvement
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Manish Khettry
• Reporter: Rick Hillegas
• Component/s: SQL
• Fix Version/s: 10.3.1.4

Description

If a query contains a GROUP BY or HAVING clause, the parser rewrites the abstract syntax tree, putting aggregates into a subselect and treating the HAVING clause as the WHERE clause of a fabricated outer select from the subquery. This allows the compiler to re-use some machinery since the HAVING clause operates on the grouped result the way that the WHERE clause operates on the from list. Unfortunately, this rewriting creates an explosion of special cases in the compiler after parsing is done. The rewriting is not systematically handled later on in the compiler. This gives rise to defects like bug 280. We need to eliminate this special rewriting and handle the HAVING clause in a straightforward way. This is not a small bugfix but is a medium sized project.

Comments

[Daniel John Debrunner]

Seems more like an improvement than a bug fix.

[Satheesh Bandaram]

Should this be marked an Enhancement request? While I agree the current scheme of rewriting the query can be improved, it does achieve the functionality. Bugs in the current implementation have been logged separately. (like DERBY-280) Changing the mechanism should be marked an Enhancement, I think.

[Rick Hillegas]

Linking this bug to 681, the jira which tracks the rewriting of the parser's GROUP BY/HAVING logic.

[Andrew McIntyre]

Linking this issue to DERBY-1624, in which a query generated by Hibernate fails on Derby due to the way queries that include GROUP BY w/ HAVING are rewritten by the parser.

[Manish Khettry]

I was looking at the query tree munging that we do in the GroupByNode and it seems that doing something with the having clause should be possible. Basically we always put a ProjectRestrictNode on top of a GroupByNode. It should be possible to rewrite the having clause and turn it into a restriction of this ProjectRestrictNode. This rewriting of the query tree should be pretty similar to DERBY-883.

Is this what you mean by "handle the having clause in a starightforward way"? If anyone has thought about this bug and have any ideas on this, please do share. I can work on this next-- it should help with atleast a couple of other bugs (DERBY-280 and DERBY-1624) but wanted to check with others on the list before I start working on it.

[Andrew McIntyre]

Hi Manish,

I took a look at the current code for group by a while ago in relation to DERBY-1624 and eventually gave up as correctly fixing the issue went beyond my knowledge of the parsing/compilation phase of query processing and my available time. I've come across blog posts like this one:

http://blog.tremend.ro/2006/10/03/about-the-maturity-of-apache-derby/

in which the main difficulty was the inability to reference column aliases from the select list in group by clauses, so I think this is a worthwhile project to work on.

I came up with a band-aid of a fix which addressed DERBY-1624 by copying the relevant base column info and correlation name for the aliased columns in the rewritten subquery into the ColumnReferences for the result column list of the inner subquery, for which the correlation name could then be compared with the names of the result columns in the outer select with the group by clause to resolve the correct base column. IIRC, this approach worked for some cases, but there were failures in lang/groupby.sql that I didn't have time to track down.

I think a proper fix, as mentioned in the description of this issue, involves eliminating the rewriting of the query that happens in sqlgrammar.jj. If for some reason it might help you, though, I could try and find the patch for DERBY-1624 that I had worked on earlier.

[Manish Khettry]

Thanks Andrew. I hope that I can eliminate the "group/having" query rewrite and this should fix DERBY-1624.

On the blog entry-- I do think that being able to use aliases in the query (group by or where clause) would be helpful. I remember seeing a bug about this issue a while ago.

[Manish Khettry]

This patch removes the "wrap group by's in a subselect" rewrite in the parser. It preserves the having clause through bind and optimize phases and during the final rewrite for aggregates in the GroupByNode, transforms the having clause to a valid restriction. I am also attaching a text file which should clarify the changes.

This patch also fixes related bugs DERBY-1624, the regresssion introduced by DERBY-280 and also completes the functionality for DERBY-883.

If this patch is approved and comitted I will file another bug to remove references to the flags generatedForGroupBy and geneatedForHaving which is dead code post this patch. I did not remove these references because it would make the patch even bigger and more complicated.

[Manish Khettry]

Notes on this patch.

[A B]

I noticed that this issue is now on the bottom of the "patch available" list--and that the patch was posted almost two months ago. So I did a quick review and my comments are below. Note: The patch is out of date (not surprisingly); it would be great if a newer version could be re-attached that incorporates the following comments (I am willing to continue the review/discussion until the patch is committed).

Oh, and thank you for the "notes.txt" file, Manish! I found it to be very helpful as I went through the code changes. In that document you wrote (at the very end):

> Also the one thing that I cannot explain in this patch: the fetch size
> in some ResultSet nodes has gone from 1 to 16. If this is serious I can
> dig in to see why this is happening.

I think the difference between a fetch size of 1 and a fetch size of 16 comes down to the difference between a TableScan and BulkTableScan. I did a quick search of the codeline and it looks like Derby will disable bulk fetching if the result set is "ordering dependent" (see SelectNode.genProjectRestrict()) and also for certain min/max optimizations in a group by (see the "considerPostOptimizeOptimization()" method of GroupByNode). My guess is that your changes have somehow made it so that we no longer need to disable bulk fetch for certain queries, and thus you are now seeing a different fetch size. This seems particular likely since all of the queries that show a 1 vs 16 difference in aggregateOptimization() come under the heading of "group by ordered on grouping columns". So long story short, my feeling is that this is an acceptable diff...

Hopefully someone will correct me if I'm wrong...

Also (w.r.t to "notes.txt" as attached to this issue):

  -- The notes you wrote for "Background on Group By" would be great as
  javadoc comments in the GroupByNode.addNewColumnsForAggregation() method
  (in addition to what's already there).

  -- The notes you wrote for "Having clause -> Design" would be great as
  comments in the GroupByNode.addAggregateColumns() method (perhaps just
  before the "if (havingClause != null)"...)

Other review comments (note: I haven't done much actual testing yet, I've just looked at the code changes; I hope to do more testing of the changes next week...):

I think it would good if the following issues could be addressed before commit:

  1) FromBaseTable.java:

  -- It looks like you added an "accept()" method to FromBaseTable that
  overrides ResultSetNode.accept(). I noticed that ResultSetNode.accept()
  recursively calls "accept" on "resultColumns", but the new method in
  FromBaseTable does not. This means that in cases where we used to accept
  visitors for base table result columns we will no longer do so. I don't
  know what the effects of that might be, but I think that's probably not
  good. It would perhaps be better to call "super.accept()" at the start
  of FromBaseTable.accept() and then go from there.

  2) GroupByNode.java:

  -- init() method has some lines that are commented out. You mention
  these in your "notes.txt" file but there is no explanation as to why
  they are commented out (and not just deleted) in the file itself;
  might be good to add such comments (you could just take them from
  notes.txt).

  3) ResultColumnList.java:

  -- With the following diff:

- int size = (size() > sourceRCL.size()) ? size() : sourceRCL.size();
+// int size = (size() > sourceRCL.size()) ? size() : sourceRCL.size();
+ int size = Math.min(size(), sourceRCL.size());

  The original line (which is now commented out) looks like it assigns size
  to be the *max* of size() and sourceRCL.size(); but your changes make it
  use the minimum. I looked at the code and I think your fix is correct--
  i.e. that we should be using the minimum size. So should the other line
  just be deleted (instead of commented out)? Also, there appears to be an
  implicit assumption that if the lists are two different sizes then the
  shorter one must correspond to the _leading_ columns of the longer one.
  If you're so inclined it might be nice to add a comment saying as much
  (to go along with your change here).

  4) ColumnReference.java:

  -- Unrelated (and perhaps accidental) code cleanup diff; better to leave
  this out of the patch.

  5) GroupByExpressionTest.java:

  -- In the "testSubSelect()" method of you added a test case that is
  identical to the one immediately preceding it (so far as I can tell).
  Was that intentional, or is there a HAVING clause missing?

The rest are nit-pick issues that I hope you might consider, though they should not block commit of the patch:

  6) SelectNode.java:

  -- Some unrelated code cleanup; not a big deal, but as a general rule
  it detracts from the patch (makes it hard to figure out what changes
  are related to the issue and what changes are not). Ex. See the
  bindNonVTITables() method.

  -- I wonder if we really need a new "init()" method here? As far as I
  can tell there are only two files that currently create a SELECT node:
  DeleteNode.java and sqlgrammar.jj. The latter uses the new init()
  method, the former uses the old one--but the only difference is the
  presence of the "havingClause" argument. DeleteNode already passes
  in several null values (with associated comments), so would it make
  sense to just add a null for "havingClause", as well? Then we would
  only need the one "init()" method for SelectNode. I admit, though,
  that this a stylistic detail and the code as you have it is correct.
  So if you prefer to leave it as it is, that's fine--although it might
  might be good to add some minimal javadoc to the new init() method
  (ex. "@param havingClause The HAVING clause, if any", to keep in line
  with the old init() method).

  7) GroupByNode.java:

  -- Unnecessary import of JBitSet and Properties.

  -- Javadoc for "init()" method doesn't mention "nestingLevel"

  -- Looks like the following comment was deleted:

- // finally reset gbc to its new position.

    Was that intentional?

  8) GroupByList.java:

  -- Might be nice if you could add some comments for the following line:

+ selectRCL.setCountMismatchAllowed(true);

    i.e. maybe explain briefly why count mismatches are allowed for GroupBy?

  9) ResultColumnList.java:

  -- With your changes there is now the following "if" statement in "propagateDCLInfo()":

    /* Do both lists, if supplied by user, have the same degree? */
    if (derivedRCL.size() != size() &&
        ! derivedRCL.getCountMismatchAllowed())
    {
        if (visibleSize() != derivedRCL.size()) {
            throw StandardException.newException(SQLState.LANG_DERIVED_COLUMN_LIST_MISMATCH, tableName);
        }
    }

  Could this be rewritten to:

    /* Do both lists, if supplied by user, have the same degree? */
    if (derivedRCL.size() != visibleSize() &&
        ! derivedRCL.getCountMismatchAllowed())
    {
        throw StandardException.newException(SQLState.LANG_DERIVED_COLUMN_LIST_MISMATCH, tableName);
    }

As I said I haven't looked much at the new test cases nor have I done much testing myself. I will hopefully have time to do more of that next week...

[Manish Khettry]

Thanks for reviewing the patch. It will take me sometime to make the patch current and look at your comments. It has, after all, been a while since I submitted the patch.

I am curious-- is it typical for a patch to gather dust for a few months before someone finds the time to look at it? And if so, is this a good thing?

Thanks,
Manish

"A B (JIRA)" <[e-mail]> wrote:
    [ https://issues.apache.org/jira/browse/DERBY-681?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12471852 ]

A B commented on DERBY-681:
---------------------------

I noticed that this issue is now on the bottom of the "patch available" list--and that the patch was posted almost two months ago. So I did a quick review and my comments are below. Note: The patch is out of date (not surprisingly); it would be great if a newer version could be re-attached that incorporates the following comments (I am willing to continue the review/discussion until the patch is committed).



I think the difference between a fetch size of 1 and a fetch size of 16 comes down to the difference between a TableScan and BulkTableScan. I did a quick search of the codeline and it looks like Derby will disable bulk fetching if the result set is "ordering dependent" (see SelectNode.genProjectRestrict()) and also for certain min/max optimizations in a group by (see the "considerPostOptimizeOptimization()" method of GroupByNode). My guess is that your changes have somehow made it so that we no longer need to disable bulk fetch for certain queries, and thus you are now seeing a different fetch size. This seems particular likely since all of the queries that show a 1 vs 16 difference in aggregateOptimization() come under the heading of "group by ordered on grouping columns". So long story short, my feeling is that this is an acceptable diff...

Hopefully someone will correct me if I'm wrong...

Also (w.r.t to "notes.txt" as attached to this issue):

  -- The notes you wrote for "Background on Group By" would be great as
  javadoc comments in the GroupByNode.addNewColumnsForAggregation() method
  (in addition to what's already there).

  -- The notes you wrote for "Having clause -> Design" would be great as
  comments in the GroupByNode.addAggregateColumns() method (perhaps just
  before the "if (havingClause != null)"...)

Other review comments (note: I haven't done much actual testing yet, I've just looked at the code changes; I hope to do more testing of the changes next week...):

I think it would good if the following issues could be addressed before commit:

  1) FromBaseTable.java:

  -- It looks like you added an "accept()" method to FromBaseTable that
  overrides ResultSetNode.accept(). I noticed that ResultSetNode.accept()
  recursively calls "accept" on "resultColumns", but the new method in
  FromBaseTable does not. This means that in cases where we used to accept
  visitors for base table result columns we will no longer do so. I don't
  know what the effects of that might be, but I think that's probably not
  good. It would perhaps be better to call "super.accept()" at the start
  of FromBaseTable.accept() and then go from there.

  2) GroupByNode.java:

  -- init() method has some lines that are commented out. You mention
  these in your "notes.txt" file but there is no explanation as to why
  they are commented out (and not just deleted) in the file itself;
  might be good to add such comments (you could just take them from
  notes.txt).

  3) ResultColumnList.java:

  -- With the following diff:

- int size = (size() > sourceRCL.size()) ? size() : sourceRCL.size();
+// int size = (size() > sourceRCL.size()) ? size() : sourceRCL.size();
+ int size = Math.min(size(), sourceRCL.size());

  The original line (which is now commented out) looks like it assigns size
  to be the *max* of size() and sourceRCL.size(); but your changes make it
  use the minimum. I looked at the code and I think your fix is correct--
  i.e. that we should be using the minimum size. So should the other line
  just be deleted (instead of commented out)? Also, there appears to be an
  implicit assumption that if the lists are two different sizes then the
  shorter one must correspond to the _leading_ columns of the longer one.
  If you're so inclined it might be nice to add a comment saying as much
  (to go along with your change here).

  4) ColumnReference.java:

  -- Unrelated (and perhaps accidental) code cleanup diff; better to leave
  this out of the patch.

  5) GroupByExpressionTest.java:

  -- In the "testSubSelect()" method of you added a test case that is
  identical to the one immediately preceding it (so far as I can tell).
  Was that intentional, or is there a HAVING clause missing?

The rest are nit-pick issues that I hope you might consider, though they should not block commit of the patch:

  6) SelectNode.java:

  -- Some unrelated code cleanup; not a big deal, but as a general rule
  it detracts from the patch (makes it hard to figure out what changes
  are related to the issue and what changes are not). Ex. See the
  bindNonVTITables() method.

  -- I wonder if we really need a new "init()" method here? As far as I
  can tell there are only two files that currently create a SELECT node:
  DeleteNode.java and sqlgrammar.jj. The latter uses the new init()
  method, the former uses the old one--but the only difference is the
  presence of the "havingClause" argument. DeleteNode already passes
  in several null values (with associated comments), so would it make
  sense to just add a null for "havingClause", as well? Then we would
  only need the one "init()" method for SelectNode. I admit, though,
  that this a stylistic detail and the code as you have it is correct.
  So if you prefer to leave it as it is, that's fine--although it might
  might be good to add some minimal javadoc to the new init() method
  (ex. "@param havingClause The HAVING clause, if any", to keep in line
  with the old init() method).

  7) GroupByNode.java:

  -- Unnecessary import of JBitSet and Properties.

  -- Javadoc for "init()" method doesn't mention "nestingLevel"

  -- Looks like the following comment was deleted:

- // finally reset gbc to its new position.

    Was that intentional?

  8) GroupByList.java:

  -- Might be nice if you could add some comments for the following line:

+ selectRCL.setCountMismatchAllowed(true);

    i.e. maybe explain briefly why count mismatches are allowed for GroupBy?

  9) ResultColumnList.java:

  -- With your changes there is now the following "if" statement in "propagateDCLInfo()":

    /* Do both lists, if supplied by user, have the same degree? */
    if (derivedRCL.size() != size() &&
        ! derivedRCL.getCountMismatchAllowed())
    {
        if (visibleSize() != derivedRCL.size()) {
            throw StandardException.newException(SQLState.LANG_DERIVED_COLUMN_LIST_MISMATCH, tableName);
        }
    }

  Could this be rewritten to:

    /* Do both lists, if supplied by user, have the same degree? */
    if (derivedRCL.size() != visibleSize() &&
        ! derivedRCL.getCountMismatchAllowed())
    {
        throw StandardException.newException(SQLState.LANG_DERIVED_COLUMN_LIST_MISMATCH, tableName);
    }

As I said I haven't looked much at the new test cases nor have I done much testing myself. I will hopefully have time to do more of that next week...


--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.



 
---------------------------------
TV dinner still cooling?
Check out "Tonight's Picks" on Yahoo! TV.

[A B]

Manish Khettry (JIRA) wrote:
>
> Thanks for reviewing the patch. It will take me sometime to make
> the patch current and look at your comments. It has, after all,
> been a while since I submitted the patch.

Thank you for willingness to continue working with the patch.

> I am curious-- is it typical for a patch to gather dust for a
> few months before someone finds the time to look at it?

I don't think it's "typical" for this to happen, no. But given the "fry your own fish" philosophy of open source development, this kind of thing does (unfortunately) happen on occasion.

The derby-dev mailing list receives a daily "subscription" email which lists issues having the "Patch Available" flag set. The issues are sorted according to the date of that last update/comment; those at the bottom of the list are the ones that have been sitting the longest with no activity. Ex.:

  http://thread.gmane.org/gmane.comp.apache.db.derby.devel/36563

That said, though, it's up to people in the community (users, developers, anyone--doesn't just have to be committers) to review patches and/or comment on the various issues according to their own interest/expertise. And as you've seen, sometimes the result is that things slip through the cracks.

I noticed DERBY-681 at the bottom of the list last week, which is what prompted me to look it up--and in doing so I saw that it had been idle for two months(!). So as I mentioned in my previous comment, I am willing to work with you on this patch so that it can now be committed. Apologies for the awfully long delay...

It's definitely not a good thing to have patches sitting for so long without review. While the decision as to who reviews what patches is entirely up to those in the community, you should feel free to "push" your patch if it's not getting any attention. See "Extra Mile" advice on the wiki:

  http://wiki.apache.org/db-derby/PatchAdvice

And in particular, note the following quote:

"Post to the list at least every couple of weeks if you don't get review. Ask if someone is available to look at your patch and what additional information reviewers might need for review."

I have found that sending an explicit email to derby-dev asking for people to review a patch which has been idle for a couple of weeks almost always leads to a reply and a subsequent review. So feel free to do so!

[A B]

I spent some time manually trying out different test cases with the patch for this issue and things seem to work well for the most part (see below). I ran the new test cases that this patch adds to GroupByExpressionTest and they behave as expected: i.e. they pass with the patch and fail without it.

I also ran the repro case for DERBY-280 and verified that Yes, the query still runs correctly with these changes. So that's good :)

Regarding "for most the part": I did find a couple of scenarios where d681.patch.txt causes queries to fail where they used to succeed. I found these while playing with the DDL from the queries attached to DERBY-1624, namely:

  create table o (name varchar(20), ord int);
  create table a (ord int, amount int);

With these tables and some minimal data, I noticed that the following statement, which used to work, fails with error 42X56 after applying d681.patch:

  create view v1 (vx, vy) as
    select name, sum(ord) from o where ord > 0 group by name, ord
      having ord <= ANY (select ord from a);

I think the problem is that there is a check in CreateViewNode which uses the "size()" method of ResultColumnList instead of the new "visibleSize()" method. When I made that change the above statement executes as normal.

I then found another example that shows a similar problem:

  create table ov (ox, oy) as
    (select name, sum(ord) from o where ord > 0 group by ord, name
      having ord <= ANY (select ord from a)) with no data;

I think the underlying problem is again the use of "size()" instead of "visibleSize()".

And if we assume the view "v1" from above is made to work (by changing the appropriate "size()" call to "visibleSize()") then the following statement, which does a union between v1 and a query on v1, will end up failing, too:

    select vx, vy from v1
      union select vx, sum(vy) from v1 group by vx, vy having (vy / 2) > 15;

So that's three examples where use of "size()" instead of "visibleSize()" is leading to an error--and each of the three errors has its origin in a different file. Which makes me wonder: are there other places where this same change needs to be made but we haven't found them yet? Is there a reliable way to search the codeline for this kind of thing? In any event, it would be good if the respective files for those three examples could be updated as necessary.

Aside from the above issue I couldn't see any other functional problems with the patch. Thank you very much for your explanations of the diffs in the various tests; that saved me some time :)

On an entirely different note, I noticed that when the patch was posted you included a comment saying:

  "This patch also fixes related bugs DERBY-1624 [...]"

When I read that I assumed that the queries attached to DERBY-1624 would now pass with d681.patch.txt--but that does not appear to be the case. Of the 18 queries in "1624_repro.sql", 10 pass when I apply the patch for DERBY-681 (only 7 pass without your changes). So we're getting better. However, it looks like there is still more work to do before DERBY-1624 can be considered resolved. Do you agree?

I have not run derbyall nor suites.All yet as I'm waiting for an updated patch before doing so...

In the meantime, thank you again for taking the time to tackle this particular issue--your contribution and your patience are both much appreciated.

[Manish Khettry]

Just a quick note to address two issues raised by Army.

DERBY-1624: It won't fix all the queries attached by Andrew for this bug because we don't resolve aliases in the group by/having/orderby clauses; the following query will fail. This bug does not address aliases-- only the original query with this bug.

select x as alias, sum(y)
from t
group by alias;

The size vs visible size is more problematic-- I really think the language layer should not be using RCL directly as much as it currently does. If it wants to check the number of columns returned by a ResultSetNode, it should call a method in the RSN called either visibleSize or runtimeSize or some such (i.e. the # of columns actually returned by the result set node). The only way to find all of these would be to grind through all the occurences of RCL.size! This in itself should suggest that the use of these classes/methods in this little area itself (i.e. # of columns in a result set node) could deal with a good dose of refactoring.

Thanks for looking at the patch, running tests and asking the right questions. I appreciate the time you've taken for this patch. I am a little busy at work but will get back to this bug when things let up (hopefully next week).

[Andrew McIntyre]

I checked Manish's patch and it does fix the original query reported not working in DERBY-1624. I applied Manish's patch and ran the original query in the description of DERBY-1624, and it works as expected.

While it would be great if column aliasing worked completely as expected with all of the queries in derby1624_repro.sql, I would argue that with Manish's patch committed DERBY-681 and DERBY-1624 should be closed, and a new issue opened to cover the remaining problems with column aliasing shown in the derby1624_repro script.

[A B]

> I would argue that with Manish's patch committed DERBY-681 and DERBY-1624 should be closed,
> and a new issue opened to cover the remaining problems with column aliasing shown in the
> derby1624_repro script.

Sounds good to me.

[Manish Khettry]

Again, thanks for very detailed and constructive review and for finding the problems with union/views/create table. I ran derbylang (one failure compressTable once, DERBY-2117?) and junit-all (one failure in a derbynet tearDown method?). Normally I would try to track this failure but this bugfix has been sitting on my laptop for too long and it really doesn't look like anything to do with my changes (famous last words?!)

I have accepted most of the review comments from A. B atleast partially except for 4, 6 and 9.

[A B]

Thank you for the second patch, Manish. I looked it over and it does indeed address most of my comments, so I think we're getting closer. The reason I say "closer" is because I noticed the following problems with the second patch, and I think these should be addressed before the patch can be committed:

  - Looks like the formatting for the first couple of lines in the license
    text for views.sql has changed. I assume this was just an accident?

  - This patch adds an explicit connection to views.sql, which I do not
    think is right:

        @@ -17,6 +14,7 @@
         -- tests for views
 
         -- set autocommit off
        +connect 'jdbc:derby:wombat';
         autocommit off;

    Notice that the result (in views.out) is a security access violation:

        @@ -17,7 +14,9 @@
         -- tests for views
 
         -- set autocommit off
        -autocommit off;
        +connect 'jdbc:derby:wombat';
        +JAVA ERROR: java.security.AccessControlException: access denied (java.util.PropertyPermission ij.URLCheck read)
        +ij> autocommit off;

    This looks wrong to me. Is the additional connect statement an intentional change,
    or is that just a remnant from some testing you were doing? If possible I think the
    extra call to "connect" should be removed and the master file updated accordingly.
    If you made the change intentionally then can you explain why it is necessary?

  - There is also the following diff in the RunTest and RunSuite classes:

- classpath = sp.getProperty("classpath");
+ classpath = sp.getProperty("java.class.path");

    Again, can you explain why this particular change is necessary? Were you
    experiencing problems while running the tests? If so, what were those problems?

Other than these test issues I think the patch is ready for commit...

[Manish Khettry]

removed runtest, runsuite. fixed views.

[A B]

I ran derbyall with the latest patch and there was one unexpected failure (Derby client only):

*** Start: setTransactionIsolation jdk1.4.2 DerbyNetClient derbynetmats:jdbcapi 2007-03-08 18:28:42 ***
532 del
< count=2, setTransactionIsolation() commits
532 add
> passCommitCheck=true
Test Failed.

However, I re-ran the test without your changes and it still failed, so I do not think it's related. The machine on which I ran derbyall is one I haven't used before so I don't know the specs; I'll look into it. In the meantime, I went ahead and committed 681_patch3.txt with svn # 516454:

   URL: http://svn.apache.org/viewvc?view=rev&rev=516454

Thank you for your patience throughout this process, Manish!

As a reminder for future patches: the more involved you are in the community as a whole--especially in terms of reviewing other people's patches/specs--the more likely it is that people will in turn respond to comments/patches you yourself post, and probably in a much more timely manner. That's no guarantee, of course (this is after all open source), but it certainly doesn't hurt ;)

In any event, thanks again for the contribution!

[A B]

Hi Manish,

I think I've found another case where use of "size()" instead of "visibleSize()" is leading to incorrect behavior after the patch for this issue. Take the following example:

ij> create table t1 (d double, vc varchar(20), x int);
0 rows inserted/updated/deleted

ij> select distinct vc, x from t1 as myt1
      where d <= (select max(myt1.d) from t1 as myt2
          where myt1.vc = myt2.vc and myt1.d <= myt2.d
          having count(distinct d) <= 3);
ERROR 42X39: Subquery is only allowed to return a single column.

Before the commit for this issue the above query ran without error. But now, with the latest trunk, the query fails because Derby thinks the subquery is returning more than one column, which is not true. The relevant code is in SubqueryNode.java:

        /* The parser does not enforce the fact that a subquery can only return
         * a single column, so we must check here.
         */
        if (resultColumns.size() != 1)
        {
            throw StandardException.newException(SQLState.LANG_NON_SINGLE_COLUMN_SUBQUERY);
        }

I think the call to "size()" here should be replaced with a call to "visibleSize()". If you agree that this is the correct fix then I can go ahead and commit this change to trunk as a "followup" patch for this issue.

[Manish Khettry]

hmmm, thanks for catching all of these. I should have thought about them and yes, it lis the correct fix.

[A B]

Actually, it turns out that just switching to "visibleSize()" does *not* solve the problem. I made that change to a clean codeline and the error still occurs. When I printed out the values of "size()" and "visibleSize()", they *both* showed "2".

I don't know if it's relevant, but if you add a group by clause to the query, it runs fine:

select distinct vc, x from t1 as myt1
      where d <= (select max(myt1.d) from t1 as myt2
          where myt1.vc = myt2.vc and myt1.d <= myt2.d
          group by d
          having count(distinct d) <= 3);

Manish, would you be willing to look into this a bit more?

[Manish Khettry]

OK. Do you think it makes sense to mark this bug as resolved and another to track this issue. Either way is fine.

[A B]

> Do you think it makes sense to mark this bug as resolved and another to track this issue. Either way is fine.

I agree, either way is fine; I'll let you decide :) If you create a new issue, please mark it as a "regression" so that we can make sure it gets fixed before the next release. Otherwise it might slip through the cracks...

Thank you very much for your time with this...

[Manish Khettry]

query with a having clause but without a group by has a generated aggregaegt added on. I made visibleSize() less restrictive-- i.e. it takes into accounts all generated columns not just those added in GroupByList#bindGroupByColumns.

Another follow up item for me to look at is the different mechanism used for order by columns which are added to the select list. This is completely different from how group by does things.

I ran junit-all and derbylang.

[Bryan Pendleton]

> Another follow up item for me to look at is the different mechanism used
> for order by columns which are added to the select list. This is completely
> different from how group by does things.

Hi Manish,

I did some work on the ORDER BY mechanism in DERBY-1861, but that work
is not yet complete. I'd be happy if you were to take a look at that as part of
your investigations into column list processing for DERBY-681.

[A B]

Thank you for the follow-up patch, Manish. I verified that "followup.patch.txt" solves the problem I reported above, so I committed the patch after adding a corresponding test case to lang/subquery.sql (always good to have a regression test case when possible):

    URL: http://svn.apache.org/viewvc?view=rev&rev=518687

I assume that the numGeneratedColumnsForGroupBy() method, which is now no longer in use, will be removed as part of your work for DERBY-2442?

[Manish Khettry]

This commit by Army, http://svn.apache.org/viewvc?view=rev&rev=518687, resolves issue.

[A B]

Looks like the fix for this issue caused a sorting regression as described in DERBY-3231. Manish, are you available to look into that a little more?

[A B]

Linking to DERBY-3303, as the problem described in that issue seems to occur as a result of the commit for this one.

[Donatas Ciuksys]

DERBY-3303 can also be observed on derby 10.3.1.4, so the bug might have crept in earlier.

[Kathey Marsden]

DERBY-4230 is a regression related to this issue. If the links are correct, this is our 12th regression related to this issue. The reports have trickled in over the past two years and have had a wide variety of symptoms.

I wonder if anyone has any ideas for a testing plan that we might be able to use to pop any additional issues related to this change?

[Rick Hillegas]

Hi Kathey,

I don't have any smart ideas, short of borrowing a query generator from some other project, generating a lot of queries (not just ones involving aggregates and GROUP BY), and comparing the results between Derby and some other database. Eliminating the parser's rewriting of the AST was a significant change to Derby's SQL interpreter. 8 years of code had piled up on top of the assumption that the AST was rewritten in the parser. The assumptions weren't linked to one another in any systematic way and I don't know of any systematic way to track them all down. So far, we have seen bugs related to GROUP BY, sort order, subqueries, unions, and column aliasing. The bugs are peppered all across the language.

[Kathey Marsden]

Well I drilled down on those linked issues and 12 was too harsh. Some of the linked issues were actually fixed by this change.

The idea of borrowing a query generator from some other project and comparing results with another db sounds like an interesting one for a new angle on general testing as well.

It looks like DERBY-4230 is an issue of size() being used where visibleSize() was appropriate. I seem to recall this being an issue with at least one other issue. It might be interesting to look at the other size() calls and make sure they shouldn't be visibleSize() instead now that the rewrite is gone.

A strange thing about Eclipse is when I search for references to QueryTreeNodeVector.size(), I am getting back all references to a size() method regardless of the class, which is a quite a lot of references.