Mailing-List: contact derby-dev-help@db.apache.org; run by ezmlm
Precedence: bulk
Reply-To: <derby-dev@db.apache.org>
Message-ID: <337764662.1236205916236.JavaMail.jira@brutus>
Date: Wed, 4 Mar 2009 14:31:56 -0800 (PST)
From: "Bryan Pendleton (JIRA)" <jira@apache.org>
To: derby-dev@db.apache.org
Subject: [jira] Commented: (DERBY-2487) Enhance Derby with EXPLAIN
 Functionality
In-Reply-To: <11016305.1174903532140.JavaMail.jira@brutus>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


    [ 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.