> 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?
The list did get out of sync with the types I was able to fold after some more work, will update accordingly. > 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 104 > > <https://reviews.apache.org/r/30697/diff/6/?file=904523#file904523line104> > > > > Is it better to put this number into a named static const var, and add > > comment to explain why you choose this magic number? Will do > 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 110 > > <https://reviews.apache.org/r/30697/diff/6/?file=904523#file904523line110> > > > > Is it better to pass in isNullable, in stead of passing RexNode node? > > Seems the only information obtained from node is the nullability infor. will do > 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 290 > > <https://reviews.apache.org/r/30697/diff/6/?file=904523#file904523line290> > > > > Will it better to put the mapping of Drill's minorType and Calcite's > > SqlTypeName into a map, so that other codes may also leverage the map, in > > stead of switch statement? There doesn't seem to be too common of a pattern in the codebase to use maps in this way, there are a lot of places where switch statements are used instead. I would be in support of changing it be this way, so I will update it. > 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. 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. - Jason ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30697/#review77612 ----------------------------------------------------------- On March 24, 2015, 4:53 p.m., Jason Altekruse wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30697/ > ----------------------------------------------------------- > > (Updated March 24, 2015, 4:53 p.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 > ----- > > > contrib/storage-hive/core/src/main/java/org/apache/drill/exec/planner/sql/HiveUDFOperator.java > 7524690 > exec/java-exec/src/main/java/io/netty/buffer/DrillBuf.java d87fb76 > > 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/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/main/java/org/apache/drill/exec/store/dfs/easy/EasyGroupScan.java > 54cad56 > 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 > >
