DRILL-1385, along with some cleanup

Cleaned up option handling. This includes using finals, making member variables
private whenever possible, and some formatting.
- fixed a bug in the string formatting for the double range validator
- OptionValidator, OptionValue, and their implementations now conspire not to
  allow the creation of malformed options because the OptionType has been added
  to validator calls to handle OptionValues that are created on demand.

Started with updated byte code rewrite from Jacques
Fixed several problems with scalar value replacement:
- use consistent ASM api version throughout
- stop using deprecated ASM methods (actually causes bugs)
  - visitMethodInsn()
- added a couple of missing super.visitEnd()s
- fixed a couple of minor FindBugs issues
- accounted for required stack size increases when replacing holders for
  longs and doubles
- added accounting for frame offsets to cope with long and double local
  variables and value holder members
- fixed a few minor bugs found with FindBugs
- stop using carrotlabs' hash map lget() method on shared constant data
- fixed an incorrect use of DUP2 on objectrefs when copying long or double
  holder members into locals
- fixed a problem with redundant POP instructions left behind after replacement
- fixed a problem with incorrect DUPs in multiple assignment statements
- fixed a problem with DUP_X1 replacement when handling constants in multiple
  assignment statements
- fixed a problem with non-replaced holder member post-decrements
- don't replace holders passed to static functions as "out" parameters
  (common with Accessors on repeated value vectors)
- increased the maximum required stack size when transferring holder members to
  locals
- changed the code generation block type mappings for constants for external
  sorts
- fixed problems handling constant and non-constant member variables in
  operator classes
  - in general, if a holder is assigned to or from an operator member variable,
    it can't be replaced (at least not until we replace those as well)
  - Use a derived ASM Analyzer (MethodAnalyzer) and Frame
    (AssignmentTrackingFrame) in order to establish relationships between
    assignments of holders through chains of local variables. This effectively
    back-propagates non-replaceability attributes so that if a holder variable
    that can't be replaced is assigned to from another holder variable, that
    second one cannot be replaced either, and so on through longer chains of
    assignments.
