db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Army <qoz...@sbcglobal.net>
Subject Re: paging Satheesh
Date Tue, 09 Aug 2005 03:39:07 GMT
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


Mime
View raw message