drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Chris Westin" <chriswesti...@gmail.com>
Subject Re: Review Request 31691: DRILL-1385.5.patch.txt: rebase squashed single patch for scalar replacement with option handling cleanup
Date Mon, 16 Mar 2015 21:22:02 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31691/
-----------------------------------------------------------

(Updated March 16, 2015, 2:21 p.m.)


Review request for drill and Jacques Nadeau.


Summary (updated)
-----------------

DRILL-1385.5.patch.txt: rebase squashed single patch for scalar replacement with option handling
cleanup


Bugs: DRILL-1385
    https://issues.apache.org/jira/browse/DRILL-1385


Repository: drill-git


Description
-------

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 replacem
    - 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
      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 variab
        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 effective
        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 place


Diffs
-----

  common/src/main/java/org/apache/drill/common/config/DrillConfig.java dee8a8a 
  common/src/main/java/org/apache/drill/common/util/PathScanner.java 6223777 
  exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java 33b05af 
  exec/java-exec/src/main/java/org/apache/drill/exec/compile/AbstractClassCompiler.java f4a3cc9

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

  exec/java-exec/src/main/java/org/apache/drill/exec/compile/CheckMethodVisitorFsm.java PRE-CREATION

  exec/java-exec/src/main/java/org/apache/drill/exec/compile/ClassTransformer.java 52d9e34

  exec/java-exec/src/main/java/org/apache/drill/exec/compile/CodeCompiler.java 7cc350e 
  exec/java-exec/src/main/java/org/apache/drill/exec/compile/CompilationConfig.java PRE-CREATION

  exec/java-exec/src/main/java/org/apache/drill/exec/compile/DrillCheckClassAdapter.java PRE-CREATION

  exec/java-exec/src/main/java/org/apache/drill/exec/compile/DrillInitMethodVisitor.java 859a14c

  exec/java-exec/src/main/java/org/apache/drill/exec/compile/FsmCursor.java PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/compile/FsmDescriptor.java PRE-CREATION

  exec/java-exec/src/main/java/org/apache/drill/exec/compile/InnerClassAccessStripper.java
PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/compile/JaninoClassCompiler.java 84bf4b0

  exec/java-exec/src/main/java/org/apache/drill/exec/compile/LogWriter.java PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/compile/MergeAdapter.java 62d439c 
  exec/java-exec/src/main/java/org/apache/drill/exec/compile/QueryClassLoader.java 065c3c8

  exec/java-exec/src/main/java/org/apache/drill/exec/compile/RetargetableClassVisitor.java
PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/compile/TemplateClassDefinition.java
2b6ddf0 
  exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/AloadPopRemover.java
PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/DirectSorter.java 4e54bc7

  exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/InstructionModifier.java
4585bd8 
  exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/MethodAnalyzer.java
PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ReplacingBasicValue.java
7b24174 
  exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ReplacingInterpreter.java
2dce4db 
  exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ScalarReplacementNode.java
8a38d32 
  exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ScalarReplacementTypes.java
a54ca72 
  exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/TrackingInstructionList.java
7cab17c 
  exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ValueHolderIden.java
a0ce390 
  exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/ValueHolderReplacementVisitor.java
2cec537 
  exec/java-exec/src/main/java/org/apache/drill/exec/compile/sig/GeneratorMapping.java 57551ed

  exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFunctionRegistry.java 00aaec6

  exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionImplementationRegistry.java
55e4121 
  exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java
c03d6a0 
  exec/java-exec/src/main/java/org/apache/drill/exec/record/SchemaBuilder.java 8bf346d 
  exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java 67342c4 
  exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java 0fb10ff 
  exec/java-exec/src/main/java/org/apache/drill/exec/server/options/DrillConfigIterator.java
e4b03d3 
  exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java
f515f8e 
  exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionManager.java 83af7db

  exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValidator.java 5b90ba5

  exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValue.java bfcbeca

  exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
d821af8 
  exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java e53fcfe

  exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java 4d2a0df 
  exec/java-exec/src/test/java/org/apache/drill/TestBugFixes.java 51f3121 
  exec/java-exec/src/test/java/org/apache/drill/TestTpchDistributed.java 254d9be 
  exec/java-exec/src/test/java/org/apache/drill/exec/compile/TestClassTransformation.java
7ff00fe 
  exec/java-exec/src/test/java/org/apache/drill/exec/compile/bytecode/ReplaceMethodInvoke.java
f9971a7 
  exec/java-exec/src/test/java/org/apache/drill/exec/expr/fn/impl/TestSimpleRepeatedFunctions.java
8c75feb 
  exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestConvertFunctions.java
a6cce3c 
  pom.xml 0e56ed6 

Diff: https://reviews.apache.org/r/31691/diff/


Testing
-------

mvn install
Functional - Passing - new => 3 known test failures: count1.q, count2.q, joinCSVParquet.q
cased by DRILL-2236.
Advanced - TPCH SF100 - Parquet => known failures of 01.q and 10.q


Thanks,

Chris Westin


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message