Column ordering ASSERT when combining column references and expressions in same ORDER BY

• Key: DERBY-1861
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Minor
• Assignee: Bryan Pendleton
• Reporter: Bryan Pendleton
• Component/s: SQL
• Affects Version/s: 10.3.1.4
• Fix Version/s: 10.3.1.4

Description

An ORDER BY clause wihch combines both column references and expressions causes the
sort engine to throw an ASSERT failure in sane builds.

Here's a repro script:

-bash-2.05b$ java org.apache.derby.tools.ij
ij version 10.3
ij> connect 'jdbc:derby:brydb;create=true';
ij> create table t (a int, b int, c int, d int);
0 rows inserted/updated/deleted
ij> insert into t values (1, 2, 3, 4);
1 row inserted/updated/deleted
ij> select * from t order by a, b, c+2;
ERROR XJ001: Java exception: 'ASSERT FAILED column ordering error: org.apache.derby.shared.common.sanity.AssertFailure'.

As a first theory to check, I believe that when columns in the ORDER BY clause go through "pullup" processing,
they are generated into the select statement's ResultColumnList and then are later removed at bind time because
they are determined to duplicate the columns implicitly selected by the "*" column list. But the expression "c+2" is not
removed from the result list because it does not duplicate any existing column in the table. During this processing,
I think that the field "addedColumnOffset" in class OrderByColumn is not managed correctly and ends up generating
a bogus column position for the "c+2" column (it doesn't reflect that pulled-up columns "a" and "b" have disappeared
from the ResultColumnList), causing the sanity assertion at execution time.

I stumbled across this problem while writing regression tests for DERBY-147, but the problem occurs
with or without the DERBY-147 fix, so I decided to log it as a separate problem.

Comments

[Bryan Pendleton]

Found this bug while writing tests for DERBY-147. Both bugs involve
processing of columns in the ORDER BY clause, but are probably
no more related than that.

[Bryan Pendleton]

Attaching some background notes that will be helpful to reviewers (I hope).

[Bryan Pendleton]

Attached is adjustOffsets_v1.patch, a proposed patch, and proposedPatchNotes.html, which provides some additional notes for reviewers. This patch passes derbyall and suites.All.

[A B]

Hi Bryan,

Thank you for writing up the issue so clearly! It was nice that I could understand your changes just by reading your notes, even though I didn't have knowledge of order by "pull up" processing prior to this.

The write-up is great, and the code is clean, contained, and well-commented. I ran lang/orderby.sql after applying the patch and it ran cleanly. When I reverted your engine changes and re-ran the new test cases, they failed as expected.

I only have two questions: one from your notes, one for the patch.

1) Question on the notes:

> it also seemed like the call to pullUpOrderByColumns had been placed
> very carefully, as there is a big comment in CursorNode stating that
> this order of operations was deliberate, so I decided there was a
> good reason for it.

Out of simple curiosity, were you able to figure out why this order was specifically chosen? I read through the comments in CursorNode a couple of times but still couldn't quite understand the significance. Did you try re-ordering the code to see what happened? Were you able to come up with any cases where such reordering caused problems? As I said, this is just a question bred from curiosity--I'm not asking you to try it out (but if you already tried it, I'm wondering what the results were).

For the record, I have no problems with your decision to leave the code as it is for safety. I think it's better to do what you did--namely, fix the data structures so that they do what they were intended to do. As you said the changes are pretty small and they make sense, so that seems like a good choice.

2) Question on the patch:

As you mentioned when you posted the patch for for DERBY-147, there are two versions of the "getOrderByColumn()" method in ResultColumnList. For DERBY-147 it looks like you made the same changes to both of the methods, despite (or perhaps because of) your comment saying:

> I reverted to a smaller patch, which doesn't combine the nearly-
> redundant versions of getOrderByColumn, but which preserves the
> existing behavior in the case of table correlation names.

That said, I noticed that for this DERBY-1861 patch, the changes that you made to the two methods are different. For one method you added the new calls described in proposedPatchNotes.html; but for the other you replaced the old code with code to throw an assertion failure:

@@ -506,10 +525,10 @@
                 }
                 else if (index >= size - orderBySelect)
- {// remove the column due to pullup of orderby item
- removeElement(resultColumn);
- decOrderBySelect();
- break;
+ {
+ SanityManager.THROWASSERT(
+ "Unexpectedly found ORDER BY column '" +
+ columnName + "' pulled up at position " +index);
                 }

Can you explain (at a high level) the difference between the two methods and maybe indicate why the latter method was changed to throw an ASSERT failure? And would it make sense to add such an explanation to the code itself?

[Yip Ng]

Hi Bryan, I have reviewed your proposal patch. Great writeup on the pull-up processing for ORDER BY.
I was able to apply the patch cleanly and ran it successfully and fails without the fix with the newly added testcases. I am fine with the proposed solution. +1 on commit if regression passes.

[Bryan Pendleton]

Thanks much Yip and Army for the reviews and suggestions.

I'm intending to investigate the issues that Army noted, but I probably won't
get to it for a little while. Depending on what I find, I'll either come back with
a revised patch, or proceed with this patch.

[Bryan Pendleton]

I still haven't had any more time to look at this. I'm considering committing the patch as is, because I think it's solid and fixes the identified issue, and filing a follow-on issue to investigate consolidating (or at least better documenting) the two closely-similar versions of getOrderByColumn. Having bumped into that confusing bit of code twice now (in DERBY-147 and in this issue) I'd like to follow it up and improve it, but I'd also like to get this patch committed before it "spoils". If nobody objects, I'll proceed with this plan in the next week or so.

[Bryan Pendleton]

OK, I changed my mind again :) In the process of bringing the patch
up to date with the current head of trunk, I decided to try and add some
more comments and JavaDoc and try to address Army's comment about
the two similar, but not identical, getOrderByColumn routines in
ResultColumnList.

Attached is adjustOffsets_v2_moreJavaDoc.diff, which differs from the
original patch only by:
- it is up to date with the head of trunk
- I've added more JavaDoc comments to the two confusing routines
  in ResultColumnList, and
- I've renamed the two routines to getOrderByColumnToBind and
  findResultColumnForOrderBy to try to make their usage more clear
  (in addition to adding the JavaDoc)

Please let me know any comments or feedback, in particular whether
the modified routines are more clear now.

[A B]

Thank you very much for the second patch with additional javadoc, Bryan. The new comments do indeed make things clearer, as does the renaming of the two similar-but-not-quite-identical methods.

I vote +1 for committing the patch with this additional javadoc--especially since, as you said, "it's solid and fixes the identified issue".
Thanks for addressing my comments!

[Bryan Pendleton]

Thanks Army for the feedback and good suggestions. I committed
the v2 patch to subversion as revision 520038.

[A B]

Hi Bryan,

There's one thing that I missed when reviewing your changes--and I only just now noticed it by sheer accident.

In order to get some numbers for DERBY-47 I did an INSANE build and then tried to build jars. The building of the jars was successful, but I just happened to notice one unusal line in the output, namely:

filteractivator:
     [echo] creating derby.jar class list
     [java] SANITY >>> /org/apache/derby/impl/sql/compile/ResultColumnList.class
     [echo] creating new DBMS.properties file

Confused by this, I opened ResultColumnList.java and did a search for SanityManager--and it turns out that the patch for this issue adds a call to SanityManager.THROWASSERT that is *not* contained inside an "if (SanityManager.DEBUG)" block.

Apparently this isn't a big deal since all of the nightly tests have been running just fine--but for the sake of correctness, I think we need to wrap the THROWASSERT call inside a SanityManager.DEBUG check. It's a one-line change that should be easy enough to make (esp. since you're a committer... ;)

[Bryan Pendleton]

Thanks Army! I'll take care of it straightaway.

[Bryan Pendleton]

Committed a simple change to enclose the ThrowAssert in a Debug check,
and confirmed that the SANITY warning now disappears from an insane jar build.
Committed to the trunk as revision 520699.