Pig, mail # dev - Review Request: [PIG-3173] - Partition filter pushdown does not happen if partition keys condition include a AND and OR construct

Review Request: [PIG-3173] - Partition filter pushdown does not happen if partition keys condition include a AND and OR construct

Rohini Palaniswamy 2013-03-20, 00:16

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

Review request for pig.
Description
-------

1) Fixed cases where partition pushdown was not happening for AND and OR construct
2) Commented out the negative test cases as they were actually not asserting anything.
This addresses bug PIG-3173.
   https://issues.apache.org/jira/browse/PIG-3173
Diffs
-----

  http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/PColFilterExtractor.java 1458047
  http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestPartitionFilterPushDown.java 1458047

Diff: https://reviews.apache.org/r/10035/diff/
Testing
-------

Unit tests added and tested few cases manually with hcat.
Thanks,

Rohini Palaniswamy

Re: Review Request: [PIG-3173] - Partition filter pushdown does not happen if partition keys condition include a AND and OR construct

Dmitriy Ryaboy 2013-03-20, 00:42

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

http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/PColFilterExtractor.java
<https://reviews.apache.org/r/10035/#comment38252>

   remove dead code?

http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/PColFilterExtractor.java
<https://reviews.apache.org/r/10035/#comment38255>

   (A and B) or (C and D)

   is impossible if (A or C) is false. We can push this up, while retaining the original filter to apply the original filter.

http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/PColFilterExtractor.java
<https://reviews.apache.org/r/10035/#comment38253>

   looks like tabulation is off?

http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/PColFilterExtractor.java
<https://reviews.apache.org/r/10035/#comment38254>

   please fix whitespace

http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestPartitionFilterPushDown.java
<https://reviews.apache.org/r/10035/#comment38256>

   is this new code or is this RB being funny?

   either way, dead code can be deleted; this is preferable to commenting it out.
- Dmitriy Ryaboy
On March 20, 2013, 12:16 a.m., Rohini Palaniswamy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10035/
> -----------------------------------------------------------
>
> (Updated March 20, 2013, 12:16 a.m.)
>
>
> Review request for pig.
>
>
> Description
> -------
>
> 1) Fixed cases where partition pushdown was not happening for AND and OR construct
> 2) Commented out the negative test cases as they were actually not asserting anything.
>
>
> This addresses bug PIG-3173.
> https://issues.apache.org/jira/browse/PIG-3173
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/PColFilterExtractor.java 1458047
> http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestPartitionFilterPushDown.java 1458047
>
> Diff: https://reviews.apache.org/r/10035/diff/
>
>
> Testing
> -------
>
> Unit tests added and tested few cases manually with hcat.
>
>
> Thanks,
>
> Rohini Palaniswamy
>
>

Re: Review Request: [PIG-3173] - Partition filter pushdown does not happen if partition keys condition include a AND and OR construct

Rohini Palaniswamy 2013-03-20, 19:02

> On March 20, 2013, 12:42 a.m., Dmitriy Ryaboy wrote:
> > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/PColFilterExtractor.java, line 229
> > <https://reviews.apache.org/r/10035/diff/1/?file=272254#file272254line229>
> >
> > looks like tabulation is off?

The whole file uses tab for indent instead of spaces. The code I added has space for indent which makes it look as if the indentation is wrong in the patch. But viewing in a editor it is fine. Based on discussion in PIG-3008, new code or modifications should adhere to "No tabs, No trailing white spaces, 4 spaces to indent the code" and not change parts of the code not touched or apply common sense as applicable. But in this case, changing tabs to spaces will change every line of code and also if blocks are not indented properly in many places. So did not make further whitespace changes.
> On March 20, 2013, 12:42 a.m., Dmitriy Ryaboy wrote:
> > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/PColFilterExtractor.java, line 220
> > <https://reviews.apache.org/r/10035/diff/1/?file=272254#file272254line220>
> >
> > remove dead code?

Will do
> On March 20, 2013, 12:42 a.m., Dmitriy Ryaboy wrote:
> > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/PColFilterExtractor.java, line 240
> > <https://reviews.apache.org/r/10035/diff/1/?file=272254#file272254line240>
> >
> > please fix whitespace

Will do
> On March 20, 2013, 12:42 a.m., Dmitriy Ryaboy wrote:
> > http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestPartitionFilterPushDown.java, line 433
> > <https://reviews.apache.org/r/10035/diff/1/?file=272255#file272255line433>
> >
> > is this new code or is this RB being funny?
> >
> > either way, dead code can be deleted; this is preferable to commenting it out.

I didn't want those test cases to get lost. So commented them out for now. Will revert it in this patch and create a separate jira to write equivalent test cases that actually assert for those cases. Need to have this class well tested to avoid full table scans with hcatalog.
> On March 20, 2013, 12:42 a.m., Dmitriy Ryaboy wrote:
> > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/PColFilterExtractor.java, line 224
> > <https://reviews.apache.org/r/10035/diff/1/?file=272254#file272254line224>
> >
> > (A and B) or (C and D)
> >
> > is impossible if (A or C) is false. We can push this up, while retaining the original filter to apply the original filter.

