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] [Updated] (DERBY-673) Get rid of the NodeFactory
Date Mon, 17 Jun 2013 10:59:20 GMT

     [ https://issues.apache.org/jira/browse/DERBY-673?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

Dag H. Wanvik updated DERBY-673:
--------------------------------

    Attachment: derby-673-1.status
                derby-673-1.diff.gz

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

Mime
View raw message