You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Sergey Shelukhin <se...@hortonworks.com> on 2016/08/30 21:55:19 UTC
Review Request 51542: HIVE-14652 incorrect results for not in on
partition columns
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51542/
-----------------------------------------------------------
Review request for hive and Jes�s Camacho Rodr�guez.
Summary (updated)
-----------------
HIVE-14652 incorrect results for not in on partition columns
Repository: hive-git
Description (updated)
-------
see jira
Diffs
-----
Diff: https://reviews.apache.org/r/51542/diff/
Testing
-------
Thanks,
Sergey Shelukhin
Re: Review Request 51542: HIVE-14652 incorrect results for not in on
partition columns
Posted by Jesús Camacho Rodríguez <jc...@hortonworks.com>.
> On Aug. 31, 2016, 6:31 p.m., Jes�s Camacho Rodr�guez wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/pcr/PcrExprProcFactory.java, line 286
> > <https://reviews.apache.org/r/51542/diff/1/?file=1488888#file1488888line286>
> >
> > Will STRUCT with all constants inside e.g. (x, y) in (1,2), be handled correctly by this code block? In that case, (1, 2) should be considered a constant, thus we could remove the IN. However, I think currently we will end up with UNKNOWN state and we will never remove it. I think that was a case that we were supporting previously, thus it would be a regression. Could you double check?
>
> Sergey Shelukhin wrote:
> Those are handled elsewhere. See pointlookup tests where they are removed
>
> Jes�s Camacho Rodr�guez wrote:
> I do not understand what you mean. Removal of pcr is handled here. I have been checking : pointlookup tests do not contain any test that removes an IN clause with STRUCT.
>
> Consider the following (based on pointlookup4):
> create table pcr_t1 (key int, value string) partitioned by (ds1 string, ds2 string);
> insert overwrite table pcr_t1 partition (ds1='2000-04-08', ds2='2001-04-08') select * from src where key < 20 order by key;
> insert overwrite table pcr_t1 partition (ds1='2000-04-09', ds2='2001-04-09') select * from src where key < 20 order by key;
> select select key, value, ds1, ds2
> from pcr_t1
> where (struct(ds1,ds2)) IN (struct('2000-04-08','2001-04-08'), struct('2000-04-09','2001-04-09'));
>
> For the query above, once we have executed partitioned prunning, the filter predicate can be removed. I am afraid that with the new change in the code, we will identify the CONST STRUCT e.g. struct('2000-04-08','2001-04-08') as UNKNOWN, and thus we will not consider that predicate for removal. In fact, the extension should be as easy as checking whether the STRUCT is a CONST STRUCT or not, and create the WalkState accordingly.
>
> I think it is important to deal with this case, as I guess this kind of predicates were the objective when support for IN-STRUCT clauses was added to PCR in the first place.
>
> Sergey Shelukhin wrote:
> These should be handled by constant folding in handleDeterministicUdf. Example predicate removed would be and (struct(ds1,ds2)) IN (const struct('2000-04-08','2001-04-08'), const struct('2000-04-09','2001-04-09'))) in pointlookup4 (generated from (ds1='2000-04-08' and ds2='2001-04-08' and key=1) or (ds1='2000-04-09' and ds2='2001-04-09' and key=2))
You are right, as checkForPartColsAndUnknown will return false instead of null. And then it is identified as constant. Thanks for double checking.
- Jes�s
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51542/#review147456
-----------------------------------------------------------
On Aug. 31, 2016, 7:38 p.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51542/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2016, 7:38 p.m.)
>
>
> Review request for hive and Jes�s Camacho Rodr�guez.
>
>
> Bugs: HIVE-14652
> https://issues.apache.org/jira/browse/HIVE-14652
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/pcr/PcrExprProcFactory.java f9388e2
> ql/src/java/org/apache/hadoop/hive/ql/parse/ParseContext.java b2125ca
> ql/src/test/queries/clientpositive/partition_condition_remover.q PRE-CREATION
> ql/src/test/results/clientpositive/partition_condition_remover.q.out PRE-CREATION
> ql/src/test/results/clientpositive/pcs.q.out 0045c1c
>
> Diff: https://reviews.apache.org/r/51542/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 51542: HIVE-14652 incorrect results for not in on
partition columns
Posted by Jesús Camacho Rodríguez <jc...@hortonworks.com>.
> On Aug. 31, 2016, 6:31 p.m., Jes�s Camacho Rodr�guez wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/pcr/PcrExprProcFactory.java, line 375
> > <https://reviews.apache.org/r/51542/diff/1/?file=1488888#file1488888line375>
> >
> > WalkState.CONSTANT or WalkState.UNKNOWN? (I think in this case it should be the latest)
>
> Sergey Shelukhin wrote:
> this is existing code moved as is :)
OK, it is CONSTANT because it is only called in handleDeterministicUDF, after other checks have been done. I honestly would not create its own method for this logic, as it is rather misleading : it does fold the expressions, but it makes some assumptions on the nature of that expression.
> On Aug. 31, 2016, 6:31 p.m., Jes�s Camacho Rodr�guez wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/pcr/PcrExprProcFactory.java, line 286
> > <https://reviews.apache.org/r/51542/diff/1/?file=1488888#file1488888line286>
> >
> > Will STRUCT with all constants inside e.g. (x, y) in (1,2), be handled correctly by this code block? In that case, (1, 2) should be considered a constant, thus we could remove the IN. However, I think currently we will end up with UNKNOWN state and we will never remove it. I think that was a case that we were supporting previously, thus it would be a regression. Could you double check?
>
> Sergey Shelukhin wrote:
> Those are handled elsewhere. See pointlookup tests where they are removed
I do not understand what you mean. Removal of pcr is handled here. I have been checking : pointlookup tests do not contain any test that removes an IN clause with STRUCT.
Consider the following (based on pointlookup4):
create table pcr_t1 (key int, value string) partitioned by (ds1 string, ds2 string);
insert overwrite table pcr_t1 partition (ds1='2000-04-08', ds2='2001-04-08') select * from src where key < 20 order by key;
insert overwrite table pcr_t1 partition (ds1='2000-04-09', ds2='2001-04-09') select * from src where key < 20 order by key;
select select key, value, ds1, ds2
from pcr_t1
where (struct(ds1,ds2)) IN (struct('2000-04-08','2001-04-08'), struct('2000-04-09','2001-04-09'));
For the query above, once we have executed partitioned prunning, the filter predicate can be removed. I am afraid that with the new change in the code, we will identify the CONST STRUCT e.g. struct('2000-04-08','2001-04-08') as UNKNOWN, and thus we will not consider that predicate for removal. In fact, the extension should be as easy as checking whether the STRUCT is a CONST STRUCT or not, and create the WalkState accordingly.
I think it is important to deal with this case, as I guess this kind of predicates were the objective when support for IN-STRUCT clauses was added to PCR in the first place.
- Jes�s
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51542/#review147456
-----------------------------------------------------------
On Aug. 31, 2016, 6:42 p.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51542/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2016, 6:42 p.m.)
>
>
> Review request for hive and Jes�s Camacho Rodr�guez.
>
>
> Bugs: HIVE-14652
> https://issues.apache.org/jira/browse/HIVE-14652
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/pcr/PcrExprProcFactory.java f9388e2
> ql/src/java/org/apache/hadoop/hive/ql/parse/ParseContext.java b2125ca
> ql/src/test/queries/clientpositive/partition_condition_remover.q PRE-CREATION
> ql/src/test/results/clientpositive/partition_condition_remover.q.out PRE-CREATION
> ql/src/test/results/clientpositive/pcs.q.out 0045c1c
>
> Diff: https://reviews.apache.org/r/51542/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 51542: HIVE-14652 incorrect results for not in on
partition columns
Posted by Sergey Shelukhin <se...@hortonworks.com>.
> On Aug. 31, 2016, 6:31 p.m., Jes�s Camacho Rodr�guez wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/pcr/PcrExprProcFactory.java, line 286
> > <https://reviews.apache.org/r/51542/diff/1/?file=1488888#file1488888line286>
> >
> > Will STRUCT with all constants inside e.g. (x, y) in (1,2), be handled correctly by this code block? In that case, (1, 2) should be considered a constant, thus we could remove the IN. However, I think currently we will end up with UNKNOWN state and we will never remove it. I think that was a case that we were supporting previously, thus it would be a regression. Could you double check?
>
> Sergey Shelukhin wrote:
> Those are handled elsewhere. See pointlookup tests where they are removed
>
> Jes�s Camacho Rodr�guez wrote:
> I do not understand what you mean. Removal of pcr is handled here. I have been checking : pointlookup tests do not contain any test that removes an IN clause with STRUCT.
>
> Consider the following (based on pointlookup4):
> create table pcr_t1 (key int, value string) partitioned by (ds1 string, ds2 string);
> insert overwrite table pcr_t1 partition (ds1='2000-04-08', ds2='2001-04-08') select * from src where key < 20 order by key;
> insert overwrite table pcr_t1 partition (ds1='2000-04-09', ds2='2001-04-09') select * from src where key < 20 order by key;
> select select key, value, ds1, ds2
> from pcr_t1
> where (struct(ds1,ds2)) IN (struct('2000-04-08','2001-04-08'), struct('2000-04-09','2001-04-09'));
>
> For the query above, once we have executed partitioned prunning, the filter predicate can be removed. I am afraid that with the new change in the code, we will identify the CONST STRUCT e.g. struct('2000-04-08','2001-04-08') as UNKNOWN, and thus we will not consider that predicate for removal. In fact, the extension should be as easy as checking whether the STRUCT is a CONST STRUCT or not, and create the WalkState accordingly.
>
> I think it is important to deal with this case, as I guess this kind of predicates were the objective when support for IN-STRUCT clauses was added to PCR in the first place.
These should be handled by constant folding in handleDeterministicUdf. Example predicate removed would be and (struct(ds1,ds2)) IN (const struct('2000-04-08','2001-04-08'), const struct('2000-04-09','2001-04-09'))) in pointlookup4 (generated from (ds1='2000-04-08' and ds2='2001-04-08' and key=1) or (ds1='2000-04-09' and ds2='2001-04-09' and key=2))
> On Aug. 31, 2016, 6:31 p.m., Jes�s Camacho Rodr�guez wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/pcr/PcrExprProcFactory.java, line 375
> > <https://reviews.apache.org/r/51542/diff/1/?file=1488888#file1488888line375>
> >
> > WalkState.CONSTANT or WalkState.UNKNOWN? (I think in this case it should be the latest)
>
> Sergey Shelukhin wrote:
> this is existing code moved as is :)
>
> Jes�s Camacho Rodr�guez wrote:
> OK, it is CONSTANT because it is only called in handleDeterministicUDF, after other checks have been done. I honestly would not create its own method for this logic, as it is rather misleading : it does fold the expressions, but it makes some assumptions on the nature of that expression.
fixed
- Sergey
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51542/#review147456
-----------------------------------------------------------
On Aug. 31, 2016, 7:38 p.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51542/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2016, 7:38 p.m.)
>
>
> Review request for hive and Jes�s Camacho Rodr�guez.
>
>
> Bugs: HIVE-14652
> https://issues.apache.org/jira/browse/HIVE-14652
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/pcr/PcrExprProcFactory.java f9388e2
> ql/src/java/org/apache/hadoop/hive/ql/parse/ParseContext.java b2125ca
> ql/src/test/queries/clientpositive/partition_condition_remover.q PRE-CREATION
> ql/src/test/results/clientpositive/partition_condition_remover.q.out PRE-CREATION
> ql/src/test/results/clientpositive/pcs.q.out 0045c1c
>
> Diff: https://reviews.apache.org/r/51542/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 51542: HIVE-14652 incorrect results for not in on
partition columns
Posted by Sergey Shelukhin <se...@hortonworks.com>.
> On Aug. 31, 2016, 6:31 p.m., Jes�s Camacho Rodr�guez wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/pcr/PcrExprProcFactory.java, line 286
> > <https://reviews.apache.org/r/51542/diff/1/?file=1488888#file1488888line286>
> >
> > Will STRUCT with all constants inside e.g. (x, y) in (1,2), be handled correctly by this code block? In that case, (1, 2) should be considered a constant, thus we could remove the IN. However, I think currently we will end up with UNKNOWN state and we will never remove it. I think that was a case that we were supporting previously, thus it would be a regression. Could you double check?
Those are handled elsewhere. See pointlookup tests where they are removed
> On Aug. 31, 2016, 6:31 p.m., Jes�s Camacho Rodr�guez wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/pcr/PcrExprProcFactory.java, line 375
> > <https://reviews.apache.org/r/51542/diff/1/?file=1488888#file1488888line375>
> >
> > WalkState.CONSTANT or WalkState.UNKNOWN? (I think in this case it should be the latest)
this is existing code moved as is :)
- Sergey
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51542/#review147456
-----------------------------------------------------------
On Aug. 30, 2016, 9:56 p.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51542/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2016, 9:56 p.m.)
>
>
> Review request for hive and Jes�s Camacho Rodr�guez.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/pcr/PcrExprProcFactory.java f9388e2
> ql/src/java/org/apache/hadoop/hive/ql/parse/ParseContext.java b2125ca
> ql/src/test/queries/clientpositive/partition_condition_remover.q PRE-CREATION
> ql/src/test/results/clientpositive/partition_condition_remover.q.out PRE-CREATION
> ql/src/test/results/clientpositive/pcs.q.out 0045c1c
>
> Diff: https://reviews.apache.org/r/51542/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 51542: HIVE-14652 incorrect results for not in on
partition columns
Posted by Jesús Camacho Rodríguez <jc...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51542/#review147456
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/optimizer/pcr/PcrExprProcFactory.java (line 286)
<https://reviews.apache.org/r/51542/#comment214641>
Will STRUCT with all constants inside e.g. (x, y) in (1,2), be handled correctly by this code block? In that case, (1, 2) should be considered a constant, thus we could remove the IN. However, I think currently we will end up with UNKNOWN state and we will never remove it. I think that was a case that we were supporting previously, thus it would be a regression. Could you double check?
ql/src/java/org/apache/hadoop/hive/ql/optimizer/pcr/PcrExprProcFactory.java (line 374)
<https://reviews.apache.org/r/51542/#comment214635>
WalkState.CONSTANT or WalkState.UNKNOWN? (I think in this case it should be the latest)
- Jes�s Camacho Rodr�guez
On Aug. 30, 2016, 9:56 p.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51542/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2016, 9:56 p.m.)
>
>
> Review request for hive and Jes�s Camacho Rodr�guez.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/pcr/PcrExprProcFactory.java f9388e2
> ql/src/java/org/apache/hadoop/hive/ql/parse/ParseContext.java b2125ca
> ql/src/test/queries/clientpositive/partition_condition_remover.q PRE-CREATION
> ql/src/test/results/clientpositive/partition_condition_remover.q.out PRE-CREATION
> ql/src/test/results/clientpositive/pcs.q.out 0045c1c
>
> Diff: https://reviews.apache.org/r/51542/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 51542: HIVE-14652 incorrect results for not in on
partition columns
Posted by Jesús Camacho Rodríguez <jc...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51542/#review147482
-----------------------------------------------------------
Ship it!
Ship It!
- Jes�s Camacho Rodr�guez
On Aug. 31, 2016, 7:38 p.m., Sergey Shelukhin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51542/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2016, 7:38 p.m.)
>
>
> Review request for hive and Jes�s Camacho Rodr�guez.
>
>
> Bugs: HIVE-14652
> https://issues.apache.org/jira/browse/HIVE-14652
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> see jira
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/pcr/PcrExprProcFactory.java f9388e2
> ql/src/java/org/apache/hadoop/hive/ql/parse/ParseContext.java b2125ca
> ql/src/test/queries/clientpositive/partition_condition_remover.q PRE-CREATION
> ql/src/test/results/clientpositive/partition_condition_remover.q.out PRE-CREATION
> ql/src/test/results/clientpositive/pcs.q.out 0045c1c
>
> Diff: https://reviews.apache.org/r/51542/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sergey Shelukhin
>
>
Re: Review Request 51542: HIVE-14652 incorrect results for not in on
partition columns
Posted by Sergey Shelukhin <se...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51542/
-----------------------------------------------------------
(Updated Aug. 31, 2016, 7:38 p.m.)
Review request for hive and Jes�s Camacho Rodr�guez.
Bugs: HIVE-14652
https://issues.apache.org/jira/browse/HIVE-14652
Repository: hive-git
Description
-------
see jira
Diffs (updated)
-----
ql/src/java/org/apache/hadoop/hive/ql/optimizer/pcr/PcrExprProcFactory.java f9388e2
ql/src/java/org/apache/hadoop/hive/ql/parse/ParseContext.java b2125ca
ql/src/test/queries/clientpositive/partition_condition_remover.q PRE-CREATION
ql/src/test/results/clientpositive/partition_condition_remover.q.out PRE-CREATION
ql/src/test/results/clientpositive/pcs.q.out 0045c1c
Diff: https://reviews.apache.org/r/51542/diff/
Testing
-------
Thanks,
Sergey Shelukhin
Re: Review Request 51542: HIVE-14652 incorrect results for not in on
partition columns
Posted by Sergey Shelukhin <se...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51542/
-----------------------------------------------------------
(Updated Aug. 31, 2016, 6:42 p.m.)
Review request for hive and Jes�s Camacho Rodr�guez.
Bugs: HIVE-14652
https://issues.apache.org/jira/browse/HIVE-14652
Repository: hive-git
Description
-------
see jira
Diffs
-----
ql/src/java/org/apache/hadoop/hive/ql/optimizer/pcr/PcrExprProcFactory.java f9388e2
ql/src/java/org/apache/hadoop/hive/ql/parse/ParseContext.java b2125ca
ql/src/test/queries/clientpositive/partition_condition_remover.q PRE-CREATION
ql/src/test/results/clientpositive/partition_condition_remover.q.out PRE-CREATION
ql/src/test/results/clientpositive/pcs.q.out 0045c1c
Diff: https://reviews.apache.org/r/51542/diff/
Testing
-------
Thanks,
Sergey Shelukhin
Re: Review Request 51542: HIVE-14652 incorrect results for not in on
partition columns
Posted by Sergey Shelukhin <se...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51542/
-----------------------------------------------------------
(Updated Aug. 30, 2016, 9:56 p.m.)
Review request for hive and Jes�s Camacho Rodr�guez.
Repository: hive-git
Description
-------
see jira
Diffs (updated)
-----
ql/src/java/org/apache/hadoop/hive/ql/optimizer/pcr/PcrExprProcFactory.java f9388e2
ql/src/java/org/apache/hadoop/hive/ql/parse/ParseContext.java b2125ca
ql/src/test/queries/clientpositive/partition_condition_remover.q PRE-CREATION
ql/src/test/results/clientpositive/partition_condition_remover.q.out PRE-CREATION
ql/src/test/results/clientpositive/pcs.q.out 0045c1c
Diff: https://reviews.apache.org/r/51542/diff/
Testing
-------
Thanks,
Sergey Shelukhin