[
https://issues.apache.org/jira/browse/DERBY-2487?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12678965#action_12678965
]
Bryan Pendleton commented on DERBY-2487:
----------------------------------------
Mamta, your observation about the handling of childResultSetStatistics in the
various
implementations of accept(XPLAINVisitor) is quite interesting.
This is a new accept() method added to most of the various ResultSetStatistics
sub-types
as part of this patch. In the original patch, the implementation of the
accept() method
was (mostly) written in a style of being careful about checking the
childResultSetStatistics
pointer before using it. For example, here's the implementation from
RealSortStatistics:
+ public void accept(XPLAINVisitor visitor) {
+ int noChildren = 0;
+ if(this.childResultSetStatistics!=null) noChildren++;
+
+ //inform the visitor
+ visitor.setNumberOfChildren(noChildren);
+
+ // pre-order, depth-first traversal
+ // me first
+ visitor.visit(this);
+ // then my child
+ if(childResultSetStatistics!=null){
+ childResultSetStatistics.accept(visitor);
+ }
+ }
Note that the code checks to see if childResultSetStatistics is null before
using it.
This is the pattern in most of the new accept() methods, with the 3 exceptions
that you noted.
However, the more I look at this code, the more I think that those 3 exceptions
are actually
the desirable pattern, and most of the other accept() implementations are being
too cautious.
For example, I think that it *DOES* make sense for
RealDeleteResultSetStatistics.accept()
to have code which checks to see whether sourceResultSetStatistics is null or
not, because
sourceResultSetStatisics is an optional part of RealDeleteResultSetStatistics.
Sometimes it
is present, sometimes not. I'm not sure *WHY* the sourceResultSetStatistics is
optional here,
but it clearly is, and the code elsewhere in RealDeleteResultSetStatistics is
careful to check
it before dereferencing it.
On the other hand for RealUnionResultSetStatistics, for example, I think the
code is being
too cautious. The current code added by this patch for this class looks like
this:
+ public void accept(XPLAINVisitor visitor) {
+ int noChildren = 0;
+ if(this.leftResultSetStatistics!=null) noChildren++;
+ if(this.rightResultSetStatistics!=null) noChildren++;
+
+ //inform the visitor
+ visitor.setNumberOfChildren(noChildren);
+
+ // pre-order, depth-first traversal
+ // me first
+ visitor.visit(this);
+ // then visit first my left child
+ if(leftResultSetStatistics!=null){
+ leftResultSetStatistics.accept(visitor);
+ }
+ // and then my right child
+ if(rightResultSetStatistics!=null){
+ rightResultSetStatistics.accept(visitor);
+ }
+ }
But I think this is unnecessary. It is clear by reading elsewhere in
RealUnionResultSetStatistics that the leftResultSetStatistics and
rightResultSetStatistics are *not* optional, and must always be present,
so I think the new code would be simpler and clearer if it was instead:
+ public void accept(XPLAINVisitor visitor) {
+ //inform the visitor
+ visitor.setNumberOfChildren(2);
+
+ // pre-order, depth-first traversal
+ // me first
+ visitor.visit(this);
+ // then visit first my left child
+ leftResultSetStatistics.accept(visitor);
+ // and then my right child
+ rightResultSetStatistics.accept(visitor);
+ }
So rather than changing the 3 Statistics classes that you mentioned to add "!=
null"
checks for the childResultSetStatistics field, I'm tempted to instead go through
the other Statistics classes and to ensure that the new accept() method only
uses conditional logic for the child statistics objects if the children truly
are optional.
Does that makes sense?
> Enhance Derby with EXPLAIN Functionality
> ----------------------------------------
>
> Key: DERBY-2487
> URL: https://issues.apache.org/jira/browse/DERBY-2487
> Project: Derby
> Issue Type: New Feature
> Components: SQL
> Affects Versions: 10.2.2.0
> Reporter: Felix Beyer
> Assignee: Bryan Pendleton
> Priority: Minor
> Attachments: Derby physical XPLAIN schema.png,
> incorporateTrunkChanges.diff, removeSourceDepth.diff, RSProtocolNew.pdf,
> rts.xls, small logical xplain schema.pdf, startRegressionTest.diff,
> startRegressionTest.diff, startUpgradeTests.diff, updateRegressionTests.diff,
> updateRegressionTests.diff, usage.txt, xplain_patch_v1.txt, xplainClasses.pdf
>
>
> This enhancement extends Derby with EXPLAIN functions. Users want to have
> more feedback than they`re getting with the current RuntimeStatistics
> facility. This extension is based on the
> RuntimeStatistics/ResultSetStatistics functions / classes.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
