You are viewing a plain text version of this content. The canonical link for it is here.
Posted to derby-dev@db.apache.org by Rick Hillegas <Ri...@Sun.COM> on 2005/08/08 22:26:51 UTC
paging Satheesh
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.
Thanks,
-Rick
Rick Hillegas wrote:
> I have attached a fix to bug 171 to its JIRA page. Bug 171 is an
> enhancement request for beefing up UPDATE and DELETE statements with
> the ANSI clauses for correlation names. I would appreciate a review.
>
> If these changes commit, please note that I have also attached a brief
> description of related changes that we will need to make to the
> Reference Manual.
>
> Thanks,
> -Rick
>
Re: paging Satheesh
Posted by Shreyas Kaushik <Sh...@Sun.COM>.
Hi Rick,
The changes look fine to me. I guess this fixes Derby-156 as well,
right ?
thanks
Shreyas
Satheesh Bandaram wrote:
> Hi Rick,
>
> I will take a look at the fix. *Shreyas*, since you worked on
> DERBY-156 and have a patch for that problem, can you review this patch
> too? We want to make sure this addresses DERBY-156 too.
>
> Satheesh
>
> 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.
>>
>> Thanks,
>> -Rick
>>
>> Rick Hillegas wrote:
>>
>>> I have attached a fix to bug 171 to its JIRA page. Bug 171 is an
>>> enhancement request for beefing up UPDATE and DELETE statements with
>>> the ANSI clauses for correlation names. I would appreciate a review.
>>>
>>> If these changes commit, please note that I have also attached a
>>> brief description of related changes that we will need to make to
>>> the Reference Manual.
>>>
>>> Thanks,
>>> -Rick
>>>
>>
>>
>>
>>
Re: paging Satheesh
Posted by Shreyas Kaushik <Sh...@Sun.COM>.
+1 from me. I had my qestion answered.
thanks
Shreyas
David Van Couvering wrote:
> Hi, Rick. Once this gets approval from Army and Shreyas I'll go ahead
> and check it in.
>
> David
>
> Rick Hillegas wrote:
>
>> I have attached a new rev of the fix to bug 171. This rev addresses
>> Army's concerns.
>>
>> Cheers,
>> -Rick
>>
>> Rick Hillegas wrote:
>>
>>> Thanks to Kathey, Shreyas, and Army for evaluating this bugfix.
>>>
>>> 1) I believe that Kathey's reservations are addressed by Suresh's
>>> response: This bugfix does not change any on-disk structures. It is
>>> OK for views to become invalid during downgrade.
>>>
>>> 2) I have added another comment to bug171, which I hope addresses
>>> Army's questions. I will purge from the submission the vacuously
>>> changed FromBaseTable. I will move my regression tests into the
>>> existing update.sql and delete.sql tests.
>>>
>>> 3) To answer Shreyas' question: Yes, this fixes bug 156 also.
>>>
>>> Cheers,
>>> -Rick
>>>
>>
>>
Re: paging Satheesh
Posted by David Van Couvering <Da...@Sun.COM>.
OK, I'm running build and derbyall and will commit if all looks good...
David
Army wrote:
> David Van Couvering wrote:
>
>> Hi, Rick. Once this gets approval from Army and Shreyas I'll go
>> ahead and check it in.
>>
>> David
>>
>> Rick Hillegas wrote:
>>
>>> I have attached a new rev of the fix to bug 171. This rev addresses
>>> Army's concerns.
>>>
>>> Cheers,
>>> -Rick
>>
>
> The new patch addresses the concerns I brought up earlier, and the
> comments in DERBY-171 now explain the changes to "refActions1" in a
> good amount of detail. I ran the modified "update.sql" and
> "delete.sql" tests locally with the patch applied and they both
> passed. Since I haven't noticed any other issues with new patch, I
> give it my +1.
>
> Thanks to Rick for answering my questions and addressing my comments,
> Army
>
Re: paging Satheesh
Posted by Army <qo...@sbcglobal.net>.
David Van Couvering wrote:
> Hi, Rick. Once this gets approval from Army and Shreyas I'll go ahead
> and check it in.
>
> David
>
> Rick Hillegas wrote:
>
>> I have attached a new rev of the fix to bug 171. This rev addresses
>> Army's concerns.
>>
>> Cheers,
>> -Rick
The new patch addresses the concerns I brought up earlier, and the comments in
DERBY-171 now explain the changes to "refActions1" in a good amount of detail.
I ran the modified "update.sql" and "delete.sql" tests locally with the patch
applied and they both passed. Since I haven't noticed any other issues with new
patch, I give it my +1.
Thanks to Rick for answering my questions and addressing my comments,
Army
Re: paging Satheesh
Posted by David Van Couvering <Da...@Sun.COM>.
Hi, Rick. Once this gets approval from Army and Shreyas I'll go ahead
and check it in.
David
Rick Hillegas wrote:
> I have attached a new rev of the fix to bug 171. This rev addresses
> Army's concerns.
>
> Cheers,
> -Rick
>
> Rick Hillegas wrote:
>
>> Thanks to Kathey, Shreyas, and Army for evaluating this bugfix.
>>
>> 1) I believe that Kathey's reservations are addressed by Suresh's
>> response: This bugfix does not change any on-disk structures. It is
>> OK for views to become invalid during downgrade.
>>
>> 2) I have added another comment to bug171, which I hope addresses
>> Army's questions. I will purge from the submission the vacuously
>> changed FromBaseTable. I will move my regression tests into the
>> existing update.sql and delete.sql tests.
>>
>> 3) To answer Shreyas' question: Yes, this fixes bug 156 also.
>>
>> Cheers,
>> -Rick
>>
>
>
Re: paging Satheesh
Posted by Rick Hillegas <Ri...@Sun.COM>.
I have attached a new rev of the fix to bug 171. This rev addresses
Army's concerns.
Cheers,
-Rick
Rick Hillegas wrote:
> Thanks to Kathey, Shreyas, and Army for evaluating this bugfix.
>
> 1) I believe that Kathey's reservations are addressed by Suresh's
> response: This bugfix does not change any on-disk structures. It is OK
> for views to become invalid during downgrade.
>
> 2) I have added another comment to bug171, which I hope addresses
> Army's questions. I will purge from the submission the vacuously
> changed FromBaseTable. I will move my regression tests into the
> existing update.sql and delete.sql tests.
>
> 3) To answer Shreyas' question: Yes, this fixes bug 156 also.
>
> Cheers,
> -Rick
>
Re: paging Satheesh
Posted by Rick Hillegas <Ri...@Sun.COM>.
Thanks to Kathey, Shreyas, and Army for evaluating this bugfix.
1) I believe that Kathey's reservations are addressed by Suresh's
response: This bugfix does not change any on-disk structures. It is OK
for views to become invalid during downgrade.
2) I have added another comment to bug171, which I hope addresses Army's
questions. I will purge from the submission the vacuously changed
FromBaseTable. I will move my regression tests into the existing
update.sql and delete.sql tests.
3) To answer Shreyas' question: Yes, this fixes bug 156 also.
Cheers,
-Rick
Re: paging Satheesh
Posted by Suresh Thalamati <su...@gmail.com>.
Kathey Marsden wrote:
> Army wrote:
> [snip , lots of good comments ]
>
>
>>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.
>
>
>
> With a very brief look at the patch, I did not see any code to disallow
> the new syntax with soft upgrade. Since users need to be able to return
> to a previous version with soft upgrade I think it is important that
> they not be able to go to 10.2 and then create a view that won't work
> when they return to 10.1
> Dan put upgrade tests under
> java/testing/org/apache/derbyTesting/upgradeTests. Unfortunately, and
> perhaps dangerously, the upgrade tests are not automated as part of our
> suite yet because we'll need to check in old versions of derby and
> manage the class path issues.
>
> Kathey
>
>
>
>
>
>
>
I think simple syntax changes does not need softupgrade check if the
new syntax usage does not create any new on-disk structures.
dan's comments on the related issue:
http://www.mail-archive.com/derby-dev@db.apache.org/msg05886.html
Thanks
-suresht
Re: paging Satheesh
Posted by Kathey Marsden <km...@sbcglobal.net>.
Army wrote:
[snip , lots of good comments ]
> 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.
With a very brief look at the patch, I did not see any code to disallow
the new syntax with soft upgrade. Since users need to be able to return
to a previous version with soft upgrade I think it is important that
they not be able to go to 10.2 and then create a view that won't work
when they return to 10.1
Dan put upgrade tests under
java/testing/org/apache/derbyTesting/upgradeTests. Unfortunately, and
perhaps dangerously, the upgrade tests are not automated as part of our
suite yet because we'll need to check in old versions of derby and
manage the class path issues.
Kathey
Re: paging Satheesh
Posted by Army <qo...@sbcglobal.net>.
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
Re: paging Satheesh
Posted by Rick Hillegas <Ri...@Sun.COM>.
Thanks, Satheesh and Shreyas.
-Rick
Satheesh Bandaram wrote:
> Hi Rick,
>
> I will take a look at the fix. *Shreyas*, since you worked on
> DERBY-156 and have a patch for that problem, can you review this patch
> too? We want to make sure this addresses DERBY-156 too.
>
> Satheesh
>
> 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.
>>
>> Thanks,
>> -Rick
>>
>> Rick Hillegas wrote:
>>
>>> I have attached a fix to bug 171 to its JIRA page. Bug 171 is an
>>> enhancement request for beefing up UPDATE and DELETE statements with
>>> the ANSI clauses for correlation names. I would appreciate a review.
>>>
>>> If these changes commit, please note that I have also attached a
>>> brief description of related changes that we will need to make to
>>> the Reference Manual.
>>>
>>> Thanks,
>>> -Rick
>>>
>>
>>
>>
>>