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