So just to confirm, you want to extract A and C from each AND condition and push (A OR C) as the partition filter for optimization and still leave ((A AND B) or (C AND D)) to be applied on each tuple?
- Rohini
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10035/#review18127
-----------------------------------------------------------
On March 20, 2013, 12:16 a.m., Rohini Palaniswamy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10035/
> -----------------------------------------------------------
>
> (Updated March 20, 2013, 12:16 a.m.)
>
>
> Review request for pig.
>
>
> Description
> -------
>
> 1) Fixed cases where partition pushdown was not happening for AND and OR construct
> 2) Commented out the negative test cases as they were actually not asserting anything.
>
>
> This addresses bug PIG-3173.
> https://issues.apache.org/jira/browse/PIG-3173
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/PColFilterExtractor.java 1458047
> http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestPartitionFilterPushDown.java 1458047
>
> Diff: https://reviews.apache.org/r/10035/diff/
>
>
> Testing
> -------
>
> Unit tests added and tested few cases manually with hcat.

Re: Review Request: [PIG-3173] - Partition filter pushdown does not happen if partition keys condition include a AND and OR construct

Dmitriy Ryaboy 2013-03-20, 19:45

> On March 20, 2013, 12:42 a.m., Dmitriy Ryaboy wrote:
> > http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/PColFilterExtractor.java, line 224
> > <https://reviews.apache.org/r/10035/diff/1/?file=272254#file272254line224>
> >
> > (A and B) or (C and D)
> >
> > is impossible if (A or C) is false. We can push this up, while retaining the original filter to apply the original filter.
>
> Rohini Palaniswamy wrote:
> So just to confirm, you want to extract A and C from each AND condition and push (A OR C) as the partition filter for optimization and still leave ((A AND B) or (C AND D)) to be applied on each tuple?

correct, unless my logic is wrong.

I actually think we made a bad decision when we decided that if we can push partitions down, we can drop the filter on the pig side -- this means we can't take advantage of partial filters loaders might support (for example, a bloom filter a loader can consult to return just the rows that "probably" match the condition, as opposed to definitely match. With filter removal, we have to have loaders implement a second-pass filtering on top of such filters).
- Dmitriy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10035/#review18127
-----------------------------------------------------------
On March 20, 2013, 12:16 a.m., Rohini Palaniswamy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10035/
> -----------------------------------------------------------
>
> (Updated March 20, 2013, 12:16 a.m.)
>
>
> Review request for pig.
>
>
> Description
> -------
>
> 1) Fixed cases where partition pushdown was not happening for AND and OR construct
> 2) Commented out the negative test cases as they were actually not asserting anything.
>
>
> This addresses bug PIG-3173.
> https://issues.apache.org/jira/browse/PIG-3173
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/PColFilterExtractor.java 1458047
> http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestPartitionFilterPushDown.java 1458047
>
> Diff: https://reviews.apache.org/r/10035/diff/
>
>
> Testing
> -------
>
> Unit tests added and tested few cases manually with hcat.
>
>
> Thanks,
>
> Rohini Palaniswamy
>
>

Re: Review Request: [PIG-3173] - Partition filter pushdown does not happen if partition keys condition include a AND and OR construct

Rohini Palaniswamy 2013-04-29, 20:22

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

(Updated April 29, 2013, 8:22 p.m.)
Review request for pig.
Changes
-------

Created PIG-3300 to address the partial filter condition extraction optimization. Addressed the rest of the comments in this patch.
Description
-------

1) Fixed cases where partition pushdown was not happening for AND and OR construct
2) Commented out the negative test cases as they were actually not asserting anything.
This addresses bug PIG-3173.
   https://issues.apache.org/jira/browse/PIG-3173
Diffs (updated)
-----

  http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/PColFilterExtractor.java 1467858
  http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestPartitionFilterPushDown.java 1467858

Diff: https://reviews.apache.org/r/10035/diff/
Testing
-------

Unit tests added and tested few cases manually with hcat.
Thanks,

Rohini Palaniswamy

Re: Review Request: [PIG-3173] - Partition filter pushdown does not happen if partition keys condition include a AND and OR construct

Cheolsoo Park 2013-04-29, 20:51

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

Ship it!
Looks good to me.

- Cheolsoo Park
On April 29, 2013, 8:22 p.m., Rohini Palaniswamy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10035/
> -----------------------------------------------------------
>
> (Updated April 29, 2013, 8:22 p.m.)
>
>
> Review request for pig.
>
>
> Description
> -------
>
> 1) Fixed cases where partition pushdown was not happening for AND and OR construct
> 2) Commented out the negative test cases as they were actually not asserting anything.
>
>
> This addresses bug PIG-3173.
> https://issues.apache.org/jira/browse/PIG-3173
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/newplan/PColFilterExtractor.java 1467858
> http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestPartitionFilterPushDown.java 1467858
>
> Diff: https://reviews.apache.org/r/10035/diff/
>
>
> Testing
> -------
>
> Unit tests added and tested few cases manually with hcat.
>
>
> Thanks,
>
> Rohini Palaniswamy
>
>