db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Army <qoz...@gmail.com>
Subject Re: [jira] Updated: (DERBY-939) NullPointerException at ResultSet.close() time for simple query using UNION and INTERSECT
Date Wed, 26 Jul 2006 23:39:30 GMT
Yip Ng (JIRA) wrote:
>      [ http://issues.apache.org/jira/browse/DERBY-939?page=all ]
> 
> Yip Ng updated DERBY-939:
> -------------------------
> 
>     Attachment: derby939trunkdiffpatch1.txt
>                 derby939trunkstatpatch1.txt
> 
> Here is the patch for DERBY-939.  

I have reviewed the patch and the content all looks good to me.  Thanks for 
picking this up and for going the extra mile to actually print out a useful 
query plan instead of just avoiding the NPE.

My only comments are very minor (and some might say annoying) ones, but I 
thought I'd make them anyways just to potentially make life easier for reviewers 
in the future (and to spare you the need to redo patches for picky people like 
me :).

Minor details:
--------------

1 - Some whitespace inconsistencies between new code and existing code.  For 
example:

SetOpResultSet.java:

@@ -99,12 +104,17 @@

          isOpen = true;
          leftSource.openCore();
          rightSource.openCore();
          rightInputRow = rightSource.getNextRowCore();
+		if (rightInputRow != null)
+		{
+			rowsSeenRight++;
+		}
+
  		numOpens++

While there is no agreed-upon whitespace format for Derby code, I think it's 
generally accepted that, until we have a standard, it's best to indent new code 
to match the code surrounding it.  There are several places in the patch where 
this kind of mixed indentation occurs.

For what it's worth, I usually do an MKS "cat" on a patch before submitting; if 
there is inconsistent indentation (esp. tabs vs. spaces) it's usually very 
obvious in the "cat" output.

 > cat derby939trunkdiffpatch1.txt | more

There also appear to be some inconsistencies within the new code itself, ex. in 
RealSetOpResultSetStatistics:

+	 *  @param   nextTime                     the time for next operation
+	 *  @param   closeTime                    the time for close operation
+	 *  @param   resultSetNumber              the result set number
+	 *  @param   rowsSeenLeft                 rows seen by left source input
+     *  @param   rowsSeenRight                rows seen by right source input
+     *  @param   rowsReturned                 rows returned
+     *  @param   optimizerEstimatedRowCount   optimizer estimated row count
+     *  @param   optimizerEstimatedCost       optimizer estimated cost
+     *  @param   leftResultSetStatistics      the left source input runtime 
statistics
+     *  @param   rightResultSetStatistics     the right source input runtime 
statistics
+	 *  @see     org.apache.derby.impl.sql.execute.SetOpResultSet
+	 */

2 - Lines longer than 80 chars

Again, Derby hasn't settled on any strict formatting rules, but I believe the 
general trend is to avoid having lines longer than 80 chars so that the code is 
easily readable by most editors.  I don't think this has ever kept a patch from 
being committed, but it is nice to respect this limit if it's possible (and so 
long as it doesn't make the code unreadable).  One notable exception to this is 
the Derby copyright/license header in the code files; those are usually left alone.

3 - New test file setOpPlan.sql

While perhaps less important now that Derby is (slowly) moving toward JUnit 
tests, I nonetheless think the preference is to avoid creating new .sql files 
where possible.  The reason is that the test harness currently has to re-create 
a database for each .sql file that it finds, which can slow the harness process 
down (I think someone somewhere once said that the time it takes to run derbyall 
is cut in half if we avoid creating new databases for every test).  Instead, 
it's generally better to add test cases to existing test files, if it makes 
sense to do so.

In this case there is a test called "intersect.sql" that does some basic testing 
of the INTERSECT and EXCEPT operators, and there's a test "union.sql" for 
testing the UNION operator.  You could add your test cases to these two files if 
you wanted to avoid creating a new setOpPlans.sql test.  But that said, I can 
also see the logic in keeping the new tests together--especially since it is 
often the case that tests which print query plans need an additional property 
(derby.optimizer.noTimeout=true) in order to ensure they pass on all 
machines/platforms.  So in the end I don't have any feelings one way or the 
other; I'll let you make the decision.

> Here is the release note if it makes it in 10.2:

Hmm...does this issue require a release note?  My understanding is that this is 
a "normal" bug fix and that, as such, it will be included in the release notes 
by default as "DERBY-939".  Typically that's good enough; I think additional 
release notes are only needed if an application that currently performs some 
operation successfully could end up seeing a difference in results and/or 
behavior as a result of the change.  In that case, assuming the new 
results/behavior are intentional and correct, a release note is needed in order 
to give existing users a warning that the behavior has changed and could affect 
their applications.

At least, that's my take on it.  I hope others will chime in if I'm wrong here.

All of that said, the info you have in the release note is *great* information, 
so I'm glad you posted it to the issue.  Thanks for being so thorough in the 
write-up!

I verified that the new lang/setOpPlan.sql test passes with your changes and 
fails without them.  I've also verified that the repros mentioned in the 
description of this issue run without a problem when your changes are applied.

Thanks again for the patch.  Once the minor formatting issues are resolved (esp. 
the whitespace issues) I think this will be a solid patch ready for commit.

Army


Mime
View raw message