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 ;

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 indeed 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.
 
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.
 
You sure are becoming correlation name expert :)
Mamta
 
On 6/1/05, Shreyas Kaushik <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 = 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
>
>