[
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
