You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Jason Altekruse <al...@gmail.com> on 2015/02/28 01:09:17 UTC

Re: Review Request 30697: Drill-2060 - part 2 - constant expression folding during planning

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30697/
-----------------------------------------------------------

(Updated Feb. 28, 2015, 12:09 a.m.)


Review request for drill, Aditya Kishore, Aman Sinha, Jacques Nadeau, Jinfeng Ni, Mehant Baid, and Parth Chandra.


Changes
-------

This issue has been split into two reviews. The changes to this portion include mostly adding more type support to constant folding and fixing the placement of the close() method call for query context.


Summary (updated)
-----------------

Drill-2060 - part 2 - constant expression folding during planning


Bugs: DRILL-2060
    https://issues.apache.org/jira/browse/DRILL-2060


Repository: drill-git


Description (updated)
-------

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 (updated)
-----

  exec/java-exec/src/main/java/io/netty/buffer/DrillBuf.java 8e9d395 
  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/memory/TopLevelAllocator.java 67e1fdb 
  exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java e413921 
  exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java c881432 
  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/DrillReduceAggregatesRule.java 93fff35 
  exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillReduceExpressionsRules.java PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillRuleSets.java 3b7adca 
  exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java d9fc997 
  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/work/foreman/Foreman.java 378e81a 
  exec/java-exec/src/test/java/org/apache/drill/PlanningBase.java e673230 
  exec/java-exec/src/test/java/org/apache/drill/exec/fn/interp/TestConstantFolding.java PRE-CREATION 
  pom.xml 525579a 

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


Re: Review Request 30697: Drill-2060 - part 2 - constant expression folding during planning

Posted by Jason Altekruse <al...@gmail.com>.

> On March 16, 2015, 9:22 p.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillReduceExpressionsRules.java, line 40
> > <https://reviews.apache.org/r/30697/diff/4/?file=896064#file896064line40>
> >
> >     I notice the rule class name is plural (Rules instead of Rule) - could you just use 'Rule' such that it is consistent with other rules. I understand you are creating 2 instances but I think we do that elsewhere also.

Fair enough, seems to be a pattern in calcite as well.


> On March 16, 2015, 9:22 p.m., Aman Sinha wrote:
> > exec/java-exec/src/test/java/org/apache/drill/exec/fn/interp/TestConstantFolding.java, line 58
> > <https://reviews.apache.org/r/30697/diff/4/?file=896069#file896069line58>
> >
> >     You might want to add a test case with slightly more complex expression containing constants and functions:  e.g  ABS((6-18)/(2*3)) should produce 2.  Also add a test case with string literals.

Added.


- Jason


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30697/#review76637
-----------------------------------------------------------


