[
https://issues.apache.org/jira/browse/DERBY-1861?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12463085
]
A B commented on DERBY-1861:
----------------------------
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?
> Column ordering ASSERT when combining column references and expressions in
> same ORDER BY
> ----------------------------------------------------------------------------------------
>
> Key: DERBY-1861
> URL: https://issues.apache.org/jira/browse/DERBY-1861
> Project: Derby
> Issue Type: Bug
> Components: SQL
> Affects Versions: 10.3.0.0
> Reporter: Bryan Pendleton
> Assigned To: Bryan Pendleton
> Priority: Minor
> Attachments: adjustOffsets_v1.diff, dataStructureNotes.html,
> proposedPatchNotes.html
>
>
> 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.
--
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
https://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