- code for dumping generated source code
- MergeAdapter dumps before and after results of scalar replacement
  (if it's on)
- fixed some problems in ReplacingBasicValue by replacing HashSet with
  IdentityHashMap
- made loggers private
- added a retry strategy for scalar replacement
  if a scalar replacement code rewriting fails, then this will try to
  regenerate the bytecode again without the scalar replacement.
- bytecode verification is always on now (required for the retry strategy)
- use system option to determine whether scalar replacement should be used
  - default option: if scalar replacement fails, retry without it
  - force replacement on or off
- unit tests for the retry strategy are based on a single known failure case,
  covered by DRILL-2326.
  - add tests TestConvertFunctions to test the three scalar replacement options
    for the failing test case (testVarCharReturnTripConvertLogical)
- made it possible to set a SYSTEM option as a java property in Drillbit
- added a command line argument to force scalar replacement to be on during
  testing in the rootmost pom.xml

In the course of this, added increased checking of intermediate stages of code
rewriting, as well as logging of classes that cause failures.
- work around a bug in ASM's CheckClassAdapter that doesn't allow for checking
  of inner classes
Added comments, tidied up formatting, and added "final" in a number of places.

Signed-off-by: vkorukanti <[email protected]>

common/src/main/java/org/apache/drill/common/config/DrillConfig.java

@@ -58,7 +58,7 @@ public final class DrillConfig extends NestedConfig{
   @VisibleForTesting
   public DrillConfig(Config config, boolean enableServer) {
     super(config);
-
+    logger.debug("Setting up config object.");
     mapper = new ObjectMapper();
 
     if (enableServer) {
@@ -80,7 +80,7 @@ public DrillConfig(Config config, boolean enableServer) {
 
     RuntimeMXBean bean = ManagementFactory.getRuntimeMXBean();
     this.startupArguments = ImmutableList.copyOf(bean.getInputArguments());
-
+    logger.debug("Config object initialized.");
   };
 
   public List<String> getStartupArguments() {

common/src/main/java/org/apache/drill/common/util/PathScanner.java

@@ -28,6 +28,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.TimeUnit;
 
 import org.apache.drill.common.config.CommonConstants;
 import org.reflections.Reflections;
@@ -37,6 +38,7 @@
 import org.reflections.util.ClasspathHelper;
 import org.reflections.util.ConfigurationBuilder;
 
+import com.google.common.base.Stopwatch;
 import com.google.common.collect.Sets;
 
 public class PathScanner {
@@ -73,20 +75,27 @@ private static Reflections getReflections() {
 
   public static <T> Class<?>[] scanForImplementationsArr(Class<T> baseClass, final List<String> scanPackages) {
     Collection<Class<? extends T>> imps = scanForImplementations(baseClass, scanPackages);
-    return imps.toArray(new Class<?>[imps.size()]);
+    Class<?>[] arr = imps.toArray(new Class<?>[imps.size()]);
+    return arr;
   }
 
   public static <T> Set<Class<? extends T>> scanForImplementations(Class<T> baseClass, final List<String> scanPackages) {
-    synchronized(SYNC) {
-      Set<Class<? extends T>> classes = getReflections().getSubTypesOf(baseClass);
-      for (Iterator<Class<? extends T>> i = classes.iterator(); i.hasNext();) {
-        Class<? extends T> c = i.next();
-        assert baseClass.isAssignableFrom(c);
-        if (Modifier.isAbstract(c.getModifiers())) {
-          i.remove();
+    Stopwatch w = new Stopwatch().start();
+    try {
+      synchronized(SYNC) {
+
+        Set<Class<? extends T>> classes = getReflections().getSubTypesOf(baseClass);
+        for (Iterator<Class<? extends T>> i = classes.iterator(); i.hasNext();) {
+          Class<? extends T> c = i.next();
+          assert baseClass.isAssignableFrom(c);
+          if (Modifier.isAbstract(c.getModifiers())) {
+            i.remove();
+          }
         }
+        return classes;
       }
-      return classes;
+    } finally{
+      logger.debug("Classpath scanning took {}ms", w.elapsed(TimeUnit.MILLISECONDS));
     }
   }
 

exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java

@@ -100,6 +100,8 @@ public interface ExecConstants {
   public static String PARQUET_NEW_RECORD_READER = "store.parquet.use_new_reader";
   public static OptionValidator PARQUET_RECORD_READER_IMPLEMENTATION_VALIDATOR = new BooleanValidator(PARQUET_NEW_RECORD_READER, false);
 
+  public static OptionValidator COMPILE_SCALAR_REPLACEMENT = new BooleanValidator("exec.compile.scalar_replacement", false);
+
   public static String JSON_ALL_TEXT_MODE = "store.json.all_text_mode";
   public static OptionValidator JSON_READER_ALL_TEXT_MODE_VALIDATOR = new BooleanValidator(JSON_ALL_TEXT_MODE, false);
 

exec/java-exec/src/main/java/org/apache/drill/exec/compile/AbstractClassCompiler.java

@@ -17,6 +17,8 @@
  */
 package org.apache.drill.exec.compile;
 
+import java.io.File;
+import java.io.FileWriter;
 import java.io.IOException;
 
 import org.apache.drill.common.util.DrillStringUtils;
@@ -35,6 +37,18 @@ public byte[][] getClassByteCode(ClassNames className, String sourceCode)
       throws CompileException, IOException, ClassNotFoundException, ClassTransformationException {
     if (getLogger().isDebugEnabled()) {
       getLogger().debug("Compiling (source size={}):\n{}", DrillStringUtils.readable(sourceCode.length()), prefixLineNumbers(sourceCode));
+
+/* uncomment this to get a dump of the generated source in /tmp
+      // This can be used to write out the generated operator classes for debugging purposes
+      // TODO: should these be put into a directory named with the query id and/or fragment id
+      final int lastSlash = className.slash.lastIndexOf('/');
+      final File dir = new File("/tmp", className.slash.substring(0, lastSlash));
+      dir.mkdirs();
+      final File file = new File(dir, className.slash.substring(lastSlash + 1) + ".java");
+      final FileWriter writer = new FileWriter(file);
+      writer.write(sourceCode);
+      writer.close();
+*/
     }
     return getByteCode(className, sourceCode);
   }

exec/java-exec/src/main/java/org/apache/drill/exec/compile/AsmUtil.java

@@ -0,0 +1,173 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.compile;
+
+import java.io.PrintWriter;
+import java.io.StringWriter;
+
+import org.objectweb.asm.ClassReader;
+import org.objectweb.asm.ClassWriter;
+import org.objectweb.asm.Opcodes;
+import org.objectweb.asm.tree.ClassNode;
+import org.objectweb.asm.util.TraceClassVisitor;
+import org.slf4j.Logger;
+
+/**
+ * Utilities commonly used with ASM.
+ *
+ * <p>There are several class verification utilities which use
+ * CheckClassAdapter (DrillCheckClassAdapter) to ensure classes are well-formed;
+ * these are packaged as boolean functions so that they can be used in assertions.
+ */
+public class AsmUtil {
+  // This class only contains static utilities.
+  private AsmUtil() {
+  }
+
+  /**
+   * Check to see if a class is well-formed.
+   *
+   * @param classNode the class to check
+   * @param logTag a tag to print to the log if a problem is found
+   * @param logger the logger to write to if a problem is found
+   * @return true if the class is ok, false otherwise
+   */
+  public static boolean isClassOk(final Logger logger, final ClassNode classNode, final String logTag) {
+    final StringWriter sw = new StringWriter();
+    final ClassWriter verifyWriter = new ClassWriter(ClassWriter.COMPUTE_FRAMES);
+    classNode.accept(verifyWriter);
+    final ClassReader ver = new ClassReader(verifyWriter.toByteArray());
+    try {
+      DrillCheckClassAdapter.verify(ver, false, new PrintWriter(sw));
+    } catch(Exception e) {
+      logger.info("Caught exception verifying class:");
+      logClass(logger, logTag, classNode);
+      throw e;
+    }
+    final String output = sw.toString();
+    if (!output.isEmpty()) {
+      logger.info("Invalid class:\n" +  output);
+      return false;
+    }
+
+    return true;
+  }
+
+  /**
+   * Check to see if a class is well-formed.
+   *
+   * @param classBytes the bytecode of the class to check
+   * @param logTag a tag to print to the log if a problem is found
+   * @param logger the logger to write to if a problem is found
+   * @return true if the class is ok, false otherwise
+   */
+  public static boolean isClassBytesOk(final Logger logger, final byte[] classBytes, final String logTag) {
+    final ClassNode classNode = classFromBytes(classBytes);
+    return isClassOk(logger, classNode, logTag);
+  }
+
+  /**
+   * Create a ClassNode from bytecode.
+   *
+   * @param classBytes the bytecode
+   * @return the ClassNode
+   */
+  public static ClassNode classFromBytes(final byte[] classBytes) {
+    final ClassNode classNode = new ClassNode(CompilationConfig.ASM_API_VERSION);
+    final ClassReader classReader = new ClassReader(classBytes);
+    classReader.accept(classNode, 0);
+    return classNode;
+  }
+
+  /**
+   * Write a class to the log.
+   *
+   * <p>Writes at level DEBUG.
+   *
+   * @param logTag a tag to print to the log
+   * @param classNode the class
+   * @param logger the logger to write to
+   */
+  public static void logClass(final Logger logger, final String logTag, final ClassNode classNode) {
+    logger.debug(logTag);
+    final StringWriter stringWriter = new StringWriter();
+    final PrintWriter printWriter = new PrintWriter(stringWriter);
+    final TraceClassVisitor traceClassVisitor = new TraceClassVisitor(printWriter);
+    classNode.accept(traceClassVisitor);
+    logger.debug(stringWriter.toString());
+  }
+
+  /**
+   * Write a class to the log.
+   *
+   * <p>Writes at level DEBUG.
+   *
+   * @param logTag a tag to print to the log
+   * @param classBytes the class' bytecode
+   * @param logger the logger to write to
+   */
+  public static void logClassFromBytes(final Logger logger, final String logTag, final byte[] classBytes) {
+    final ClassNode classNode = classFromBytes(classBytes);
+    logClass(logger, logTag, classNode);
+  }
+
+  /**
+   * Determine if the given opcode is a load of a constant (xCONST_y).
+   *
+   * @param opcode the opcode
+   * @return true if the opcode is one of the constant loading ones, false otherwise
+   */
+  public static boolean isXconst(final int opcode) {
+    switch(opcode) {
+    case Opcodes.ICONST_0:
+    case Opcodes.ICONST_1:
+    case Opcodes.ICONST_2:
+    case Opcodes.ICONST_3:
+    case Opcodes.ICONST_4:
+    case Opcodes.ICONST_5:
+    case Opcodes.ICONST_M1:
+    case Opcodes.DCONST_0:
+    case Opcodes.DCONST_1:
+    case Opcodes.FCONST_0:
+    case Opcodes.FCONST_1:
+    case Opcodes.LCONST_0:
+    case Opcodes.LCONST_1:
+      return true;
+    }
+
+    return false;
+  }
+
+  /**
+   * Determine if the given opcode is an ADD of some kind (xADD).
+   *
+   * @param opcode the opcode
+   * @return true if the opcode is one of the ADDs, false otherwise
+   */
+  public static boolean isXadd(final int opcode) {
+    switch(opcode) {
+    case Opcodes.IADD:
+    case Opcodes.DADD:
+    case Opcodes.FADD:
+    case Opcodes.LADD:
+      return true;
+    }
+
+    return false;
+  }
+}