Return-Path: Delivered-To: apmail-db-derby-dev-archive@www.apache.org Received: (qmail 74378 invoked from network); 2 Jun 2005 07:01:02 -0000 Received: from hermes.apache.org (HELO mail.apache.org) (209.237.227.199) by minotaur.apache.org with SMTP; 2 Jun 2005 07:01:02 -0000 Received: (qmail 71269 invoked by uid 500); 2 Jun 2005 07:01:00 -0000 Delivered-To: apmail-db-derby-dev-archive@db.apache.org Received: (qmail 71244 invoked by uid 500); 2 Jun 2005 07:00:59 -0000 Mailing-List: contact derby-dev-help@db.apache.org; run by ezmlm Precedence: bulk list-help: list-unsubscribe: List-Post: List-Id: Reply-To: "Derby Development" Delivered-To: mailing list derby-dev@db.apache.org Received: (qmail 71220 invoked by uid 99); 2 Jun 2005 07:00:59 -0000 X-ASF-Spam-Status: No, hits=0.0 required=10.0 tests= X-Spam-Check-By: apache.org Received-SPF: pass (hermes.apache.org: local policy) Received: from brmea-mail-3.Sun.COM (HELO brmea-mail-3.sun.com) (192.18.98.34) by apache.org (qpsmtpd/0.28) with ESMTP; Thu, 02 Jun 2005 00:00:58 -0700 Received: from phys-biff-1 ([129.158.227.36]) by brmea-mail-3.sun.com (8.12.10/8.12.9) with ESMTP id j5270gn0028479 for ; Thu, 2 Jun 2005 01:00:43 -0600 (MDT) Received: from conversion-daemon.biff-mail1.india.sun.com by biff-mail1.india.sun.com (iPlanet Messaging Server 5.2 HotFix 1.24 (built Dec 19 2003)) id <0IHG00K0133O18@biff-mail1.india.sun.com> (original mail from Shreyas.Kaushik@Sun.COM) for derby-dev@db.apache.org; Thu, 02 Jun 2005 12:30:42 +0530 (IST) Received: from [129.158.229.246] (lilly.India.Sun.COM [129.158.229.246]) by biff-mail1.india.sun.com (iPlanet Messaging Server 5.2 HotFix 1.24 (built Dec 19 2003)) with ESMTP id <0IHG006LQ3H52F@biff-mail1.india.sun.com> for derby-dev@db.apache.org; Thu, 02 Jun 2005 12:30:42 +0530 (IST) Date: Thu, 02 Jun 2005 12:27:12 +0530 From: Shreyas Kaushik Subject: Re: [PATCH] Derby-156 In-reply-to: To: Derby Development Message-id: <429EADC8.6000604@Sun.com> MIME-version: 1.0 Content-type: text/plain; charset=ISO-8859-1; format=flowed Content-transfer-encoding: 7BIT X-Accept-Language: en-us, en User-Agent: Mozilla Thunderbird 1.0 (X11/20041208) References: <42632E66.3020403@sun.com> <4263441B.9020908@sun.com> <4265CF33.4070700@Sun.com> <4265D734.6010506@debrunners.com> <4265D9BC.4040500@Sun.com> <426720A8.7080602@Sun.com> <42937EC4.1010508@Sourcery.Org> <42941C16.2010605@Sun.com> <429D9BE5.8040602@Sun.com> X-Virus-Checked: Checked X-Spam-Rating: minotaur.apache.org 1.6.2 0/1000/N Hi Mamta, Thanks for your comments. Please see inline. ~Shreyas Mamta Satoor wrote: > Hi Shreyas, > > I reviewed your code briefly and it looks good. Couple comments > > 1)In DeleteNode.java, you check if correlationName is not null (copied > your code below for reference) >>+ if(targetTable instanceof FromBaseTable) { >>+ String correlationName; >>+ correlationName = > ((FromBaseTable)targetTable).correlationName; >>+ if(correlationName != null) >>+ fbt.correlationName = correlationName; >>+ } > > Is that check necessary? Is it possible to replace the code above with > following? >>+ if(targetTable instanceof FromBaseTable) >>+ fbt.correlationName = > ((FromBaseTable)targetTable).correlationName ; Yes the check is necessary, if a normal delete is called without the correlation name it will be null , in "fbt" also it will be null. By checking I am avoiding an assignment. > > Taking one step further, is it possible to also skip the check for > instanceof FromBaseTable and just have following assignment? >>+ fbt.correlationName = > targetTable.correlationName; > I have only briefly looked at the changes and it is possible that we do in Yes this check is necessary as the FromTable node in the method deleteBody() in SQLParser.java is an instance of a FromBaseTable. The code already there checks whether targetTable is an instance of FromVTI maintaining consistency this should also check for the instance of FromBaseTable. > deed need the checks you have but I thought I would check in any case. > > 2)Rather than a completely new sql test file, I think you could use > existing lang/derived.sql which tests correlation names as well. Ok I can do this. > > 3)You might need to generate a new patch(because it has been quite > sometime since you submitted the patch), rerun the tests and submit > the patch based on latest codeline. Sure. > > You sure are becoming correlation name expert :) I am looking into Derby-84 as well ;-) > Mamta > > On 6/1/05, *Shreyas Kaushik* > wrote: > > Did anyone get a chance to review this? > > thanks > Shreyas > > Shreyas Kaushik wrote: > > > Attached is the latest patch. Ran derbylang suite successfully > without > > any failures. > > > > ~ Shreyas > > > > Satheesh Bandaram wrote: > > > >> Is this patch ready for review? Still any pending issues here? > >> > >> Satheesh > >> > >> Shreyas Kaushik wrote: > >> > >> > >> > >>> Hi Dan, > >>> > >>> If you are ok with this, I can add the comments to the patch > and send > >>> it out on the alias. > >>> > >>> thanks > >>> Shreyas > >>> > >>> Shreyas Kaushik wrote: > >>> > >>> > >>> > >>>> Ok. Thanks for your suggestions I will do it. > >>>> > >>>> The idea behibd adding that peice of code is: > >>>> > >>>> The *delete from* did not have support for handling correlation > >>>> names. So when we are creating a fresh ResultColumnList > >>>> and the FromBaseTable we need to pass the correlation name to > this > >>>> newly created FromBaseTable object from the target table where it > >>>> will stored. > >>>> > >>>> thanks > >>>> Shreyas > >>>> > >>>> Daniel John Debrunner wrote: > >>>> > >>>> > >>>> > >>>>> Shreyas Kaushik wrote: > >>>>> > >>>>> > >>>>> > >>>>> > >>>>>> Did anyone gfo through this? > >>>>>> > >>>>>> > >>>>> > >>>>> > >>>>> I looked briefly. Do you have an explanation of your changes? > >>>>> Adding comments to the code you added in DeleteNode, > explaining what > >>>>> exactly that new block is doing would be really useful. > >>>>> > >>>>> I would say the chance of a fix being committed quickly > increases > >>>>> significantly if it is well explained and well commented. > The code > >>>>> changes really need to stand alone, ie. be understandable > through > >>>>> comments, as anyone looking through the code in the future > will not > >>>>> have > >>>>> a link to any e-mail discussion to help them along. Though a > >>>>> summary of > >>>>> the changes with the contribution can be useful. Such a > summary is > >>>>> also > >>>>> a useful commit comment for the svn history of changes to the > >>>>> codeline. > >>>>> > >>>>> Writing code comments is also very helpful to the writer of > the code, > >>>>> helps to cement the mental ideas into code. A good practise > is to > >>>>> write > >>>>> the comments first and then the code. > >>>>> > >>>>> Dan. > >>>>> > >>>>> > >>>>> > >>>>> > >>>>> > >>>>>>>>> Index: > >>>>>>>>> > java/engine/org/apache/derby/impl/sql/compile/DeleteNode.java > >>>>>>>>> > =================================================================== > >>>>>>>>> > >>>>>>>>> --- > >>>>>>>>> > java/engine/org/apache/derby/impl/sql/compile/DeleteNode.java > >>>>>>>>> (revision 161449) > >>>>>>>>> +++ > >>>>>>>>> > java/engine/org/apache/derby/impl/sql/compile/DeleteNode.java > >>>>>>>>> (working copy) > >>>>>>>>> @@ -241,6 +241,13 @@ > >>>>>>>>> resultColumnList = new > ResultColumnList(); > >>>>>>>>> > >>>>>>>>> FromBaseTable fbt = > >>>>>>>>> getResultColumnList(resultColumnList); > >>>>>>>>> + > >>>>>>>>> + if(targetTable instanceof > >>>>>>>>> FromBaseTable) { > >>>>>>>>> + String correlationName; > >>>>>>>>> + correlationName = > >>>>>>>>> ((FromBaseTable)targetTable).correlationName; > >>>>>>>>> + if(correlationName != null) > >>>>>>>>> + fbt.correlationName = > >>>>>>>>> correlationName; > >>>>>>>>> + } > >>>>>>>>> > >>>>>>>>> readColsBitSet = > getReadMap(dataDictionary, > >>>>>>>>> > >>>>>>>>> targetTableDescriptor); > >>>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>> > >>>>> > >>>>> > >>>>> > >>>>> > >>>>> > >>>>> > >>>>> > >>>> > >>> > >>> > >> > >> > >> > >> > >------------------------------------------------------------------------ > > > >Index: java/engine/org/apache/derby/impl/sql/compile/DeleteNode.java > >=================================================================== > >--- > java/engine/org/apache/derby/impl/sql/compile/DeleteNode.java (revision > 178358) > >+++ > java/engine/org/apache/derby/impl/sql/compile/DeleteNode.java (working > copy) > >@@ -241,6 +241,18 @@ > > resultColumnList = new ResultColumnList(); > > > > FromBaseTable fbt = > getResultColumnList(resultColumnList); > >+ > >+ /* When we are creating a fresh > ResultColumnList > >+ * and the FromBaseTable we need to pass > the correlation name > >+ * to this newly created FromBaseTable > object from the target table > >+ * where it will be stored. > >+ */ > >+ if(targetTable instanceof FromBaseTable) { > >+ String correlationName; > >+ correlationName = > ((FromBaseTable)targetTable).correlationName; > >+ if(correlationName != null) > >+ fbt.correlationName = > correlationName; > >+ } > > > > readColsBitSet = getReadMap(dataDictionary, > > > targetTableDescriptor); > >Index: java/engine/org/apache/derby/impl/sql/compile/sqlgrammar.jj > >=================================================================== > >--- > java/engine/org/apache/derby/impl/sql/compile/sqlgrammar.jj (revision > 178358) > >+++ > java/engine/org/apache/derby/impl/sql/compile/sqlgrammar.jj (working > copy) > >@@ -2582,16 +2582,27 @@ > > QueryTreeNode retval; > > Properties targetProperties = null; > > Token whereToken = null; > >+ String correlationName = null; > >+ Object []objArr = null; > > } > > { > > LOOKAHEAD( { fromNewInvocationFollows() } ) > > javaToSQLNode = newInvocation() > >+ /** > >+ * Get the correlation name if there is any in the SQL > statement > >+ * Extract the correlation name and pass it on to the > FromTable Node > >+ * for binding > >+ */ > >+ { > >+ objArr = optionalTableClauses(); > >+ correlationName = > (String)objArr[OPTIONAL_TABLE_CLAUSES_CORRELATION_NAME]; > >+ } > > [ whereToken = whereClause = whereClause(whereToken) ] > > { > > fromTable = (FromTable) nodeFactory.getNode( > > > C_NodeTypes.FROM_VTI, > > > javaToSQLNode.getJavaValueNode(), > >- (String) > null, > >+ correlationName, > > > > null, > > > (Properties) null, > > > getContextManager()); > >@@ -2600,6 +2611,15 @@ > > } > > | > > tableName = qualifiedName(Limits.MAX_IDENTIFIER_LENGTH) > >+ /** > >+ * Get the correlation name if there is any in the SQL > statement > >+ * Extract the correlation name and pass it on to the > FromTable Node > >+ * for binding > >+ */ > >+ { > >+ objArr = optionalTableClauses(); > >+ correlationName = > (String)objArr[OPTIONAL_TABLE_CLAUSES_CORRELATION_NAME]; > >+ } > > [targetProperties = propertyList() ] > > [ > > whereToken = > >@@ -2631,7 +2651,7 @@ > > fromTable = (FromTable) nodeFactory.getNode( > > > C_NodeTypes.FROM_BASE_TABLE, > > > tableName, > >- null, > >+ correlationName, > > > ReuseFactory.getInteger( > > > FromBaseTable.DELETE), > > > null, > >Index: > java/testing/org/apache/derbyTesting/functionTests/tests/lang/corrDelete.sql > >=================================================================== > >--- > java/testing/org/apache/derbyTesting/functionTests/tests/lang/corrDelete.sql > (revision 0) > >+++ > java/testing/org/apache/derbyTesting/functionTests/tests/lang/corrDelete.sql > (revision 0) > >@@ -0,0 +1,24 @@ > >+-- This tests the delete functionality with correlation name > >+ > >+create table corrDelete(ival int, cval varchar(10)); > >+insert into corrDelete values(1,'test1'); > >+insert into corrDelete values(2,'test2'); > >+insert into corrDelete values(3,'test3'); > >+insert into corrDelete values(4,'test4'); > >+insert into corrDelete values(5,'test5'); > >+insert into corrDelete values(6,'test6'); > >+ > >+select * from corrDelete; > >+ > >+delete from corrDelete d where ival=3; > >+ > >+select * from corrDelete; > >+ > >+delete from corrDelete as d where d.ival=5; > >+ > >+select * from corrDelete; > >+ > >+delete from corrDelete d; > >+ > >+select * from corrDelete; > >+ > >Index: > java/testing/org/apache/derbyTesting/functionTests/tests/lang/refActions1.sql > > >=================================================================== > >--- > java/testing/org/apache/derbyTesting/functionTests/tests/lang/refActions1.sql (revision > 178358) > >+++ > java/testing/org/apache/derbyTesting/functionTests/tests/lang/refActions1.sql (working > copy) > >@@ -719,6 +719,14 @@ > > select * from db2test.emp13 order by dno, name, mgrname; > > select * from db2test.emp14 order by dno, name, mgrname; > > select * from db2test.emp15 order by dno, name, mgrname; > >+delete from db2test.dept d where > >+ dno in (select dno from db2test.emp e where > >+ e.dno = d.dno and e.dno in (select dno from db2test.emp2 e2 where > >+ e2.dno = e.dno and e2.dno in (select dno from db2test.emp3 e3 where > >+ e3.dno = e2.dno and e3.dno in (select dno from db2test.emp4 e4 > where > >+ e4.dno = e3.dno and e4.dno in (select dno from db2test.emp5 e5 > where > >+ e5.dno = e4.dno and e5.dno in (select dno from db2test.emp6 e6 > where > >+ e6.dno = e5.dno and e6.dno in ('K55', 'K52'))))))); > > -- "END OF TESTUNIT: 11"; > > > > > >@@ -2306,14 +2314,6 @@ > > e5.dno = e4.dno and e5.dno in (select dno from db2test.emp6 e6 > where > > e6.dno = e5.dno and e6.dno in ('K55', 'K52'))))))) > > order by 2, 3; > >-delete from db2test.dept d where > >- dno in (select dno from db2test.emp e where > >- e.dno = d.dno and e.dno in (select dno from db2test.emp2 e2 where > >- e2.dno = e.dno and e2.dno in (select dno from db2test.emp3 e3 where > >- e3.dno = e2.dno and e3.dno in (select dno from db2test.emp4 e4 > where > >- e4.dno = e3.dno and e4.dno in (select dno from db2test.emp5 e5 > where > >- e5.dno = e4.dno and e5.dno in (select dno from db2test.emp6 e6 > where > >- e6.dno = e5.dno and e6.dno in ('K55', 'K52'))))))); > > select * from db2test.dept order by dno, dname; > > select * from db2test.emp order by dno, name, mgrname; > > select * from db2test.secondemp order by dno, name, mgrname; > >Index: > java/testing/org/apache/derbyTesting/functionTests/tests/lang/copyfiles.ant > > >=================================================================== > >--- > java/testing/org/apache/derbyTesting/functionTests/tests/lang/copyfiles.ant (revision > 178358) > >+++ > java/testing/org/apache/derbyTesting/functionTests/tests/lang/copyfiles.ant (working > copy) > >@@ -209,3 +209,4 @@ > > wisconsin_app.properties > > wisconsin_derby.properties > > wisconsin_sed.properties > >+corrDelete.sql > >Index: > java/testing/org/apache/derbyTesting/functionTests/master/corrDelete.out > > >=================================================================== > >--- > java/testing/org/apache/derbyTesting/functionTests/master/corrDelete.out > (revision 0) > >+++ > java/testing/org/apache/derbyTesting/functionTests/master/corrDelete.out > (revision 0) > >@@ -0,0 +1,49 @@ > >+ij> -- This tests the delete functionality with correlation name > >+create table corrDelete(ival int, cval varchar(10)); > >+0 rows inserted/updated/deleted > >+ij> insert into corrDelete values(1,'test1'); > >+1 row inserted/updated/deleted > >+ij> insert into corrDelete values(2,'test2'); > >+1 row inserted/updated/deleted > >+ij> insert into corrDelete values(3,'test3'); > >+1 row inserted/updated/deleted > >+ij> insert into corrDelete values(4,'test4'); > >+1 row inserted/updated/deleted > >+ij> insert into corrDelete values(5,'test5'); > >+1 row inserted/updated/deleted > >+ij> insert into corrDelete values(6,'test6'); > >+1 row inserted/updated/deleted > >+ij> select * from corrDelete; > >+IVAL |CVAL > >+---------------------- > >+1 |test1 > >+2 |test2 > >+3 |test3 > >+4 |test4 > >+5 |test5 > >+6 |test6 > >+ij> delete from corrDelete d where ival=3; > >+1 row inserted/updated/deleted > >+ij> select * from corrDelete; > >+IVAL |CVAL > >+---------------------- > >+1 |test1 > >+2 |test2 > >+4 |test4 > >+5 |test5 > >+6 |test6 > >+ij> delete from corrDelete as d where d.ival=5; > >+1 row inserted/updated/deleted > >+ij> select * from corrDelete; > >+IVAL |CVAL > >+---------------------- > >+1 |test1 > >+2 |test2 > >+4 |test4 > >+6 |test6 > >+ij> delete from corrDelete d; > >+4 rows inserted/updated/deleted > >+ij> select * from corrDelete; > >+IVAL |CVAL > >+---------------------- > >+ij> > >Index: > java/testing/org/apache/derbyTesting/functionTests/master/refActions1.out > > >=================================================================== > >--- > java/testing/org/apache/derbyTesting/functionTests/master/refActions1.out (revision > 178358) > >+++ > java/testing/org/apache/derbyTesting/functionTests/master/refActions1.out (working > copy) > >@@ -1733,6 +1733,15 @@ > > 5 |JOE2 |ASHOK |K51 > > 2 |JOHN |ASHOK |K51 > > 3 |ROBIN |ASHOK |K51 > >+ij> delete from db2test.dept d where > >+ dno in (select dno from db2test.emp e where > >+ e.dno = d.dno and e.dno in (select dno from db2test.emp2 e2 where > >+ e2.dno = e.dno and e2.dno in (select dno from db2test.emp3 e3 where > >+ e3.dno = e2.dno and e3.dno in (select dno from db2test.emp4 e4 > where > >+ e4.dno = e3.dno and e4.dno in (select dno from db2test.emp5 e5 > where > >+ e5.dno = e4.dno and e5.dno in (select dno from db2test.emp6 e6 > where > >+ e6.dno = e5.dno and e6.dno in ('K55', 'K52'))))))); > >+0 rows inserted/updated/deleted > > ij> -- "END OF TESTUNIT: 11"; > > -- > ************************************************************************* > > -- TESTUNIT : 12 > >@@ -5998,15 +6007,6 @@ > > -------------------------- > > 2 |K52|OFC > > 1 |K55|DB > >-ij> delete from db2test.dept d where > >- dno in (select dno from db2test.emp e where > >- e.dno = d.dno and e.dno in (select dno from db2test.emp2 e2 where > >- e2.dno = e.dno and e2.dno in (select dno from db2test.emp3 e3 where > >- e3.dno = e2.dno and e3.dno in (select dno from db2test.emp4 e4 > where > >- e4.dno = e3.dno and e4.dno in (select dno from db2test.emp5 e5 > where > >- e5.dno = e4.dno and e5.dno in (select dno from db2test.emp6 e6 > where > >- e6.dno = e5.dno and e6.dno in ('K55', 'K52'))))))); > >-ERROR 42X01: Syntax error: Encountered "d" at line 1, column 26. > > ij> select * from db2test.dept order by dno, dname; > > C0 |DNO|DNAME > > -------------------------- > >@@ -7585,7 +7585,7 @@ > > where e3.name = e2.mgrname > group by dno having > > e2.dno in (select dno from db2test.emp e1 > > where e1.name = e.mgrname and > e1.mgrname = 'JOHN'))))); > >-ERROR 42X01: Syntax error: Encountered "e" at line 1, column 25. > >+ERROR 42X04: Column 'E5.NAME ' is not in any > table in the FROM list or it appears within a join specification > and is outside the scope of the join specification or it appears > in a HAVING clause and is not in the GROUP BY list. If this is a > CREATE or ALTER TABLE statement then ' E5.NAME ' > is not a column in the target table. > > ij> select * from db2test.emp order by dno, name, mgrname; > > C0 |NAME |MGRNAME |DNO > > -------------------------------------- > >@@ -7753,7 +7753,7 @@ > > where e.name = e2.mgrname > group by dno having > > e2.dno in (select dno from db2test.emp e1 > > where e.mgrname = 'JOHN'))))); > >-ERROR 42X01: Syntax error: Encountered "e" at line 1, column 25. > >+ERROR 42X04: Column 'E.NAME ' is not in any table > in the FROM list or it appears within a join specification and is > outside the scope of the join specification or it appears in a > HAVING clause and is not in the GROUP BY list. If this is a > CREATE or ALTER TABLE statement then ' E.NAME ' is > not a column in the target table. > > ij> select * from db2test.emp order by dno, name, mgrname; > > C0 |NAME |MGRNAME |DNO > > -------------------------------------- > >Index: > java/testing/org/apache/derbyTesting/functionTests/suites/derbylang.runall > >=================================================================== > >--- > java/testing/org/apache/derbyTesting/functionTests/suites/derbylang.runall > (revision 178358) > >+++ > java/testing/org/apache/derbyTesting/functionTests/suites/derbylang.runall > (working copy) > >@@ -31,6 +31,7 @@ > > lang/connect.sql > > lang/consistencyChecker.sql > > lang/constantExpression.sql > >+lang/corrDelete.sql > > lang/currentSchema.sql > > lang/currentof.java > > lang/cursor.java > > > > > >