[jira] [Comment Edited] (DERBY-673) Get rid of the NodeFactory

["Dag H. Wanvik (JIRA)" <ji...@apache.org>]

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

Dag H. Wanvik edited comment on DERBY-673 at 6/17/13 11:20 AM:
---------------------------------------------------------------

Uploading derby-673-1 which removes the node factory and does some further cleanup in the compiler.
 
a) Replaced the old init methods in "*Node.java" classes with constructors. Some logical node types have different "C_NodeType" values in their nodeType field after construction but still share the same node class.  I have not attempted to increase the number of node classes to match logical == physical node classes this once. Actually one class was removed because it unused: "SQLBooleanConstantNode". "IsNode" is also currently unused but there is a JIRA to make use of it (DERBY-5973), so I left it in place.

   Boxed argument types were replaced by primitive types except in a few cases where instanceof was used on them to detect type overloading; this could be gotten rid of by adding more constructors.

   Since the constructor arguments are now strongly typed, a great many casts were removed in the process and readability is improved a lot.

   In some cases the old init procedures did computations before calling "super.init". Since the call to the corresponding super constructor needs to be the first code in a constructor, I sometimes had to introduce new private static methods to compute the correct arguments to send on to the super class constructor, e.g. "getTypeId" in "UserTypeConstantNode". I think there is some redundancy here that could be removed in a follow-up patch.

   All the non-abstract node classes (still) set their corresponding "C_NodeType" value; but in many (most?) cases the field is no longer needed. This could be improved by removing them altogether and introduced class constants where needed to differentiate between logical node type mapped to one class. This is already done halfheartedly to some extent, e.g. enum "Sign" in "IsNullNode".

   The old "tools/jar/DBMSnodes.properties" file could be removed altogether since the node classes are now added automatically due to dependencies that the compiler can see (no longer constructed by reflection).

   The old nodeFactory method "doJoinOrderOptimization" was moved to the OptimizerFactory now that the NodeFactory has gone.

b) Added @Override tags to methods that override existing methods (not those that merely implement an interface)

c) Removed unused imports and sorted import statements for ease of future maintenance by IDEs

d) Renamed variables that shadowed class members

e) Replaced usage of StringBuffer with StringBuilder

f) Restricted public visibility to package private for all classes, methods and members in compile/impl unless they needed more visibility according to actual usage.

g) Made List arguments to node classes use generics in those cases it was missing.

e) Renamed some node types to make the the type mirror the node class correctly (there were only very few exceptions to that rule), e.g.  LIKE_OPERATOR_NODE -> LIKE_ESCAPE_OPERATOR_NODE since the class is called LikeEscapeOperatorNode.

f) Reduced scope of some variables: initialized to null values never used long before actual usage in code. By moving the declaration closed to usage, the unnecessary initialization could often be removed.

g) Fixed some spelling errors in comments.

h) Renamed some SQL-related constants (StoredFormatIds, TypeId) from "longint" " to "bigint" to reflect Derby SQL syntax.

Overall the patch removes ca 5K bytes in the insane derby.jar file, and ca 5000 source lines.

All regressions passed, but I'll be running more tests on more platforms since it is a big change.

Reviewers: you need to do "svn remove" of the five files that went away before attempting to build Derby, cf. the status file.

Things to look out for: missed opportunities to remove casts. The compiler doesn't help me detect superfluous ones ;-)

I realize this is a big patch, and if anybody thinks I should break it up, or drop parts of it, I am willing to consider it. I didn't experience much in the way of errors during the conversion, though, so my confidence in the patch is pretty good.  I did the changes incrementally over some 75 change/test cycles.


                
      was (Author: dagw):
    Uploading derby-673-1 which removes the node factory and does some further cleanup in the compiler.
 