On March 20, 2015, 5:30 p.m., Jason Altekruse wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30697/
> -----------------------------------------------------------
> 
> (Updated March 20, 2015, 5:30 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/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
> 
>


Re: Review Request 30697: Drill-2060 - part 2 - constant expression folding during planning

Posted by Aman Sinha <as...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30697/#review76637
-----------------------------------------------------------


Couple of comments below; overall looks good.


exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillReduceExpressionsRules.java
<https://reviews.apache.org/r/30697/#comment124208>

    I notice the rule class name is plural (Rules instead of Rule) - could you just use 'Rule' such that it is consistent with other rules. I understand you are creating 2 instances but I think we do that elsewhere also.



exec/java-exec/src/test/java/org/apache/drill/exec/fn/interp/TestConstantFolding.java
<https://reviews.apache.org/r/30697/#comment124226>

    You might want to add a test case with slightly more complex expression containing constants and functions:  e.g  ABS((6-18)/(2*3)) should produce 2.  Also add a test case with string literals.


- Aman Sinha


On March 16, 2015, 5:17 p.m., Jason Altekruse wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30697/
> -----------------------------------------------------------
> 
> (Updated March 16, 2015, 5:17 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
> -----
> 
>   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/interpreter/InterpreterEvaluator.java 0fe36cb 
>   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/DrillReduceExpressionsRules.java PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillRuleSets.java 496bc9a 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java 710418b 
>   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/interp/TestConstantFolding.java PRE-CREATION 
>   exec/java-exec/src/test/resources/parquet/alltypes.json PRE-CREATION 
>   pom.xml 0e56ed6 
> 
> 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
> 
>


Re: Review Request 30697: Drill-2060 - part 2 - constant expression folding during planning

Posted by Jason Altekruse <al...@gmail.com>.

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


Re: Review Request 30697: Drill-2060 - part 2 - constant expression folding during planning

Posted by Jason Altekruse <al...@gmail.com>.

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


Re: Review Request 30697: Drill-2060 - part 2 - constant expression folding during planning

Posted by Jinfeng Ni <jn...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30697/#review77612
-----------------------------------------------------------



exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillConstExecutor.java
<https://reviews.apache.org/r/30697/#comment125749>

    I saw VARBINARY, DECIMAL9 etc in the switch statement in the reduce() method. Why are those types listed as NON_REDUCIBLE_TYPES?



exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillConstExecutor.java
<https://reviews.apache.org/r/30697/#comment125759>

    Is it better to put this number into a named static const var, and add comment to explain why you choose this magic number?



exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillConstExecutor.java
<https://reviews.apache.org/r/30697/#comment125748>

    Is it better to pass in isNullable, in stead of passing RexNode node? Seems the only information obtained from node is the nullability infor.



exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillConstExecutor.java
<https://reviews.apache.org/r/30697/#comment125757>

    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?



exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillConstExecutor.java
<https://reviews.apache.org/r/30697/#comment125752>

    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.


- Jinfeng Ni


On March 24, 2015, 9:53 a.m., Jason Altekruse wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30697/
> -----------------------------------------------------------
> 
> (Updated March 24, 2015, 9:53 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
> -----
> 
>   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
> 
>


Re: Review Request 30697: Drill-2060 - part 2 - constant expression folding during planning

Posted by Aman Sinha <as...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30697/#review77833
-----------------------------------------------------------

Ship it!


Ship It!

- Aman Sinha


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


Re: Review Request 30697: Drill-2060 - part 2 - constant expression folding during planning

Posted by Jinfeng Ni <jn...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30697/#review77851
-----------------------------------------------------------

Ship it!


Ship It!

- Jinfeng Ni


On March 25, 2015, 5:01 p.m., Jason Altekruse wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30697/
> -----------------------------------------------------------
> 
> (Updated March 25, 2015, 5:01 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
> -----
> 
>   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
> 
>


Re: Review Request 30697: Drill-2060 - part 2 - constant expression folding during planning

Posted by Jason Altekruse <al...@gmail.com>.
-----------------------------------------------------------
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.


Changes
-------

Addressed review comments


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 (updated)
-----

  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


Re: Review Request 30697: Drill-2060 - part 2 - constant expression folding during planning

Posted by Jason Altekruse <al...@gmail.com>.

> On March 25, 2015, 12:55 a.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/easy/EasyGroupScan.java, line 139
> > <https://reviews.apache.org/r/30697/diff/6/?file=904529#file904529line139>
> >
> >     I feel that the estRowCount should use a formula that is consistent - in this case the formula is data/<row size>.  This will reduce chances of introducing unintended side effects.  So, instead of using the number of chunks, could we scale down the denominator until we get a non-zero estRowCount ? 
> >     
> >     Also, I would be in favor of treating this as a separate jira since it was exposed by partition pruning.
> 
> Jason Altekruse wrote:
>     Sounds good, I'll open a JIRA and mark the test ignored with a JIRA number.

https://issues.apache.org/jira/browse/DRILL-2553


- Jason


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30697/#review77690
-----------------------------------------------------------


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


Re: Review Request 30697: Drill-2060 - part 2 - constant expression folding during planning

Posted by Jason Altekruse <al...@gmail.com>.

> On March 25, 2015, 12:55 a.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/easy/EasyGroupScan.java, line 139
> > <https://reviews.apache.org/r/30697/diff/6/?file=904529#file904529line139>
> >
> >     I feel that the estRowCount should use a formula that is consistent - in this case the formula is data/<row size>.  This will reduce chances of introducing unintended side effects.  So, instead of using the number of chunks, could we scale down the denominator until we get a non-zero estRowCount ? 
> >     
> >     Also, I would be in favor of treating this as a separate jira since it was exposed by partition pruning.

Sounds good, I'll open a JIRA and mark the test ignored with a JIRA number.


- Jason


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30697/#review77690
-----------------------------------------------------------


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


Re: Review Request 30697: Drill-2060 - part 2 - constant expression folding during planning

Posted by Aman Sinha <as...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30697/#review77690
-----------------------------------------------------------



exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillReduceExpressionsRule.java
<https://reviews.apache.org/r/30697/#comment125873>

    You could have a single function createEmptyRelOrEquivalent(cluster, traitset, child...) that could be invoked for both FilterRel and CalcRel.



exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/easy/EasyGroupScan.java
<https://reviews.apache.org/r/30697/#comment125884>

    I feel that the estRowCount should use a formula that is consistent - in this case the formula is data/<row size>.  This will reduce chances of introducing unintended side effects.  So, instead of using the number of chunks, could we scale down the denominator until we get a non-zero estRowCount ? 
    
    Also, I would be in favor of treating this as a separate jira since it was exposed by partition pruning.


- Aman Sinha


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


Re: Review Request 30697: Drill-2060 - part 2 - constant expression folding during planning

Posted by Jason Altekruse <al...@gmail.com>.

> On March 24, 2015, 9:58 p.m., Jinfeng Ni wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillReduceExpressionsRule.java, line 40
> > <https://reviews.apache.org/r/30697/diff/6/?file=904524#file904524line40>
> >
> >     This is not an issue for now, since Drill uses a forked version of Calcite. But in the Calcite master branch, one restriction is added for the naming convention for the RelOptRule. If we move Drill on top of the Calcite master (which I'm doing right now), then it will cause problem. Could you please modify the rule name here?   
> >     
> >         if (!description.matches("[A-Za-z][-A-Za-z0-9_.():]*")) {
> >           throw new RuntimeException("Rule description '" + description
> >               + "' is not valid");
> >         }

Fixed


- Jason


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30697/#review77666
-----------------------------------------------------------


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


Re: Review Request 30697: Drill-2060 - part 2 - constant expression folding during planning

Posted by Jinfeng Ni <jn...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30697/#review77666
-----------------------------------------------------------



exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillReduceExpressionsRule.java
<https://reviews.apache.org/r/30697/#comment125835>

    This is not an issue for now, since Drill uses a forked version of Calcite. But in the Calcite master branch, one restriction is added for the naming convention for the RelOptRule. If we move Drill on top of the Calcite master (which I'm doing right now), then it will cause problem. Could you please modify the rule name here?   
    
        if (!description.matches("[A-Za-z][-A-Za-z0-9_.():]*")) {
          throw new RuntimeException("Rule description '" + description
              + "' is not valid");
        }


- Jinfeng Ni


On March 24, 2015, 9:53 a.m., Jason Altekruse wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30697/
> -----------------------------------------------------------
> 
> (Updated March 24, 2015, 9:53 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
> -----
> 
>   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
> 
>


Re: Review Request 30697: Drill-2060 - part 2 - constant expression folding during planning

Posted by Jason Altekruse <al...@gmail.com>.
-----------------------------------------------------------
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.


Changes
-------

Fix failing testcase with small change to the cost calcuation of an EasyGroupScan in the case of very small files. Also fixed matierializing null outputs from folded constant expressions.


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 (updated)
-----

  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


Re: Review Request 30697: Drill-2060 - part 2 - constant expression folding during planning

Posted by Jason Altekruse <al...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30697/
-----------------------------------------------------------

(Updated March 20, 2015, 5:30 p.m.)


Review request for drill, Aditya Kishore, Aman Sinha, Jacques Nadeau, Jinfeng Ni, Mehant Baid, and Parth Chandra.


Changes
-------

Address Aman's review comments, fix a few last failing tests, one of the unit tests was actually causing a re-allocation of a buffer during a contstant expression evaluation, this was not being handled properly as the DrillBuf was previously relying on having a FragmentContext or an OperatorContext to appropriately manage replacing the buffers.


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 (updated)
-----

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


Re: Review Request 30697: Drill-2060 - part 2 - constant expression folding during planning

Posted by Jason Altekruse <al...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30697/
-----------------------------------------------------------

(Updated March 16, 2015, 5:17 p.m.)


Review request for drill, Aditya Kishore, Aman Sinha, Jacques Nadeau, Jinfeng Ni, Mehant Baid, and Parth Chandra.


Changes
-------

Update with new patch, significant portions have been separated into a separate issue DRILL-2406, the diff is easiest to view alone (as in view it from orig-4 on reviewboard) the 3-4 diff looks very messy as it shows everything that was stripped out of this changeset and put into the other one.


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 (updated)
-----

  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/interpreter/InterpreterEvaluator.java 0fe36cb 
  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/DrillReduceExpressionsRules.java PRE-CREATION 
  exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillRuleSets.java 496bc9a 
  exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java 710418b 
  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/interp/TestConstantFolding.java PRE-CREATION 
  exec/java-exec/src/test/resources/parquet/alltypes.json PRE-CREATION 
  pom.xml 0e56ed6 

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