db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mamta Satoor <msat...@gmail.com>
Subject Re: [PATCH] Derby-156
Date Thu, 02 Jun 2005 06:29:34 GMT
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 <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