Army 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? 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 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
Jack Klebanoff
