drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jacques Nadeau" <jacques.dr...@gmail.com>
Subject Re: Review Request 31691: DRILL-1385.4.patch.txt: rebase squashed single patch for scalar replacement with option handling cleanup
Date Sun, 15 Mar 2015 20:48:39 GMT

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

Ship it!


Two quick changes then let's get this merged.


exec/java-exec/src/main/java/org/apache/drill/exec/compile/ClassTransformer.java
<https://reviews.apache.org/r/31691/#comment124065>

    It seems like we should just have a clean way for options to map to enums.  Will you file
a separate enhancement request?  Maybe have EnumValidator?
    
    That way we wouldn't have to redo this conversion on every transformation.



exec/java-exec/src/main/java/org/apache/drill/exec/compile/FsmDescriptor.java
<https://reviews.apache.org/r/31691/#comment122744>

    merge issue


- Jacques Nadeau


On March 3, 2015, 6:43 p.m., Chris Westin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31691/
> -----------------------------------------------------------
> 
> (Updated March 3, 2015, 6:43 p.m.)
> 
> 
> Review request for drill and Jacques Nadeau.
> 
> 
> 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 5efcce8 
>   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
f320bbb 
>   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
3d3e96f 
>   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 cf99577 
>   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
bb855c9 
>   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 525579a 
> 
> 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