[
https://issues.apache.org/jira/browse/DRILL-5116?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15800202#comment-15800202
]
ASF GitHub Bot commented on DRILL-5116:
---------------------------------------
Github user Ben-Zvi commented on a diff in the pull request:
https://github.com/apache/drill/pull/716#discussion_r94706789
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/expr/ClassGenerator.java ---
@@ -374,6 +388,124 @@ public HoldingContainer declare(MajorType t, boolean
includeNewInstance) {
return this.workspaceVectors;
}
+ /**
+ * Prepare the generated class for use as a plain-old Java class
+ * (to be compiled by a compiler and directly loaded without a
+ * byte-code merge. Three additions are necessary:
+ * <ul>
+ * <li>The class must extend its template as we won't merge byte
+ * codes.</li>
+ * <li>A constructor is required to call the <tt>__DRILL_INIT__</tt>
+ * method. If this is a nested class, then the constructor must
+ * include parameters defined by the base class.</li>
+ * <li>For each nested class, create a method that creates an
+ * instance of that nested class using a well-defined name. This
+ * method overrides the base class method defined for this purpose.</li>
+ */
+
+ public void preparePlainJava() {
+
+ // If this generated class uses the "straight Java" technique
+ // (no byte code manipulation), then the class must extend the
+ // template so it plays by normal Java rules for finding the
+ // template methods via inheritance rather than via code injection.
+
+ Class<?> baseClass = sig.getSignatureClass();
+ clazz._extends(baseClass);
+
+ // Create a constuctor for the class: either a default one,
+ // or (for nested classes) one that passes along arguments to
+ // the super class constructor.
+
+ Constructor<?>[] ctors = baseClass.getConstructors();
+ for (Constructor<?> ctor : ctors) {
+ addCtor(ctor.getParameterTypes());
+ }
+
+ // Some classes have no declared constructor, but we need to generate
one
+ // anyway.
+
+ if ( ctors.length == 0 ) {
+ addCtor( new Class<?>[] {} );
+ }
+
+ // Repeat for inner classes.
+
+ for(ClassGenerator<T> child : innerClasses.values()) {
+ child.preparePlainJava();
+
+ // If there are inner classes, then we need to generate a "shim"
method
+ // to instantiate that class.
+ //
+ // protected TemplateClass.TemplateInnerClass newTemplateInnerClass(
args... ) {
+ // return new GeneratedClass.GeneratedInnerClass( args... );
+ // }
+ //
+ // The name is special, it is "new" + inner class name. The template
must
+ // provide a method of this name that creates the inner class
instance.
+
+ String innerClassName = child.clazz.name();
+ JMethod shim = clazz.method(JMod.PROTECTED,
child.sig.getSignatureClass(), "new" + innerClassName);
+ JInvocation childNew = JExpr._new(child.clazz);
+ Constructor<?>[] childCtors =
child.sig.getSignatureClass().getConstructors();
+ Class<?>[] params;
+ if (childCtors.length==0) {
+ params = new Class<?>[0];
+ } else {
+ params = childCtors[0].getParameterTypes();
+ }
+ for (int i = 1; i < params.length; i++) {
+ Class<?> p = params[i];
+ childNew.arg(shim.param(model._ref(p), "arg" + i));
+ }
+ shim.body()._return(childNew);
+ }
+ }
+
+ /**
+ * The code generator creates a method called __DRILL_INIT__ which takes
the
+ * place of the constructor when the code goes though the byte code
merge.
+ * For Plain-old Java, we call the method from a constructor created for
+ * that purpose. (Generated code, fortunately, never includes a
constructor,
+ * so we can create one.) Since the init block throws an exception (which
+ * should never occur), the generated constructor converts the checked
+ * exception into an unchecked one so as to not require changes to the
+ * various places that create instances of the generated classes.
+ *
+ * Example:<code><pre>
+ * public StreamingAggregatorGen1() {
+ * try {
+ * __DRILL_INIT__();
+ * } catch (SchemaChangeException e) {
+ * throw new UnsupportedOperationException(e);
+ * }
+ * }</pre></code>
+ *
+ * Note: in Java 8 we'd use the <tt>Parameter</tt> class defined in
Java's
+ * introspection package. But, Drill prefers Java 7 which only provides
+ * parameter types.
+ */
+
+ private void addCtor(Class<?>[] parameters) {
+ JMethod ctor = clazz.constructor(JMod.PUBLIC);
+ JInvocation superCall = null;
--- End diff --
A minor suggestion: Write all the code between lines 491-502 inclusive
inside a single if statement:
```
// if there are parameters, need to pass them to the super class
if ( parameters.length > 0 ) {
JInvocation superCall = JExpr.invoke("super");
......
}
```
> Enable generated code debugging in each Drill operator
> ------------------------------------------------------
>
> Key: DRILL-5116
> URL: https://issues.apache.org/jira/browse/DRILL-5116
> Project: Apache Drill
> Issue Type: Improvement
> Affects Versions: 1.9.0
> Reporter: Paul Rogers
> Assignee: Paul Rogers
> Priority: Minor
>
> DRILL-5052 adds the ability to debug generated code. Some of the code
> generated by Drill's operators has minor problems when compiled directly
> using the new technique. These issues are ignore by the byte-code-merge
> technique uses in production. This ticket asks to try the DRILL-5052 feature
> in each operator, clean up any minor problems, and ensure each operator
> generates code suitable for debugging. Use the new
> {{CodeGenerator.plainOldJavaCapable()}} method to mark each generated class
> as ready for "plain-old Java" code gen.
> The advantages of this feature are two:
> 1. Ability to step through the generated code to increase understanding of
> existing operators and to ease development of improvements to existing
> operators and of any new operators we choose to create.
> 2. Open the door to experimenting with how to improve performance of the
> generated code.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
