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/03/09 23:07:27 UTC
Review Request 31871: 2406 - part 2 - (subset of 2060 part 2) enable
interpreted expression evaluation at planning time
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/
-----------------------------------------------------------
Review request for drill and Jacques Nadeau.
Bugs: DRILL-2406
https://issues.apache.org/jira/browse/DRILL-2406
Repository: drill-git
Description
-------
To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
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/memory/TopLevelAllocator.java 2a28bcb
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 aa1dffd
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/DrillReduceAggregatesRule.java 93fff35
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
Diff: https://reviews.apache.org/r/31871/diff/
Testing
-------
Cluster tests completed, waiting on unit tests.
Thanks,
Jason Altekruse
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Jason Altekruse <al...@gmail.com>.
> On March 9, 2015, 11:08 p.m., Jason Altekruse wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java, line 54
> > <https://reviews.apache.org/r/31871/diff/1/?file=889812#file889812line54>
> >
> > The reason this is kept in a variable called random is that we have been calling this probperty isRandom in our function definitions. Calcite uses the opposite flag, of deterministic to refer to the same property. I decided to push this all the way to the point where we connect to calicte to try to avoid confusion or major refactoring for little benefit.
property*
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/#review75807
-----------------------------------------------------------
On March 9, 2015, 10:07 p.m., Jason Altekruse wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31871/
> -----------------------------------------------------------
>
> (Updated March 9, 2015, 10:07 p.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2406
> https://issues.apache.org/jira/browse/DRILL-2406
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
>
>
> 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/memory/TopLevelAllocator.java 2a28bcb
> 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 aa1dffd
> 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/DrillReduceAggregatesRule.java 93fff35
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
>
> Diff: https://reviews.apache.org/r/31871/diff/
>
>
> Testing
> -------
>
> Cluster tests completed, waiting on unit tests.
>
>
> Thanks,
>
> Jason Altekruse
>
>
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Jason Altekruse <al...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/#review75807
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java
<https://reviews.apache.org/r/31871/#comment123110>
I had rebased this on top of Chris' resliency work to see if it would help resolve some of the test failures I was seeing. I misssed reverting these comments once I rebased on top of Drill master.
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java
<https://reviews.apache.org/r/31871/#comment123108>
The reason this is kept in a variable called random is that we have been calling this probperty isRandom in our function definitions. Calcite uses the opposite flag, of deterministic to refer to the same property. I decided to push this all the way to the point where we connect to calicte to try to avoid confusion or major refactoring for little benefit.
- Jason Altekruse
On March 9, 2015, 10:07 p.m., Jason Altekruse wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31871/
> -----------------------------------------------------------
>
> (Updated March 9, 2015, 10:07 p.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2406
> https://issues.apache.org/jira/browse/DRILL-2406
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
>
>
> 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/memory/TopLevelAllocator.java 2a28bcb
> 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 aa1dffd
> 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/DrillReduceAggregatesRule.java 93fff35
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
>
> Diff: https://reviews.apache.org/r/31871/diff/
>
>
> Testing
> -------
>
> Cluster tests completed, waiting on unit tests.
>
>
> Thanks,
>
> Jason Altekruse
>
>
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Jason Altekruse <al...@gmail.com>.
> On March 10, 2015, 8:01 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java, line 165
> > <https://reviews.apache.org/r/31871/diff/1/?file=889813#file889813line165>
> >
> > After my patch, QueryContext no longer needs to be closed -- so there's no possibility of leaking an exception here, because there can't be one. So I wouldn't worry about this for now -- you can leave it as-is if it's working for you, but beware of merging if this goes in after my patch.
After a discussion with Chris I will be making the following changes.
I will make the QueryContext class properly return Exception, not IOException. I had written this originally to implement the Closable interface (which throws IOException) and ended up changing it to AutoClosable during the time I had rebased this on top of Chris' work for testing. This did not cause an issue with compilation or runtime, as IOExcption inherits from Exception, but it did unnecesarily create confusion as I had changed the signature from AutoCloseable.
I will also add a return statement after the call to move to the FAILED state, this will prevent sending the message that was provided to the call to cleanup originally.
As the moveToState method calls cleanup to send the failure message to the client, I will add a flag to QueryContext to make the close method idempotent as encouraged by the AutoClosable documentation. http://docs.oracle.com/javase/7/docs/api/java/lang/AutoCloseable.html
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/#review75958
-----------------------------------------------------------
On March 9, 2015, 10:07 p.m., Jason Altekruse wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31871/
> -----------------------------------------------------------
>
> (Updated March 9, 2015, 10:07 p.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2406
> https://issues.apache.org/jira/browse/DRILL-2406
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
>
>
> 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/memory/TopLevelAllocator.java 2a28bcb
> 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 aa1dffd
> 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/DrillReduceAggregatesRule.java 93fff35
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
>
> Diff: https://reviews.apache.org/r/31871/diff/
>
>
> Testing
> -------
>
> Cluster tests completed, waiting on unit tests.
>
>
> Thanks,
>
> Jason Altekruse
>
>
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/#review75958
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java
<https://reviews.apache.org/r/31871/#comment123307>
After my patch, QueryContext no longer needs to be closed -- so there's no possibility of leaking an exception here, because there can't be one. So I wouldn't worry about this for now -- you can leave it as-is if it's working for you, but beware of merging if this goes in after my patch.
- Chris Westin
On March 9, 2015, 3:07 p.m., Jason Altekruse wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31871/
> -----------------------------------------------------------
>
> (Updated March 9, 2015, 3:07 p.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2406
> https://issues.apache.org/jira/browse/DRILL-2406
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
>
>
> 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/memory/TopLevelAllocator.java 2a28bcb
> 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 aa1dffd
> 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/DrillReduceAggregatesRule.java 93fff35
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
>
> Diff: https://reviews.apache.org/r/31871/diff/
>
>
> Testing
> -------
>
> Cluster tests completed, waiting on unit tests.
>
>
> Thanks,
>
> Jason Altekruse
>
>
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Jason Altekruse <al...@gmail.com>.
> On March 10, 2015, 9:06 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java, line 26
> > <https://reviews.apache.org/r/31871/diff/1/?file=889807#file889807line26>
> >
> > This should be AutoCloseable, not Closeable. Note that close() throws Exception then.
Fixed
> On March 10, 2015, 9:06 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java, line 103
> > <https://reviews.apache.org/r/31871/diff/1/?file=889809#file889809line103>
> >
> > Does getAllocator() really need to be public?
Unfortunately it will for now. There is a pattern throughout the code to require direct access to an allocator to create a Vector or a ValueHolder of a type that is not stored on heap (such as varchar). The primary interfaces they interact with are defined in TypeHelper, such as getNewVector(). As almost all of these calls are invoked from operators with an allocator available through OperatorContext, there might be a case that we should provide the interface to request these kind of resources directly from their repsective contexts, rather than requesting an allocator and passing it into a static utility method. This would allow us to create a more explicit contract within operators to specifically allow creation of vectors, or managed buffers (mangement might have to change scope from the fragment to Operators themselves to allow more flexibility). Giving direct access to allocators provides everyone with access to the public buffer() methods, while this is currently used sparringl
y, we should probably discuss the possibility of standardizing how Operators manage and properly close general purpose buffers that may not be well suited for the current managed buffer pattern used in UDFs.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/#review75973
-----------------------------------------------------------
On March 9, 2015, 10:07 p.m., Jason Altekruse wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31871/
> -----------------------------------------------------------
>
> (Updated March 9, 2015, 10:07 p.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2406
> https://issues.apache.org/jira/browse/DRILL-2406
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
>
>
> 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/memory/TopLevelAllocator.java 2a28bcb
> 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 aa1dffd
> 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/DrillReduceAggregatesRule.java 93fff35
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
>
> Diff: https://reviews.apache.org/r/31871/diff/
>
>
> Testing
> -------
>
> Cluster tests completed, waiting on unit tests.
>
>
> Thanks,
>
> Jason Altekruse
>
>
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/#review75973
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java
<https://reviews.apache.org/r/31871/#comment123329>
This should be AutoCloseable, not Closeable. Note that close() throws Exception then.
exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java
<https://reviews.apache.org/r/31871/#comment123330>
Please document what the units for these are. Bytes? KB? MB? GB?
exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java
<https://reviews.apache.org/r/31871/#comment123337>
Just an FYI - https://issues.apache.org/jira/browse/DRILL-2421
exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java
<https://reviews.apache.org/r/31871/#comment123338>
Please add this comment:
// TODO(DRILL-1942) the new allocator has this capability built-in, so this can be removed once that is available
exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java
<https://reviews.apache.org/r/31871/#comment123339>
Does getAllocator() really need to be public?
exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java
<https://reviews.apache.org/r/31871/#comment123340>
throws Exception, since it's AutoCloseable.
exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java
<https://reviews.apache.org/r/31871/#comment123341>
Please add this comment:
// TODO(DRILL-1942) the new allocator has this capability built-in, so this can be removed once that is available
- Chris Westin
On March 9, 2015, 3:07 p.m., Jason Altekruse wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31871/
> -----------------------------------------------------------
>
> (Updated March 9, 2015, 3:07 p.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2406
> https://issues.apache.org/jira/browse/DRILL-2406
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
>
>
> 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/memory/TopLevelAllocator.java 2a28bcb
> 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 aa1dffd
> 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/DrillReduceAggregatesRule.java 93fff35
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
>
> Diff: https://reviews.apache.org/r/31871/diff/
>
>
> Testing
> -------
>
> Cluster tests completed, waiting on unit tests.
>
>
> Thanks,
>
> Jason Altekruse
>
>
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Jason Altekruse <al...@gmail.com>.
> On March 10, 2015, 4:45 a.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java, line 165
> > <https://reviews.apache.org/r/31871/diff/1/?file=889813#file889813line165>
> >
> > Seems like this should be Exception, not just IOException.
>
> Jason Altekruse wrote:
> This is catching the expected exception from calling close on the QueryContext. This is partially an artifact of the use of the AutoClosable interface, which could just be cahged to closable. If we want to catch unexpected exceptions here as well should I put the calls above to retire the forman, and remove the fragment and drillbit status listeners inside of the try block as well?
>
> Jacques Nadeau wrote:
> Please review with Chris. We shouldn't leak exceptions (caught and uncaught). In this case I want to make sure that we're not.
see response below, Chris made a new comment when I think he meant to respond to this one.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/#review75851
-----------------------------------------------------------
On March 10, 2015, 11:21 p.m., Jason Altekruse wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31871/
> -----------------------------------------------------------
>
> (Updated March 10, 2015, 11:21 p.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2406
> https://issues.apache.org/jira/browse/DRILL-2406
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
>
>
> 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/memory/BufferAllocator.java 30b905f
> exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java 2a28bcb
> 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/fragment/SimpleParallelizer.java f8d1803
> 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/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java 4ab3cc0
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java fc1c6b9
> exec/java-exec/src/test/java/org/apache/drill/exec/pop/TestFragmentChecker.java 07d310a
>
> Diff: https://reviews.apache.org/r/31871/diff/
>
>
> Testing
> -------
>
> Cluster tests completed, waiting on unit tests.
>
>
> Thanks,
>
> Jason Altekruse
>
>
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Jason Altekruse <al...@gmail.com>.
> On March 10, 2015, 4:45 a.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java, line 165
> > <https://reviews.apache.org/r/31871/diff/1/?file=889813#file889813line165>
> >
> > Seems like this should be Exception, not just IOException.
This is catching the expected exception from calling close on the QueryContext. This is partially an artifact of the use of the AutoClosable interface, which could just be cahged to closable. If we want to catch unexpected exceptions here as well should I put the calls above to retire the forman, and remove the fragment and drillbit status listeners inside of the try block as well?
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/#review75851
-----------------------------------------------------------
On March 9, 2015, 10:07 p.m., Jason Altekruse wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31871/
> -----------------------------------------------------------
>
> (Updated March 9, 2015, 10:07 p.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2406
> https://issues.apache.org/jira/browse/DRILL-2406
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
>
>
> 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/memory/TopLevelAllocator.java 2a28bcb
> 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 aa1dffd
> 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/DrillReduceAggregatesRule.java 93fff35
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
>
> Diff: https://reviews.apache.org/r/31871/diff/
>
>
> Testing
> -------
>
> Cluster tests completed, waiting on unit tests.
>
>
> Thanks,
>
> Jason Altekruse
>
>
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Jacques Nadeau <ja...@gmail.com>.
> On March 10, 2015, 4:45 a.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java, line 165
> > <https://reviews.apache.org/r/31871/diff/1/?file=889813#file889813line165>
> >
> > Seems like this should be Exception, not just IOException.
>
> Jason Altekruse wrote:
> This is catching the expected exception from calling close on the QueryContext. This is partially an artifact of the use of the AutoClosable interface, which could just be cahged to closable. If we want to catch unexpected exceptions here as well should I put the calls above to retire the forman, and remove the fragment and drillbit status listeners inside of the try block as well?
Please review with Chris. We shouldn't leak exceptions (caught and uncaught). In this case I want to make sure that we're not.
- Jacques
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/#review75851
-----------------------------------------------------------
On March 9, 2015, 10:07 p.m., Jason Altekruse wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31871/
> -----------------------------------------------------------
>
> (Updated March 9, 2015, 10:07 p.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2406
> https://issues.apache.org/jira/browse/DRILL-2406
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
>
>
> 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/memory/TopLevelAllocator.java 2a28bcb
> 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 aa1dffd
> 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/DrillReduceAggregatesRule.java 93fff35
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
>
> Diff: https://reviews.apache.org/r/31871/diff/
>
>
> Testing
> -------
>
> Cluster tests completed, waiting on unit tests.
>
>
> Thanks,
>
> Jason Altekruse
>
>
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Jason Altekruse <al...@gmail.com>.
> On March 10, 2015, 4:45 a.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java, line 166
> > <https://reviews.apache.org/r/31871/diff/1/?file=889813#file889813line166>
> >
> > Does this work correct in relationship to sendResult? It seems like if everything was okay but then we failed just above, we should send a failure rather than a success result.
I just tried this out by adding an exception to the close method. Currently we are reaching the COMPLETED state before we have actually finished the query, so the attempted transition to the failed state here is dropped. This is fixed in Chris' patch, so I will add the necessary defensive programming here to assume that the state transitions will be fixed soon, but for now the failure code will do nothing.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/#review75851
-----------------------------------------------------------
On March 9, 2015, 10:07 p.m., Jason Altekruse wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31871/
> -----------------------------------------------------------
>
> (Updated March 9, 2015, 10:07 p.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2406
> https://issues.apache.org/jira/browse/DRILL-2406
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
>
>
> 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/memory/TopLevelAllocator.java 2a28bcb
> 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 aa1dffd
> 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/DrillReduceAggregatesRule.java 93fff35
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
>
> Diff: https://reviews.apache.org/r/31871/diff/
>
>
> Testing
> -------
>
> Cluster tests completed, waiting on unit tests.
>
>
> Thanks,
>
> Jason Altekruse
>
>
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/#review75851
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java
<https://reviews.apache.org/r/31871/#comment123169>
Seems like this should be Exception, not just IOException.
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java
<https://reviews.apache.org/r/31871/#comment123170>
Does this work correct in relationship to sendResult? It seems like if everything was okay but then we failed just above, we should send a failure rather than a success result.
- Jacques Nadeau
On March 9, 2015, 10:07 p.m., Jason Altekruse wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31871/
> -----------------------------------------------------------
>
> (Updated March 9, 2015, 10:07 p.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2406
> https://issues.apache.org/jira/browse/DRILL-2406
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
>
>
> 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/memory/TopLevelAllocator.java 2a28bcb
> 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 aa1dffd
> 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/DrillReduceAggregatesRule.java 93fff35
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
>
> Diff: https://reviews.apache.org/r/31871/diff/
>
>
> Testing
> -------
>
> Cluster tests completed, waiting on unit tests.
>
>
> Thanks,
>
> Jason Altekruse
>
>
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Jason Altekruse <al...@gmail.com>.
> On March 10, 2015, 4:52 a.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java, line 43
> > <https://reviews.apache.org/r/31871/diff/1/?file=889812#file889812line43>
> >
> > Can you update this interface to be isDeterministic? Update func holder as well. Simply keep the old interface of isRandom() and mark as deprecated. Additionally, please provide a default constructor that is isDeterministic = true so that we don't have to change all the places were we assumed that behavior.
>
> Jason Altekruse wrote:
> I will start this work, but I was trying to avoid confusion within Drill as much as possible, as well as major refactoring. If we want to change this pattern everywhere, that would include where we define all of the non-deterministic UDFs (although admittedly there aren't too many of them). I had chosen this point as it was exactly where we meet with the Calcite API.
I'm nervous about this being inconsistent within Drill. I believe that if we are going to try to be consistent with Calcite, we should change how we refer to this property everywhere. I have a patch that does the renaming, UDF updates and change the interface to Hive UDFS that I think is necessary to make this complete change. I would advocate for splitting this into a separate JIRA if we want to make the change.
I have updated all current references to this modified constructor in the patch already posted, so there is no need for a default constructor that does not provide determinism. While indeterministic functions are rare, I think it would be best to leave this required so any new function types defined will be explicitly created to fill this field appropriately.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/#review75852
-----------------------------------------------------------
On March 10, 2015, 11:21 p.m., Jason Altekruse wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31871/
> -----------------------------------------------------------
>
> (Updated March 10, 2015, 11:21 p.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2406
> https://issues.apache.org/jira/browse/DRILL-2406
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
>
>
> 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/memory/BufferAllocator.java 30b905f
> exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java 2a28bcb
> 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/fragment/SimpleParallelizer.java f8d1803
> 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/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java 4ab3cc0
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java fc1c6b9
> exec/java-exec/src/test/java/org/apache/drill/exec/pop/TestFragmentChecker.java 07d310a
>
> Diff: https://reviews.apache.org/r/31871/diff/
>
>
> Testing
> -------
>
> Cluster tests completed, waiting on unit tests.
>
>
> Thanks,
>
> Jason Altekruse
>
>
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Jason Altekruse <al...@gmail.com>.
> On March 10, 2015, 4:52 a.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java, line 183
> > <https://reviews.apache.org/r/31871/diff/1/?file=889809#file889809line183>
> >
> > This should be fixed. Otherwise, you're simply introducing a bug the moment that we reduce an expression containing current date in a constant expression.
I had originally thought we would need to diverge the current time storage for physical plan execution and sql query planning, I realized that we actually create a QueryContext in either case, so I just moved to population of QueryDateTimeInfo there and passed it down to the plan serialization.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/#review75852
-----------------------------------------------------------
On March 11, 2015, 1 a.m., Jason Altekruse wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31871/
> -----------------------------------------------------------
>
> (Updated March 11, 2015, 1 a.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2406
> https://issues.apache.org/jira/browse/DRILL-2406
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
>
>
> 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/memory/BufferAllocator.java 30b905f
> exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java 2a28bcb
> 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 108f5bb
> 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/fragment/SimpleParallelizer.java f8d1803
> 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/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java 4ab3cc0
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java fc1c6b9
> exec/java-exec/src/test/java/org/apache/drill/exec/pop/TestFragmentChecker.java 07d310a
>
> Diff: https://reviews.apache.org/r/31871/diff/
>
>
> Testing
> -------
>
> Cluster tests completed, waiting on unit tests.
>
>
> Thanks,
>
> Jason Altekruse
>
>
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Jason Altekruse <al...@gmail.com>.
> On March 10, 2015, 4:52 a.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java, line 43
> > <https://reviews.apache.org/r/31871/diff/1/?file=889812#file889812line43>
> >
> > Can you update this interface to be isDeterministic? Update func holder as well. Simply keep the old interface of isRandom() and mark as deprecated. Additionally, please provide a default constructor that is isDeterministic = true so that we don't have to change all the places were we assumed that behavior.
>
> Jason Altekruse wrote:
> I will start this work, but I was trying to avoid confusion within Drill as much as possible, as well as major refactoring. If we want to change this pattern everywhere, that would include where we define all of the non-deterministic UDFs (although admittedly there aren't too many of them). I had chosen this point as it was exactly where we meet with the Calcite API.
>
> Jason Altekruse wrote:
> I'm nervous about this being inconsistent within Drill. I believe that if we are going to try to be consistent with Calcite, we should change how we refer to this property everywhere. I have a patch that does the renaming, UDF updates and change the interface to Hive UDFS that I think is necessary to make this complete change. I would advocate for splitting this into a separate JIRA if we want to make the change.
>
> I have updated all current references to this modified constructor in the patch already posted, so there is no need for a default constructor that does not provide determinism. While indeterministic functions are rare, I think it would be best to leave this required so any new function types defined will be explicitly created to fill this field appropriately.
This has not been tested, but this is what I believe the changeset for tracking determinism would require. https://github.com/jaltekruse/incubator-drill/commit/40d475324255df5b3313ca877b678c3796051fb8
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/#review75852
-----------------------------------------------------------
On March 10, 2015, 11:21 p.m., Jason Altekruse wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31871/
> -----------------------------------------------------------
>
> (Updated March 10, 2015, 11:21 p.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2406
> https://issues.apache.org/jira/browse/DRILL-2406
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
>
>
> 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/memory/BufferAllocator.java 30b905f
> exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java 2a28bcb
> 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/fragment/SimpleParallelizer.java f8d1803
> 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/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java 4ab3cc0
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java fc1c6b9
> exec/java-exec/src/test/java/org/apache/drill/exec/pop/TestFragmentChecker.java 07d310a
>
> Diff: https://reviews.apache.org/r/31871/diff/
>
>
> Testing
> -------
>
> Cluster tests completed, waiting on unit tests.
>
>
> Thanks,
>
> Jason Altekruse
>
>
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Jason Altekruse <al...@gmail.com>.
> On March 10, 2015, 4:52 a.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java, line 43
> > <https://reviews.apache.org/r/31871/diff/1/?file=889812#file889812line43>
> >
> > Can you update this interface to be isDeterministic? Update func holder as well. Simply keep the old interface of isRandom() and mark as deprecated. Additionally, please provide a default constructor that is isDeterministic = true so that we don't have to change all the places were we assumed that behavior.
I will start this work, but I was trying to avoid confusion within Drill as much as possible, as well as major refactoring. If we want to change this pattern everywhere, that would include where we define all of the non-deterministic UDFs (although admittedly there aren't too many of them). I had chosen this point as it was exactly where we meet with the Calcite API.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/#review75852
-----------------------------------------------------------
On March 9, 2015, 10:07 p.m., Jason Altekruse wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31871/
> -----------------------------------------------------------
>
> (Updated March 9, 2015, 10:07 p.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2406
> https://issues.apache.org/jira/browse/DRILL-2406
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
>
>
> 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/memory/TopLevelAllocator.java 2a28bcb
> 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 aa1dffd
> 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/DrillReduceAggregatesRule.java 93fff35
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
>
> Diff: https://reviews.apache.org/r/31871/diff/
>
>
> Testing
> -------
>
> Cluster tests completed, waiting on unit tests.
>
>
> Thanks,
>
> Jason Altekruse
>
>
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/#review75852
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java
<https://reviews.apache.org/r/31871/#comment123171>
Give short description of purpose
exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java
<https://reviews.apache.org/r/31871/#comment123172>
Disagree with TODO. Please remove. Our goal is to not expose outer context to inner contexts.
exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java
<https://reviews.apache.org/r/31871/#comment123173>
This comment seems like it could easily become wrong. I would remove. People can simply find search code for references to getAllocator which would be a better indiciator of why this exists.
exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java
<https://reviews.apache.org/r/31871/#comment123174>
This should be fixed. Otherwise, you're simply introducing a bug the moment that we reduce an expression containing current date in a constant expression.
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java
<https://reviews.apache.org/r/31871/#comment123175>
Can you update this interface to be isDeterministic? Update func holder as well. Simply keep the old interface of isRandom() and mark as deprecated. Additionally, please provide a default constructor that is isDeterministic = true so that we don't have to change all the places were we assumed that behavior.
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java
<https://reviews.apache.org/r/31871/#comment123176>
Can you update this interface to be isDeterministic? Update func holder as well. Simply keep the old interface of isRandom() and mark as deprecated. Additionally, please provide a default constructor that is isDeterministic = true so that we don't have to change all the places were we assumed that behavior.
- Jacques Nadeau
On March 9, 2015, 10:07 p.m., Jason Altekruse wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31871/
> -----------------------------------------------------------
>
> (Updated March 9, 2015, 10:07 p.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2406
> https://issues.apache.org/jira/browse/DRILL-2406
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
>
>
> 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/memory/TopLevelAllocator.java 2a28bcb
> 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 aa1dffd
> 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/DrillReduceAggregatesRule.java 93fff35
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
>
> Diff: https://reviews.apache.org/r/31871/diff/
>
>
> Testing
> -------
>
> Cluster tests completed, waiting on unit tests.
>
>
> Thanks,
>
> Jason Altekruse
>
>
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Jason Altekruse <al...@gmail.com>.
> On March 11, 2015, 9:52 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java, line 27
> > <https://reviews.apache.org/r/31871/diff/1-3/?file=889807#file889807line27>
> >
> > Should be @link
>
> Jason Altekruse wrote:
> Did not realize this, I have this misused in a number of my comments in the constant folding patches. I will correct them.
These issues have been fixed throughout my patches, will be posting updated to all of them shortly.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/#review76135
-----------------------------------------------------------
On March 11, 2015, 1 a.m., Jason Altekruse wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31871/
> -----------------------------------------------------------
>
> (Updated March 11, 2015, 1 a.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2406
> https://issues.apache.org/jira/browse/DRILL-2406
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
>
>
> 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/memory/BufferAllocator.java 30b905f
> exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java 2a28bcb
> 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 108f5bb
> 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/fragment/SimpleParallelizer.java f8d1803
> 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/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java 4ab3cc0
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java fc1c6b9
> exec/java-exec/src/test/java/org/apache/drill/exec/pop/TestFragmentChecker.java 07d310a
>
> Diff: https://reviews.apache.org/r/31871/diff/
>
>
> Testing
> -------
>
> Cluster tests completed, waiting on unit tests.
>
>
> Thanks,
>
> Jason Altekruse
>
>
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Jason Altekruse <al...@gmail.com>.
> On March 11, 2015, 9:52 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java, line 115
> > <https://reviews.apache.org/r/31871/diff/1-3/?file=889809#file889809line115>
> >
> > Why did you take away the finals?
I took out things that I had accidentally pulled in from your patch. As I have made changes nearby here I'm hoping it will make the conflicts smaller when we have to merge the stuff together again in whichever direction it happens.
> On March 11, 2015, 9:52 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java, line 27
> > <https://reviews.apache.org/r/31871/diff/1-3/?file=889807#file889807line27>
> >
> > Should be @link
Did not realize this, I have this misused in a number of my comments in the constant folding patches. I will correct them.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/#review76135
-----------------------------------------------------------
On March 11, 2015, 1 a.m., Jason Altekruse wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31871/
> -----------------------------------------------------------
>
> (Updated March 11, 2015, 1 a.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2406
> https://issues.apache.org/jira/browse/DRILL-2406
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
>
>
> 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/memory/BufferAllocator.java 30b905f
> exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java 2a28bcb
> 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 108f5bb
> 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/fragment/SimpleParallelizer.java f8d1803
> 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/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java 4ab3cc0
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java fc1c6b9
> exec/java-exec/src/test/java/org/apache/drill/exec/pop/TestFragmentChecker.java 07d310a
>
> Diff: https://reviews.apache.org/r/31871/diff/
>
>
> Testing
> -------
>
> Cluster tests completed, waiting on unit tests.
>
>
> Thanks,
>
> Jason Altekruse
>
>
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Chris Westin <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/#review76135
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java
<https://reviews.apache.org/r/31871/#comment123591>
Should be @link
exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java
<https://reviews.apache.org/r/31871/#comment123593>
Why did you take away the finals?
- Chris Westin
On March 10, 2015, 6 p.m., Jason Altekruse wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31871/
> -----------------------------------------------------------
>
> (Updated March 10, 2015, 6 p.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2406
> https://issues.apache.org/jira/browse/DRILL-2406
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
>
>
> 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/memory/BufferAllocator.java 30b905f
> exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java 2a28bcb
> 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 108f5bb
> 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/fragment/SimpleParallelizer.java f8d1803
> 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/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java 4ab3cc0
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java fc1c6b9
> exec/java-exec/src/test/java/org/apache/drill/exec/pop/TestFragmentChecker.java 07d310a
>
> Diff: https://reviews.apache.org/r/31871/diff/
>
>
> Testing
> -------
>
> Cluster tests completed, waiting on unit tests.
>
>
> Thanks,
>
> Jason Altekruse
>
>
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Jason Altekruse <al...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/
-----------------------------------------------------------
(Updated March 17, 2015, 10:04 p.m.)
Review request for drill and Jacques Nadeau.
Changes
-------
Added isDeterministic() to funtion holder, marked isRandom() as deprecated.
Bugs: DRILL-2406
https://issues.apache.org/jira/browse/DRILL-2406
Repository: drill-git
Description
-------
To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFuncHolder.java 279c428
exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFunctionRegistry.java fafc286
exec/java-exec/src/main/java/org/apache/drill/exec/memory/BufferAllocator.java 30b905f
exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java 2a28bcb
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 108f5bb
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/fragment/SimpleParallelizer.java f8d1803
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/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java 4ab3cc0
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java fc1c6b9
exec/java-exec/src/test/java/org/apache/drill/exec/pop/TestFragmentChecker.java 07d310a
Diff: https://reviews.apache.org/r/31871/diff/
Testing
-------
Cluster tests completed, waiting on unit tests.
Thanks,
Jason Altekruse
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/#review76795
-----------------------------------------------------------
Ship it!
Add isDeterministic() to DrillFuncHolder and mark isRandom deprecated and use deterministic through DrillSqlOperator. Otherwise, +1
- Jacques Nadeau
On March 16, 2015, 5:11 p.m., Jason Altekruse wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31871/
> -----------------------------------------------------------
>
> (Updated March 16, 2015, 5:11 p.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2406
> https://issues.apache.org/jira/browse/DRILL-2406
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
>
>
> 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/memory/BufferAllocator.java 30b905f
> exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java 2a28bcb
> 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 108f5bb
> 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/fragment/SimpleParallelizer.java f8d1803
> 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/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java 4ab3cc0
> exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java fc1c6b9
> exec/java-exec/src/test/java/org/apache/drill/exec/pop/TestFragmentChecker.java 07d310a
>
> Diff: https://reviews.apache.org/r/31871/diff/
>
>
> Testing
> -------
>
> Cluster tests completed, waiting on unit tests.
>
>
> Thanks,
>
> Jason Altekruse
>
>
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Jason Altekruse <al...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/
-----------------------------------------------------------
(Updated March 16, 2015, 5:11 p.m.)
Review request for drill and Jacques Nadeau.
Changes
-------
small doc update
Bugs: DRILL-2406
https://issues.apache.org/jira/browse/DRILL-2406
Repository: drill-git
Description
-------
To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
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/memory/BufferAllocator.java 30b905f
exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java 2a28bcb
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 108f5bb
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/fragment/SimpleParallelizer.java f8d1803
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/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java 4ab3cc0
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java fc1c6b9
exec/java-exec/src/test/java/org/apache/drill/exec/pop/TestFragmentChecker.java 07d310a
Diff: https://reviews.apache.org/r/31871/diff/
Testing
-------
Cluster tests completed, waiting on unit tests.
Thanks,
Jason Altekruse
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Jason Altekruse <al...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/
-----------------------------------------------------------
(Updated March 11, 2015, 1 a.m.)
Review request for drill and Jacques Nadeau.
Changes
-------
Fixed the getAllocator method in QueryContext to be public again, had temporarily set it to packae to see the impact, only to discover that my use of it happened in a later patch. I had forgotten to revert it. I also re-generated the parent patch so now the changes on FragmentContext properly show here.
Bugs: DRILL-2406
https://issues.apache.org/jira/browse/DRILL-2406
Repository: drill-git
Description
-------
To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
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/memory/BufferAllocator.java 30b905f
exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java 2a28bcb
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 108f5bb
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/fragment/SimpleParallelizer.java f8d1803
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/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java 4ab3cc0
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java fc1c6b9
exec/java-exec/src/test/java/org/apache/drill/exec/pop/TestFragmentChecker.java 07d310a
Diff: https://reviews.apache.org/r/31871/diff/
Testing
-------
Cluster tests completed, waiting on unit tests.
Thanks,
Jason Altekruse
Re: Review Request 31871: 2406 - part 2 - (subset of 2060 part 2)
enable interpreted expression evaluation at planning time
Posted by Jason Altekruse <al...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31871/
-----------------------------------------------------------
(Updated March 10, 2015, 11:21 p.m.)
Review request for drill and Jacques Nadeau.
Changes
-------
Addresses all review comments except the refactoring to make drill track determinism instead of randomness.
Bugs: DRILL-2406
https://issues.apache.org/jira/browse/DRILL-2406
Repository: drill-git
Description
-------
To enable planning rules to take advantage of constant expressions in queries, or to allow them to run queries against small datasets, such as partition information we need to e able to evaluate expressions at planning time. Expression evaluation in the regular expecuation path is relatively expensive, as it invovles java code generation, compilation and JITing expression trees. An interpreted expression system was added recently to allow evaluating an expression without generating java code at runtime. This patch exposes that work (after some refactorings in 2143 and the first part of 2406) to the planning context for use in optimizer rules.
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/memory/BufferAllocator.java 30b905f
exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java 2a28bcb
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/fragment/SimpleParallelizer.java f8d1803
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/physical/visitor/InsertLocalExchangeVisitor.java 907fcb1
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java 6b54c43
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 409450f
exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java 4ab3cc0
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java fc1c6b9
exec/java-exec/src/test/java/org/apache/drill/exec/pop/TestFragmentChecker.java 07d310a
Diff: https://reviews.apache.org/r/31871/diff/
Testing
-------
Cluster tests completed, waiting on unit tests.
Thanks,
Jason Altekruse