a) Replaced the old init methods in "*Node.java" classes with constructors. Some logical node types have different "C_NodeType" values in their nodeType field after construction but still share the same node class.  I have not attempted to increase the number of node classes to match logical == physical node classes this once. Actually one class was removed because it unused: "SQLBooleanConstantNode". "IsNode" is also currently unused but there is a JIRA to make use of it (DERBY-5973), so I left it in place.

   Boxed argument types were replaced by primitive types except in a few cases where instanceof was used on them to detect type overloading; this could be gotten rid of by adding more constructors.

   Since the constructor arguments are now strongly typed, a great many casts were removed in the process and readability is improved a lot.

   In some cases the old init procedures did computations before calling "super.init". Since the call to the corresponding super constructor needs to be the first code in a constructor, I sometimes had to introduce new privat static methods to compute the correct arguments to send on to the super class constructor, e.g. "getTypeId" in "UserTypeConstantNode". I think there is some redundancy here that could be removed in a follow-up patch.

   All the non-abstract node classes (still) set their corresponding "C_NodeType" value; but in many (most?) cases the field is no longer needed. This could be improved by removing them altogether and introduced class constants where needed to differentate between logical node type mapped to one class. This is already done half heartedly to some extent, e.g. enum "Sign" in "IsNullNode".

   The old "tools/jar/DBMSnodes.properties" file could be removed altogether since the node classes are now added automatically due to dependencies that the compiler can see (no longer constructed by reflection).

   The old nodeFactory method "doJoinOrderOptimization" was moved to the OptimizerFactory now that the NodeFactory has gone.

b) Added @Override tags to methods that override existing methods (not those that merely implement an interface)

c) Removed unused imports and sorted import statements for ease of future maintenance by IDEs

d) Renamed variables that shadowed class members

e) Replaced usage of StringBuffer with StringBuilder

f) Restricted public visibility to package private for all classes, methods and members in compile/impl unless they needed more visibility according to actual usage.

g) Made List arguments to node classes use generics in those cases it was missing.

e) Renamed some node types to make the the type mirror the node class correctly (there were only very few exceptions to that rule), e.g.  LIKE_OPERATOR_NODE -> LIKE_ESCAPE_OPERATOR_NODE since the class is called LikeEscapeOperatorNode.

f) Reduced scope of some variables: initialized to null values never used long before actual usage in code. By moving the declaration closed to usage, the unnecessary initialization could often be removed.

g) Fixed some spelling errors in comments.

h) Renamed some SQL-related constants (StoredFormatIds, TypeId) from "longint" " to "bigint" to reflect Derby SQL syntax.

Overall the patch removes ca 5K bytes in the insane derby.jar file, and ca 5000 source lines.

All regressions passed, but I'll be running more tests on more platforms since it is a big change.

Reviewers: you need to do "svn remove" of the five files that went away before attempting to build Derby, cf. the status file.

Things to look out for: missed opportunities to remove casts. The compiler doesn't help be detect superfluos ones ;-)

I realize this is a big patch, and if anybody thinks I should break it up, or drop parts of it, I am willing to consider it. I didn't experience much in the way of errors during the conversion, though, so my confidence in the patch is pretty good.  I did the changes incrementally over some 75 change/test cycles.


                  
> Get rid of the NodeFactory
> --------------------------
>
>                 Key: DERBY-673
>                 URL: https://issues.apache.org/jira/browse/DERBY-673
>             Project: Derby
>          Issue Type: Improvement
>          Components: SQL
>            Reporter: Rick Hillegas
>            Assignee: Dag H. Wanvik
>         Attachments: derby-673-1.diff.gz, derby-673-1.status, nodefactory-31.status, nodefactory-31.zip
>
>
> This piece of code once had a purpose in life. It was one of the double-joints which allowed cloudscape to ship with and without compiler support for the synchronization language. Synchronization has been removed. If we want to plug in optional language components, I think there are better ways to do this.
> The NodeFactory turned into a big, sprawling piece of code. At some point this code was slimmed down by telescoping all of its factory methods into a couple unwieldly, weakly-typed overloads backed by cumbersome logic in the actual node constructors. I would like to reintroduce strongly typed node constructors which the parser can call directly. This will make node generation easier to read and less brittle and it will get rid of the now useless NodeFactory class.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira