You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Chris Westin <ch...@gmail.com> on 2015/03/03 19:43:23 UTC
Review Request 31691: DRILL-1385.4.patch.txt: rebase squashed single
patch for scalar replacement with option handling cleanup
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31691/
-----------------------------------------------------------
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
Re: Review Request 31691: DRILL-1385.5.patch.txt: rebase squashed
single patch for scalar replacement with option handling cleanup
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
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
Re: Review Request 31691: DRILL-1385.4.patch.txt: rebase squashed
single patch for scalar replacement with option handling cleanup
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Fixed a couple of minor issues from the latest review.
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 (updated)
-----
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
Re: Review Request 31691: DRILL-1385.4.patch.txt: rebase squashed
single patch for scalar replacement with option handling cleanup
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
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
>
>