db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dag H. Wanvik (JIRA)" <j...@apache.org>
Subject [jira] Issue Comment Edited: (DERBY-4087) Clean up debug printing of the abstract syntax trees after parsing, binding and optimization
Date Sat, 08 Aug 2009 00:06:14 GMT

    [ https://issues.apache.org/jira/browse/DERBY-4087?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12740760#action_12740760
] 

Dag H. Wanvik edited comment on DERBY-4087 at 8/7/09 5:04 PM:
--------------------------------------------------------------

I have found some further issues regarding toString, treePrint and
printSubNodes. The comments only apply to subclasses of 
QueryTreeNode.

- Some class variables' value are printed with a leading "<label>: "
  in front of the value, others do not have a label, the value is
  printed alone. Usually, the label is the same as the class
  variable's name. Shouldn't this always be the case for uniformity?

- Some class variable labels are printed even if the values is not,
  in cases where the variables value is null, e.g. "preJoinFL: " in
  SelectNode. I suggest not printing in such cases to save space.

- Some toString methods call super.toString *before* formatting the
  class's own member variables, others do it the other way around. Is
  there a rationale for this I wonder? Or is it just accidental? It
  seems the established pattern is that super be called *after*, except
  for DDL operations, which always call super *first*, so I suggest
  adjusting toString to those two patterns throughout.
 
  Non-DDL classes that call super.toString first:
       CoalesceFunctionNode
       CurrentDatetimeOperatorNode
       SpecialFunctionNode
       ExtractOperatorNode

- Some usages of toString terminate with a newline, most do not. I
  think they should either end with a newline (DDL) or newline + call
  to super.toString (DML). At the root (qua anchor),
  QueryTreeNode.toString just prints "", so effectively for DML, the
  last thing printed will be the newline also.

- I think all nodes should call super.toString. This could be because
  the author "knew" they are right under QueryTreeNode, and so has
  chosen to optimize the call away (QueryTreeNode.toString prints the
  empty string), or it could be an omission. I think it would be good
  to be consistent here, too.

  Example of nodes that do not call super.toString at all:
       GenerationClauseNode
       OrderByColumn
       TableName

  TableName is possibly an allowable exception, I think, because its
  toString will return a string even in insane builds, so I not sure
  it can tolerate a terminating newline, so I intend to not touch
  this.

- Some nodes call toString of underlying nodes in their own toString,
  but such subnodes should, according to the contract defined in
  QueryTreeNode, be handled by PrintSubNodes/TreePrint:

       DistinctNode (formats childResult)
       GroupByNode (formats childResult)
       OrderByNode (formats childResult)
       OrderByColumn (formats expression)

  I suggest bringing such instances into line.

- Vectors are handled specially. Vectors with tree elements are
  subclasses of QueryTreeNodeVector, which contains a generic toString
  method which calls toString of the elements of the vector and
  concatenates them. I suggest changing this to be printSubNodes and
  use series of calls to treePrint instead to comply with the general
  pattern. Subclasses which have no additional members need not
  override toString either.

- Some subclasses of QueryTreeNodeVector needlessly(*) define their own
  printSubNodes method. I propose to remove those:

       ValueNodeList
       SubqueryList
       PredicateList
       FromList

  (*) this now inherit QueryTreeNodeVector's printSubNodes.

- One subclasses of QueryTreeNodeVector (TableElementList) rolls its
  own toString to print the elements. I suggest to remove this an
  dinstead rely on the general mechanism outlined above.

  Some classes neglect to print own member variables:
       ResultColumnList
       
