From dev-return-11017-apmail-drill-dev-archive=drill.apache.org@drill.apache.org Tue Mar 3 18:43:25 2015 Return-Path: X-Original-To: apmail-drill-dev-archive@www.apache.org Delivered-To: apmail-drill-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 4D414103E7 for ; Tue, 3 Mar 2015 18:43:25 +0000 (UTC) Received: (qmail 49674 invoked by uid 500); 3 Mar 2015 18:43:25 -0000 Delivered-To: apmail-drill-dev-archive@drill.apache.org Received: (qmail 49614 invoked by uid 500); 3 Mar 2015 18:43:25 -0000 Mailing-List: contact dev-help@drill.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@drill.apache.org Delivered-To: mailing list dev@drill.apache.org Received: (qmail 49599 invoked by uid 500); 3 Mar 2015 18:43:24 -0000 Delivered-To: apmail-incubator-drill-dev@incubator.apache.org Received: (qmail 49595 invoked by uid 99); 3 Mar 2015 18:43:24 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 03 Mar 2015 18:43:24 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id F228B1CAC21; Tue, 3 Mar 2015 18:43:23 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============3419009536601833719==" MIME-Version: 1.0 Subject: Review Request 31691: DRILL-1385.4.patch.txt: rebase squashed single patch for scalar replacement with option handling cleanup From: "Chris Westin" To: "Jacques Nadeau" Cc: "Chris Westin" , "drill" Date: Tue, 03 Mar 2015 18:43:23 -0000 Message-ID: <20150303184323.12263.77889@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: "Chris Westin" X-ReviewGroup: drill-git X-ReviewRequest-URL: https://reviews.apache.org/r/31691/ X-Sender: "Chris Westin" Reply-To: "Chris Westin" X-ReviewRequest-Repository: drill-git --===============3419009536601833719== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- 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 --===============3419009536601833719==--