[PATCH] Derby-127

[Jack Klebanoff <kl...@sbcglobal.net>]

I have attached a patch that fixes Jira bug Derby-127 
(http://issues.apache.org/jira/browse/DERBY-127). The problem is with 
select statements that use a correlation name in the select list, a 
group by clause, and an order by clause that refers to a column by its 
database name instead of its correlation name. e.g.
  select c1 as x from t where ... group by x order by c1
Derby throws an exception with SQLState 42x04 complaining that it cannot 
resolve "c1".

The underlying problem is that the Derby parser transforms the select 
into a query tree for the following statement:
  select * from (select c1 as x from t where ...) order by c1
The code in class OrderByColumn did not take this into account. I 
changed methods pullUpOrderByColumn and bindOrderByColumn to handle this 
case specially. pullUpOrderByColumn adds the sort key to the 
ResultColumnList if it cannot find it there. It is called before binding 
and before select list wildcards ("*") are expanded. I changed it to 
pull the sort key into the ResultColumnList of the subselect generated 
to handle GROUP BY. I also changed it to remember where it was added. 
This simplifies the bindOrderByColumn, which is called after 
pullUpOrderByColumn.

I also fixed the handling of table names in class OrderByColumn. It 
treated them as strings, which does not work when the schema or table 
name is a quoted string containing a period. I changed OrderByColumn to 
use class TableName to represent a table name. The 
ResultColumnList.getOrderByColumn methods where changed accordingly

Jack Klebanoff.

Re: [PATCH] Derby-127

[Army <qo...@sbcglobal.net>]

Jack Klebanoff wrote:
> I agree with Army's suggestion for an extra test. Should I wait for my 
> original patch to be committed and submit the new test in a new patch, 
> or should I revise and re-submit my patch?

Not sure if that question was directed at me or not.  If it was, then my answer 
is "Ummm...Either way seems fine to me."

> I suggest committing my current patch first.

Okay, works for me.  Committers?
Army

Re: [PATCH] Derby-127

[Satheesh Bandaram <sa...@Sourcery.Org>]

I have committed this patch. If you would like to submit test updates
later, that would be fine.

Satheesh

Jack Klebanoff wrote:

> Army wrote:
>
>> Jack Klebanoff wrote:
>>
>>> I have attached a patch that fixes Jira bug Derby-127
>>> (http://issues.apache.org/jira/browse/DERBY-127).
>>
>>
>>
>> I reviewed this patch, applied it to a clean codeline without
>> problem, and ran the orderby.sql test that was included.  From what I
>> can tell, everything looks good here.
>>
>> My only minor comment is that it might be nice to add a case to the
>> orderby.sql test to make sure things work if _multiple_ columns are
>> provided in an order by clause.  Ex.
>>
>> ij> select c1 as x, c2 as y from bug2769 group by bug2769.c1,
>> bug2769.c2 order by c1, c2;
>>
>> ij> select c1 as x, c2 as y from bug2769 group by bug2769.c1,
>> bug2769.c2 order by c1, y;
>>
>> I tried these and they both work fine--no problems there.  But it was
>> something I was wondering while I was reviewing, so it might be nice
>> to include it in the test...*shrug*
>>
>> In any event, the patch gets my +1,
>> Army
>>
>>
> I agree with Army's suggestion for an extra test. Should I wait for my
> original patch to be committed and submit the new test in a new patch,
> or should I revise and re-submit my patch? Since Army has shown that
> the patch does handle his test case I suggest committing my current
> patch first. This will get things moving. Adding a few cases to an
> existing test results in a small, easily reviewed patch. If I revise
> the submitted patch it will be more difficult to review the submission.
>
> Jack Klebanoff
>
>
>

Re: [PATCH] Derby-127

[Jack Klebanoff <kl...@sbcglobal.net>]

Army wrote:

> Jack Klebanoff wrote:
>
>> I have attached a patch that fixes Jira bug Derby-127 
>> (http://issues.apache.org/jira/browse/DERBY-127).
>
>
> I reviewed this patch, applied it to a clean codeline without problem, 
> and ran the orderby.sql test that was included.  From what I can tell, 
> everything looks good here.
>
> My only minor comment is that it might be nice to add a case to the 
> orderby.sql test to make sure things work if _multiple_ columns are 
> provided in an order by clause.  Ex.
>
> ij> select c1 as x, c2 as y from bug2769 group by bug2769.c1, 
> bug2769.c2 order by c1, c2;
>
> ij> select c1 as x, c2 as y from bug2769 group by bug2769.c1, 
> bug2769.c2 order by c1, y;
>
> I tried these and they both work fine--no problems there.  But it was 
> something I was wondering while I was reviewing, so it might be nice 
> to include it in the test...*shrug*
>
> In any event, the patch gets my +1,
> Army
>
>
I agree with Army's suggestion for an extra test. Should I wait for my 
original patch to be committed and submit the new test in a new patch, 
or should I revise and re-submit my patch? Since Army has shown that the 
patch does handle his test case I suggest committing my current patch 
first. This will get things moving. Adding a few cases to an existing 
test results in a small, easily reviewed patch. If I revise the 
submitted patch it will be more difficult to review the submission.

Jack Klebanoff

Re: [PATCH] Derby-127

[Army <qo...@sbcglobal.net>]

Jack Klebanoff wrote:
> I have attached a patch that fixes Jira bug Derby-127 
> (http://issues.apache.org/jira/browse/DERBY-127).

I reviewed this patch, applied it to a clean codeline without problem, and ran 
the orderby.sql test that was included.  From what I can tell, everything looks 
good here.

My only minor comment is that it might be nice to add a case to the orderby.sql 
test to make sure things work if _multiple_ columns are provided in an order by 
clause.  Ex.

ij> select c1 as x, c2 as y from bug2769 group by bug2769.c1, bug2769.c2 order 
by c1, c2;

ij> select c1 as x, c2 as y from bug2769 group by bug2769.c1, bug2769.c2 order 
by c1, y;

I tried these and they both work fine--no problems there.  But it was something 
I was wondering while I was reviewing, so it might be nice to include it in the 
test...*shrug*

In any event, the patch gets my +1,
Army