- Some subclasses of QueryTreeNodeVector define their own treePrint
  method, e.g.
  
       ResultColumnList

  This seems to be a remnant from a time when ResultColumnList was not
  a subclass of QueryTreeNode, c.f this comment in

       /**
        * This class needs a treePrint method, even though it is not a
        * descendant of QueryTreeNode, because its members contain tree..

  so I intend to remove this. 

      was (Author: dagw):
    I have found some further issues regarding toString, treePrint and
printSubNodes. The comments only apply to subclasses of 
QueryTreeNode.

- Some class variables' value are printed with a leading "<label>: "
  in front of the value, others do not have a label, the value is
  printed alone. Usually, the label is the same as the class
  variable's name. Shouldn't this always be the case for uniformity?

- Some class variable labels are printed even if the values is not,
  in cases where the variables value is null, e.g. "preJoinFL: " in
  SelectNode. I suggest not printing in such cases to save space.

- Some toString methods call super.toString *before* formatting the
  class's own member variables, others do it the other way around. Is
  there a rationale for this I wonder? Or is it just accidental? It
  seems the established pattern is that super be called *after*, except
  for DDL operations, which always call super *first*, so I suggest
  adjusting toString to those two patterns throughout.
 
  Non-DDL classes that call super.toString first:
       CoalesceFunctionNode
       CurrentDatetimeOperatorNode
       SpecialFunctionNode
       ExtractOperatorNode

- Some usages of toString terminate with a newline, most do not. I
  think they should either end with a newline (DDL) or newline + call
  to super.toString (DML). At the root (qua anchor),
  QueryTreeNode.toString just prints "", so effectively for DML, the
  last thing printed will be the newline also.

- I think all nodes should call super.toString. This could be because
  the author "knew" they are right under QueryTreeNode, and so has
  chosen to optimize the call away (QueryTreeNode.toString prints the
  empty string), or it could be an omission. I think it would be good
  to be consistent here, too.

  Example of nodes that do not call super.toString at all:
       GenerationClauseNode
       OrderByColumn
       TableName

  TableName is possibly an allowable exception, I think, because its
  toString will return a string even in insane builds, so I not sure
  it can tolerate a terminating newline, so I intend to not touch
  this.

- Some nodes call toString of underlying nodes in their own toString,
  but such subnodes should, according to the contract defined in
  QueryTreeNode, be handled by PrintSubNodes/TreePrint:

       DistinctNode (formats childResult)
       GroupByNode (formats childResult)
       OrderByNode (formats childResult)
       OrderByColumn (formats expression)

  I suggest bringing such instances into line.

- Vectors are handled specially. Vectors with tree elements are
  subclasses of QueryTreeNodeVector, which contains a generic toString
  method which calls toString of the elements of the vector and
  concatenates them. I suggest changing this to be printSubNodes and
  use series of calls to treePrint instead to comply with the general
  pattern. Subclasses which have no additional members need not
  override toString either.

- Some subclasses of QueryTreeNodeVector needlessly(*) define their own
  printSubNodes method. I propose to remove those:

       ValueNodeList
       SubqueryList
       PredicateList
       FromList

  (*) this now inherit QueryTreeNodeVector's printSubNodes.

- One subclasses of QueryTreeNodeVector (TableElementList) rolls its
  own toString to print the elements. I suggest to remove this an
  dinstead rely on the general mechanism outlined above.

  Some subclasses of QueryTreeNodeVector do *not* define their own
  printSubNodes, e.g:

       ResultColumnList
       
- Some subclasses of QueryTreeNodeVector define their own treePrint
  method, e.g.
  
       ResultColumnList

  This seems to be a remnant from a time when ResultColumnList was not
  a subclass of QueryTreeNode, c.f this comment in

       /**
        * This class needs a treePrint method, even though it is not a
        * descendant of QueryTreeNode, because its members contain tree..

  so I intend to remove this. 
  
> Clean up debug printing of the abstract syntax trees after parsing, binding and optimization

> ---------------------------------------------------------------------------------------------
>
>                 Key: DERBY-4087
>                 URL: https://issues.apache.org/jira/browse/DERBY-4087
>             Project: Derby
>          Issue Type: Improvement
>          Components: Miscellaneous, SQL
>            Reporter: Dag H. Wanvik
>            Assignee: Dag H. Wanvik
>            Priority: Trivial
>
> Currently, the printing is often inconsistent:
> - some subtrees not printed
> - wrong indentation due to missing newlines, or lacking level increments
> - redundant printing of subtrees (AST is really a DAG, would be nice to print only once
and then refer back to show aliasing)
> - some items printed twice due to inconsistent usage of pattern

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message