> On March 24, 2015, 7:27 p.m., Jinfeng Ni wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillConstExecutor.java, > > line 75 > > <https://reviews.apache.org/r/30697/diff/6/?file=904523#file904523line75> > > > > I saw VARBINARY, DECIMAL9 etc in the switch statement in the reduce() > > method. Why are those types listed as NON_REDUCIBLE_TYPES? > > Jason Altekruse wrote: > The list did get out of sync with the types I was able to fold after some > more work, will update accordingly.
I fixed one issue in the materialization of the varbinary calciate literals, only to discover that we are completely lacking the varbinary literal at the Drill logical expression level. I have opened a JIRA to track this. For now I left it in the list of non-reducible types and removed the literal creation from the switch. https://issues.apache.org/jira/browse/DRILL-2551 > On March 24, 2015, 7:27 p.m., Jinfeng Ni wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillConstExecutor.java, > > line 353 > > <https://reviews.apache.org/r/30697/diff/6/?file=904523#file904523line353> > > > > Is better to throw exception or other way to inform there is error, in > > stead of returing the original expression, for those currently unsupported > > types? If the type is not supported, that seems to indicate there is a bug > > in the caller if it tries to call this method in the first place. > > Jason Altekruse wrote: > I ran into a few places where the non-redicuble types list was actually > not being used. Unfortunately for any function that optiq manages itself > (such as the + operator) it does not bother looking at our UDFs for > information about whether or not the function is constant. I was able to fix > all of the cases I ran into, but it seemed safest to just return the original > expression if there is a type we cannot fold. This will allow constant > folding to be left on and avoid throwing an exception during planning. If we > threw an exception here instead, the only workaround would be to turn off the > constant folding rules entirely, which might hurt performance in some cases. I have added a debug log message so we do not lose track of this entirely. As the results of constant folding are so obvious to find the the plans, I think we can easily find individual cases where failed constant folding is impacting performance. I would still argue for not throwing an exception in this case to allow the default on behavior for the constant folding rule not to throw people off. - Jason ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30697/#review77612 ----------------------------------------------------------- On March 26, 2015, 12:01 a.m., Jason Altekruse wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30697/ > ----------------------------------------------------------- > > (Updated March 26, 2015, 12:01 a.m.) > > > Review request for drill, Aditya Kishore, Aman Sinha, Jacques Nadeau, Jinfeng > Ni, Mehant Baid, and Parth Chandra. > > > Bugs: DRILL-2060 > https://issues.apache.org/jira/browse/DRILL-2060 > > > Repository: drill-git > > > Description > ------- > > THIS REQUEST HAS BEEN BROKEN INTO TWO REVIEWS: > - I had to make a fairly substandial change to the interpreteed expression > evaluation to make the tests work. > > Use a small modification of a rule in optiq and hook up the interpreted > expression evaluator to fold constant expressions (including those with Drill > UDFs) into literals. Fragment memory limits have been disabled, a more > complete refactoring of memory management is planned, the changes made here > were to allow the creation of a childAllocator without access to a fragment > context. I have added two comments next to the modifiations I made to the > optiq rule for our use case. > > > Diffs > ----- > > > common/src/main/java/org/apache/drill/common/expression/ExpressionStringBuilder.java > d3839ed > > contrib/storage-hive/core/src/main/java/org/apache/drill/exec/planner/sql/HiveUDFOperator.java > 7524690 > exec/java-exec/src/main/codegen/templates/TypeHelper.java d387b74 > exec/java-exec/src/main/java/io/netty/buffer/DrillBuf.java d87fb76 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java > 6d42136 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFunctionRegistry.java > 32cf362 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctionHelpers.java > 3b1d7ef > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/interpreter/InterpreterEvaluator.java > 35c35ec > exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java > 536f6fd > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/project/ProjectRecordBatch.java > 0c7a71a > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillConstExecutor.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillReduceExpressionsRule.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillRuleSets.java > b1a7189 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java > f320157 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java > 710418b > > exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java > 608fac7 > exec/java-exec/src/test/java/org/apache/drill/PlanningBase.java e673230 > exec/java-exec/src/test/java/org/apache/drill/TestProjectPushDown.java > cbb5c6e > > exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestAggregateFunctions.java > 72ad31a > > exec/java-exec/src/test/java/org/apache/drill/exec/fn/interp/TestConstantFolding.java > PRE-CREATION > exec/java-exec/src/test/resources/parquet/alltypes.json PRE-CREATION > pom.xml 05573a1 > > Diff: https://reviews.apache.org/r/30697/diff/ > > > Testing > ------- > > This is a work in progress, some testing has been done, no full unit test run > yet > > > Thanks, > > Jason Altekruse > >
