Rick Hillegas wrote:
Hi Satheesh,
Do you think you could find some spare cycles to review this
enhancement and either commit the fix or suggest some improvments? I
would really appreciate it.
Well I'm neither Satheesh nor Shreyas ;), but I did take a quick look at the
patch and here are some minor things I noticed.
1) The diff for FromBaseTable.java is just the addition of a blank line; I think
that can be removed from the patch...
2) I don't think we need to create a new test file to test this patch; it seems
like the test cases that you put in bug171.sql could easily be included in the
existing tests "update.sql" and "delete.sql". Generally speaking, it's less
work to add test cases to existing (related) test files, and doing so has the
added benefits of a) leaving the test directory less cluttered, and b) requiring
fewer changes to the suite properties files (ex. your changes to
lang/copyfiles.ant and suites/derbylang.runall wouldn't be necessary). Of
course, the downside to this is that your test cases for DERBY-171 would then be
separated into two files--but that doesn't seem like a major thing to me...*shrug*
3) The comment that you added to DERBY-171 when you posted the patch said the
following:
"I am now passing the outer fromList context down the subquery binding stack.
This makes it possible to bind correlated references in those subqueries and
fixes a cluster of other bugs."
Can you elaborate on the "cluster of other bugs" that you mention here? If you
are thinking of any other bugs in particular that are fixed with this patch,
it'd be good to enumerate the bugs explicitly so that people (users and
developers alike) who encounter them later can search Jira, find this issue, and
see that their problem has been resolved as of svn revision <...>. A list of
the other bugs that this patch fixes, along with test cases to show that the
bugs existed and are fixed with this patch, would be extremely helpful. Maybe
that's what your changes to refActions.sql are doing? (see type "d" changes
below). If so, it'd be good to mention those specifics in the Jira issue so
people know where to look for the relevant test cases.
4) From what I can tell, the changes to refActions1.out fall into four
categories:
a -- update/delete statements that used to throw syntax errors because of
DERBY-171 were left unchanged but now run without error. Ex. see lines
7914-7923 of the patched refActions1.out file.
b -- update/delete statements that used to throw syntax errors because of
DERBY-171 were _rewritten_ and now run without error. Ex. see lines 5999-6005
of the patched refActions1.out file.
c -- update/delete statements that used to throw syntax errors still throw
syntax errors, but with different table names in the message. Ex. see lines
2777-2786 of the patched refActions1.out file.
d -- select statements that used to fail with syntax errors now succeed,
presumbaly because they have been fixed as part of the "cluster of other errors"
that you mentioned when you posted the patch (that's just my guess). Ex. see
lines 2935-2947 of the patched refActions1.out file.
That said, I have the following two questions:
* For the type "b" changes, I'm having problems seeing why the queries had to be
rewritten. Since, so far as I can tell, the error that used to be thrown was a
direct result of DERBY-171, it seems like running the queries exactly as-is with
your patch should have led to a successful query. But that said, I'm guessing
that wasn't the case, or you wouldn't have rewritten them ;) So can you say why
the queries had to be rewritten? Was it because they were leading to diffs like
those found in the type "c" changes? If they were throwing other errors, is it
possible that the errors they were throwing were intentional (ex. to show some
other feature/syntax that Derby didn't support)? I'm not saying that the
queries shouldn't have been rewritten, I'm just curious as to _why_ they were
rewritten...
* For the type "c" changes, I'm just wondering if you intentionally left these
diffs in, or was the intent to remove the errors altogether by rewriting the
queries, as was done with the type "b" changes, in which case these just slipped
through?
Those are the comments I had on the patch. As I said, they're pretty minor and
for a couple of them (esp. the last ones), the relevant changes might well be
perfectly okay the way they are--I'm just trying to get things straight in my
own head...
Hope that's more helpful than it is confusing/frustrating...
Army
