Review Request 35973: Patch for DRILL-3410

2015-06-27 Thread Steven Phillips 

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

Review request for drill.


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


Repository: drill-git


Description
---

DRILL-3410: rewrite OR and AND operators to have only 2 operands so partitoning 
pruning will work correctly


Diffs
-

  
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/partition/PruneScanRule.java
 ae183313f5bcfc394ffb61c766562f9102cf5a87 
  
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/partition/RewriteAsBinaryOperators.java
 PRE-CREATION 
  exec/java-exec/src/test/java/org/apache/drill/TestCTASPartitionFilter.java 
3943426ed7763986f131dfc2428b994935ced305 

Diff: https://reviews.apache.org/r/35973/diff/


Testing
---


Thanks,

Steven Phillips

Re: Review Request 35973: Patch for DRILL-3410

2015-06-27 Thread Aman Sinha 

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



exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/partition/PruneScanRule.java
 (line 328)


remove this line.



exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/partition/PruneScanRule.java
 (line 342)


Interestingly, when DrillOptiq.visitCall() processes the operator, it 
appears as BINARY even though it could have more than than 2 inputs.  One could 
enhance the PruneScanRule to handle N input boolean operators (without the 
RewriteAsBinaryOperators) or one could potentially move the 
RewriteAsBinaryOperators to DrillOptiq such that the rewrite is done up-front.  
However, these can be future tasks to consider, not an issue for this patch.



exec/java-exec/src/test/java/org/apache/drill/TestCTASPartitionFilter.java 
(line 75)


Since this is like a temporary table, it would be good to have a name 
specific to this test instead of a generic name that could conflict with 
another table.


- Aman Sinha


On June 27, 2015, 6:30 p.m., Steven Phillips wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35973/
> ---
> 
> (Updated June 27, 2015, 6:30 p.m.)
> 
> 
> Review request for drill.
> 
> 
> Bugs: DRILL-3410
> https://issues.apache.org/jira/browse/DRILL-3410
> 
> 
> Repository: drill-git
> 
> 
> Description
> ---
> 
> DRILL-3410: rewrite OR and AND operators to have only 2 operands so 
> partitoning pruning will work correctly
> 
> 
> Diffs
> -
> 
>   
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/partition/PruneScanRule.java
>  ae183313f5bcfc394ffb61c766562f9102cf5a87 
>   
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/partition/RewriteAsBinaryOperators.java
>  PRE-CREATION 
>   exec/java-exec/src/test/java/org/apache/drill/TestCTASPartitionFilter.java 
> 3943426ed7763986f131dfc2428b994935ced305 
> 
> Diff: https://reviews.apache.org/r/35973/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steven Phillips
> 
>

Re: Review Request 35973: Patch for DRILL-3410

2015-06-27 Thread Aman Sinha 

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

Ship it!


Prior comments are non-blockers; marking it ship-it.

- Aman Sinha


On June 27, 2015, 6:30 p.m., Steven Phillips wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35973/
> ---
> 
> (Updated June 27, 2015, 6:30 p.m.)
> 
> 
> Review request for drill.
> 
> 
> Bugs: DRILL-3410
> https://issues.apache.org/jira/browse/DRILL-3410
> 
> 
> Repository: drill-git
> 
> 
> Description
> ---
> 
> DRILL-3410: rewrite OR and AND operators to have only 2 operands so 
> partitoning pruning will work correctly
> 
> 
> Diffs
> -
> 
>   
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/partition/PruneScanRule.java
>  ae183313f5bcfc394ffb61c766562f9102cf5a87 
>   
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/partition/RewriteAsBinaryOperators.java
>  PRE-CREATION 
>   exec/java-exec/src/test/java/org/apache/drill/TestCTASPartitionFilter.java 
> 3943426ed7763986f131dfc2428b994935ced305 
> 
> Diff: https://reviews.apache.org/r/35973/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steven Phillips
> 
>

Re: Review Request 35973: Patch for DRILL-3410

2015-06-27 Thread Steven Phillips 


> On June 27, 2015, 9:35 p.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/partition/PruneScanRule.java,
> >  line 342
> > 
> >
> > Interestingly, when DrillOptiq.visitCall() processes the operator, it 
> > appears as BINARY even though it could have more than than 2 inputs.  One 
> > could enhance the PruneScanRule to handle N input boolean operators 
> > (without the RewriteAsBinaryOperators) or one could potentially move the 
> > RewriteAsBinaryOperators to DrillOptiq such that the rewrite is done 
> > up-front.  However, these can be future tasks to consider, not an issue for 
> > this patch.

Your suggestions make sense, but in this case, I wanted to make the simplest, 
least-impactful change possible, at least for now.


- Steven


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


On June 27, 2015, 6:30 p.m., Steven Phillips wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35973/
> ---
> 
> (Updated June 27, 2015, 6:30 p.m.)
> 
> 
> Review request for drill.
> 
> 
> Bugs: DRILL-3410
> https://issues.apache.org/jira/browse/DRILL-3410
> 
> 
> Repository: drill-git
> 
> 
> Description
> ---
> 
> DRILL-3410: rewrite OR and AND operators to have only 2 operands so 
> partitoning pruning will work correctly
> 
> 
> Diffs
> -
> 
>   
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/partition/PruneScanRule.java
>  ae183313f5bcfc394ffb61c766562f9102cf5a87 
>   
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/partition/RewriteAsBinaryOperators.java
>  PRE-CREATION 
>   exec/java-exec/src/test/java/org/apache/drill/TestCTASPartitionFilter.java 
> 3943426ed7763986f131dfc2428b994935ced305 
> 
> Diff: https://reviews.apache.org/r/35973/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steven Phillips
> 
>