db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Shreyas Kaushik <Shreyas.Kaus...@Sun.COM>
Subject Re: [PATCH] Derby-156
Date Thu, 02 Jun 2005 06:57:12 GMT
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* <Shreyas.Kaushik@sun.com 
> <mailto:Shreyas.Kaushik@sun.com>> 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() } )
>     >       <FROM> 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 = <WHERE> whereClause = whereClause(whereToken) ]
>     >       {
>     >               fromTable =  (FromTable) nodeFactory.getNode(
>     >                                                                      
>     C_NodeTypes.FROM_VTI,
>     >                                                                      
>     javaToSQLNode.getJavaValueNode(),
>     >-                                                                      (String)
>     null,
>     >+                                                                      correlationName,
>
>     >                                                                      
>     null,
>     >                                                                      
>     (Properties) null,
>     >                                                                      
>     getContextManager());
>     >@@ -2600,6 +2611,15 @@
>     >       }
>     > |
>     >       <FROM> 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 = <WHERE>
>     >@@ -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 <http://e3.name> = e2.mgrname
>     group by dno having
>     >                   e2.dno in (select dno from db2test.emp  e1
>     >                   where e1.name <http://e1.name> = e.mgrname and
>     e1.mgrname = 'JOHN')))));
>     >-ERROR 42X01: Syntax error: Encountered "e" at line 1, column 25.
>     >+ERROR 42X04: Column 'E5.NAME <http://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 <http://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 <http://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 <http://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 <http://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
>     >
>     >
>
>

Mime
View raw message