db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "A B (JIRA)" <j...@apache.org>
Subject [jira] Commented: (DERBY-1861) Column ordering ASSERT when combining column references and expressions in same ORDER BY
Date Mon, 08 Jan 2007 18:05:27 GMT

    [ 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
> ----------------------------------------------------------------------------------------
>                 Key: DERBY-1861
>                 URL: https://issues.apache.org/jira/browse/DERBY-1861
>             Project: Derby
>          Issue Type: Bug
>          Components: SQL
>    Affects Versions:
>            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


View raw message