----------------------------------------------------------- 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 > >
