drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ben-Zvi <...@git.apache.org>
Subject [GitHub] drill pull request #716: DRILL-5116: Enable generated code debugging in each...
Date Thu, 05 Jan 2017 03:48:15 GMT
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");
        ......   
      }   
    ```



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message