You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Aman Sinha <as...@maprtech.com> on 2015/03/26 04:49:23 UTC
Review Request 32523: DRILL-2568: Conditionally drop filter during
partition pruning
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32523/
-----------------------------------------------------------
Review request for drill and Jacques Nadeau.
Bugs: DRILL-2568
https://issues.apache.org/jira/browse/DRILL-2568
Repository: drill-git
Description
-------
Drop the filter plan node if all conjuncts in the filter have been pushed as part of partition pruning, except for the situation where the new set of files is empty - we add a single file in that case, so the Filter is preserved in that case.
Diffs
-----
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/partition/PruneScanRule.java b8c9ebf
exec/java-exec/src/test/java/org/apache/drill/exec/expr/TestPrune.java d15555e
Diff: https://reviews.apache.org/r/32523/diff/
Testing
-------
Manual inspection of Explain plans for several types of queries. Running regression tests..
Thanks,
Aman Sinha
Re: Review Request 32523: DRILL-2568: Conditionally drop filter during
partition pruning
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32523/#review78132
-----------------------------------------------------------
Ship it!
Let's not worry about partial drop. Otherwise, lgtm. +1
- Jacques Nadeau
On March 26, 2015, 3:49 a.m., Aman Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32523/
> -----------------------------------------------------------
>
> (Updated March 26, 2015, 3:49 a.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2568
> https://issues.apache.org/jira/browse/DRILL-2568
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Drop the filter plan node if all conjuncts in the filter have been pushed as part of partition pruning, except for the situation where the new set of files is empty - we add a single file in that case, so the Filter is preserved in that case.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/partition/PruneScanRule.java b8c9ebf
> exec/java-exec/src/test/java/org/apache/drill/exec/expr/TestPrune.java d15555e
>
> Diff: https://reviews.apache.org/r/32523/diff/
>
>
> Testing
> -------
>
> Manual inspection of Explain plans for several types of queries. Running regression tests..
>
>
> Thanks,
>
> Aman Sinha
>
>
Re: Review Request 32523: DRILL-2568: Conditionally drop filter during
partition pruning
Posted by Aman Sinha <as...@maprtech.com>.
> On March 26, 2015, 4:08 a.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/partition/PruneScanRule.java, line 271
> > <https://reviews.apache.org/r/32523/diff/1/?file=906436#file906436line271>
> >
> > Shouldn't this be a new filter with the newCondition if canDropFilter and only include full filter in case that !canDropFilter?
We need to check for both canDropFilter and newCondition.isAlwaysTrue (this indicates all conjuncts were pushed). We never create a new FilterRel with new conditions - we always either copy the existing FilterRel or drop it completely.
- Aman
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32523/#review77861
-----------------------------------------------------------
On March 26, 2015, 3:49 a.m., Aman Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32523/
> -----------------------------------------------------------
>
> (Updated March 26, 2015, 3:49 a.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2568
> https://issues.apache.org/jira/browse/DRILL-2568
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Drop the filter plan node if all conjuncts in the filter have been pushed as part of partition pruning, except for the situation where the new set of files is empty - we add a single file in that case, so the Filter is preserved in that case.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/partition/PruneScanRule.java b8c9ebf
> exec/java-exec/src/test/java/org/apache/drill/exec/expr/TestPrune.java d15555e
>
> Diff: https://reviews.apache.org/r/32523/diff/
>
>
> Testing
> -------
>
> Manual inspection of Explain plans for several types of queries. Running regression tests..
>
>
> Thanks,
>
> Aman Sinha
>
>
Re: Review Request 32523: DRILL-2568: Conditionally drop filter during
partition pruning
Posted by Jacques Nadeau <ja...@gmail.com>.
> On March 26, 2015, 4:08 a.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/partition/PruneScanRule.java, line 271
> > <https://reviews.apache.org/r/32523/diff/1/?file=906436#file906436line271>
> >
> > Shouldn't this be a new filter with the newCondition if canDropFilter and only include full filter in case that !canDropFilter?
>
> Aman Sinha wrote:
> We need to check for both canDropFilter and newCondition.isAlwaysTrue (this indicates all conjuncts were pushed). We never create a new FilterRel with new conditions - we always either copy the existing FilterRel or drop it completely.
I'm saying: we have a new partial filter condition. In that case, why don't you recreate the filter with the partial filter condition? It seems trivial given the nature of this code and seems to always to do the optimization rather than all or nothing.
- Jacques
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32523/#review77861
-----------------------------------------------------------
On March 26, 2015, 3:49 a.m., Aman Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32523/
> -----------------------------------------------------------
>
> (Updated March 26, 2015, 3:49 a.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2568
> https://issues.apache.org/jira/browse/DRILL-2568
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Drop the filter plan node if all conjuncts in the filter have been pushed as part of partition pruning, except for the situation where the new set of files is empty - we add a single file in that case, so the Filter is preserved in that case.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/partition/PruneScanRule.java b8c9ebf
> exec/java-exec/src/test/java/org/apache/drill/exec/expr/TestPrune.java d15555e
>
> Diff: https://reviews.apache.org/r/32523/diff/
>
>
> Testing
> -------
>
> Manual inspection of Explain plans for several types of queries. Running regression tests..
>
>
> Thanks,
>
> Aman Sinha
>
>
Re: Review Request 32523: DRILL-2568: Conditionally drop filter during
partition pruning
Posted by Aman Sinha <as...@maprtech.com>.
> On March 26, 2015, 4:08 a.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/partition/PruneScanRule.java, line 271
> > <https://reviews.apache.org/r/32523/diff/1/?file=906436#file906436line271>
> >
> > Shouldn't this be a new filter with the newCondition if canDropFilter and only include full filter in case that !canDropFilter?
>
> Aman Sinha wrote:
> We need to check for both canDropFilter and newCondition.isAlwaysTrue (this indicates all conjuncts were pushed). We never create a new FilterRel with new conditions - we always either copy the existing FilterRel or drop it completely.
>
> Jacques Nadeau wrote:
> I'm saying: we have a new partial filter condition. In that case, why don't you recreate the filter with the partial filter condition? It seems trivial given the nature of this code and seems to always to do the optimization rather than all or nothing.
I could do that...I was being conservative since we are about to release 0.8. Let me make the change and run tests.
- Aman
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32523/#review77861
-----------------------------------------------------------
On March 26, 2015, 3:49 a.m., Aman Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32523/
> -----------------------------------------------------------
>
> (Updated March 26, 2015, 3:49 a.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2568
> https://issues.apache.org/jira/browse/DRILL-2568
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Drop the filter plan node if all conjuncts in the filter have been pushed as part of partition pruning, except for the situation where the new set of files is empty - we add a single file in that case, so the Filter is preserved in that case.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/partition/PruneScanRule.java b8c9ebf
> exec/java-exec/src/test/java/org/apache/drill/exec/expr/TestPrune.java d15555e
>
> Diff: https://reviews.apache.org/r/32523/diff/
>
>
> Testing
> -------
>
> Manual inspection of Explain plans for several types of queries. Running regression tests..
>
>
> Thanks,
>
> Aman Sinha
>
>
Re: Review Request 32523: DRILL-2568: Conditionally drop filter during
partition pruning
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32523/#review77861
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/partition/PruneScanRule.java
<https://reviews.apache.org/r/32523/#comment126184>
Shouldn't this be a new filter with the newCondition if canDropFilter and only include full filter in case that !canDropFilter?
- Jacques Nadeau
On March 26, 2015, 3:49 a.m., Aman Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32523/
> -----------------------------------------------------------
>
> (Updated March 26, 2015, 3:49 a.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-2568
> https://issues.apache.org/jira/browse/DRILL-2568
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Drop the filter plan node if all conjuncts in the filter have been pushed as part of partition pruning, except for the situation where the new set of files is empty - we add a single file in that case, so the Filter is preserved in that case.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/partition/PruneScanRule.java b8c9ebf
> exec/java-exec/src/test/java/org/apache/drill/exec/expr/TestPrune.java d15555e
>
> Diff: https://reviews.apache.org/r/32523/diff/
>
>
> Testing
> -------
>
> Manual inspection of Explain plans for several types of queries. Running regression tests..
>
>
> Thanks,
>
> Aman Sinha
>
>
Re: Review Request 32523: DRILL-2568: Conditionally drop filter during
partition pruning
Posted by Aman Sinha <as...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32523/
-----------------------------------------------------------
(Updated March 28, 2015, 7:46 a.m.)
Review request for drill and Jacques Nadeau.
Changes
-------
Updating the patch for compeletenss sake; it does not need another review - the main change from the previous patch is that I have added plan checking and results validation checking for several tests in TestPartitionFilter. Also moved all tests within TestPrune into TestPartitionFilter and removed TestPrune.
Bugs: DRILL-2568
https://issues.apache.org/jira/browse/DRILL-2568
Repository: drill-git
Description
-------
Drop the filter plan node if all conjuncts in the filter have been pushed as part of partition pruning, except for the situation where the new set of files is empty - we add a single file in that case, so the Filter is preserved in that case.
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/partition/FindPartitionConditions.java 3acf29d
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/partition/PruneScanRule.java 413259d
exec/java-exec/src/test/java/org/apache/drill/TestPartitionFilter.java 6b904ec
exec/java-exec/src/test/java/org/apache/drill/exec/expr/TestPrune.java d15555e
Diff: https://reviews.apache.org/r/32523/diff/
Testing
-------
Manual inspection of Explain plans for several types of queries. Running regression tests..
Thanks,
Aman Sinha