Re: Review Request 69202: HIVE-20804 Further improvements to group by optimization with constraints

[Jesús Camacho Rodríguez]

> On Nov. 7, 2018, 2:09 a.m., Jesús Camacho Rodríguez wrote:
> > ql/src/test/queries/clientpositive/constraints_optimization.q
> > Lines 355 (patched)
> > 
> >
> > Can we add two more tests:
> > - One with column swapping before GroupBy (probably if you use group by 
> > b,c,a and table contains a,b,c, it should work and add the Project in 
> > between the TS and the GroupBy).
> > - One with a join and a group by on one column for other table that is 
> > also the join key of the table where all columns are coming from (as in the 
> > whiteboard).
> 
> Vineet Garg wrote:
> I have added the first test, but the one with join doesn't work
> e.g. 
> -- transitive equivalence on pk column, therefore all other columns 
> shoule be removed
> EXPLAIN   CBO
> SELECT
>   C_FIRST_NAME
> FROM
>   CUSTOMER
> , STORE_SALES
> WHERE
>   C_CUSTOMER_SK   =   SS_CUSTOMER_SK
> GROUP BY
>   SS_CUSTOMER_SK
> , C_FIRST_NAME
> , C_LAST_NAME
> , C_PREFERRED_CUST_FLAG
> , C_BIRTH_COUNTRY
> , C_LOGIN
> , C_EMAIL_ADDRESS
> ;
> C_CUSTOMER_SK here is key so ideally we should remove all columns from 
> group by except SS_CUSTOMER_EX and C_FISRT_NAME but getExpressionLineage 
> returs only STOERS_SALES as ref for SS_CUSTOMER_SK column. 
> I looked at the RelMdExpressionLineage logic for join and it doesn't look 
> like it take join condition into account while determining lineage.

You are right, it will need additional logic :( We can create a follow-up for 
that.


- Jesús


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


On Nov. 7, 2018, 7:39 p.m., Vineet Garg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69202/
> ---
> 
> (Updated Nov. 7, 2018, 7:39 p.m.)
> 
> 
> Review request for hive and Jesús Camacho Rodríguez.
> 
> 
> Bugs: HIVE-20804
> https://issues.apache.org/jira/browse/HIVE-20804
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> See Jira
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelOptUtil.java 
> 9aa30129b6 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
>  b7c31bdfca 
>   ql/src/test/queries/clientpositive/constraints_optimization.q 70ab8509c5 
>   ql/src/test/results/clientpositive/llap/constraints_optimization.q.out 
> 96caa4d6dd 
> 
> 
> Diff: https://reviews.apache.org/r/69202/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vineet Garg
> 
>

Re: Review Request 69202: HIVE-20804 Further improvements to group by optimization with constraints

[Jesús Camacho Rodríguez]

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


Ship it!




Ship It!

- Jesús Camacho Rodríguez


On Nov. 7, 2018, 7:39 p.m., Vineet Garg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69202/
> ---
> 
> (Updated Nov. 7, 2018, 7:39 p.m.)
> 
> 
> Review request for hive and Jesús Camacho Rodríguez.
> 
> 
> Bugs: HIVE-20804
> https://issues.apache.org/jira/browse/HIVE-20804
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> See Jira
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelOptUtil.java 
> 9aa30129b6 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
>  b7c31bdfca 
>   ql/src/test/queries/clientpositive/constraints_optimization.q 70ab8509c5 
>   ql/src/test/results/clientpositive/llap/constraints_optimization.q.out 
> 96caa4d6dd 
> 
> 
> Diff: https://reviews.apache.org/r/69202/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vineet Garg
> 
>

Re: Review Request 69202: HIVE-20804 Further improvements to group by optimization with constraints

[Vineet Garg]

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

(Updated Nov. 7, 2018, 7:39 p.m.)


Review request for hive and Jesús Camacho Rodríguez.


Bugs: HIVE-20804
https://issues.apache.org/jira/browse/HIVE-20804


Repository: hive-git


Description
---

See Jira


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelOptUtil.java 
9aa30129b6 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
 b7c31bdfca 
  ql/src/test/queries/clientpositive/constraints_optimization.q 70ab8509c5 
  ql/src/test/results/clientpositive/llap/constraints_optimization.q.out 
96caa4d6dd 


Diff: https://reviews.apache.org/r/69202/diff/5/

Changes: https://reviews.apache.org/r/69202/diff/4-5/


Testing
---


Thanks,

Vineet Garg

Re: Review Request 69202: HIVE-20804 Further improvements to group by optimization with constraints

[Vineet Garg]

> On Nov. 7, 2018, 2:09 a.m., Jesús Camacho Rodríguez wrote:
> > ql/src/test/queries/clientpositive/constraints_optimization.q
> > Lines 355 (patched)
> > 
> >
> > Can we add two more tests:
> > - One with column swapping before GroupBy (probably if you use group by 
> > b,c,a and table contains a,b,c, it should work and add the Project in 
> > between the TS and the GroupBy).
> > - One with a join and a group by on one column for other table that is 
> > also the join key of the table where all columns are coming from (as in the 
> > whiteboard).

I have added the first test, but the one with join doesn't work
e.g. 
-- transitive equivalence on pk column, therefore all other columns shoule be 
removed
EXPLAIN CBO
SELECT
C_FIRST_NAME
FROM
CUSTOMER
,   STORE_SALES
WHERE
C_CUSTOMER_SK   =   SS_CUSTOMER_SK
GROUP BY
SS_CUSTOMER_SK
,   C_FIRST_NAME
,   C_LAST_NAME
,   C_PREFERRED_CUST_FLAG
,   C_BIRTH_COUNTRY
,   C_LOGIN
,   C_EMAIL_ADDRESS
;
C_CUSTOMER_SK here is key so ideally we should remove all columns from group by 
except SS_CUSTOMER_EX and C_FISRT_NAME but getExpressionLineage returs only 
STOERS_SALES as ref for SS_CUSTOMER_SK column. 
I looked at the RelMdExpressionLineage logic for join and it doesn't look like 
it take join condition into account while determining lineage.


- Vineet


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


On Nov. 7, 2018, 1:49 a.m., Vineet Garg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69202/
> ---
> 
> (Updated Nov. 7, 2018, 1:49 a.m.)
> 
> 
> Review request for hive and Jesús Camacho Rodríguez.
> 
> 
> Bugs: HIVE-20804
> https://issues.apache.org/jira/browse/HIVE-20804
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> See Jira
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelOptUtil.java 
> 9aa30129b6 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
>  b7c31bdfca 
>   ql/src/test/queries/clientpositive/constraints_optimization.q 70ab8509c5 
>   ql/src/test/results/clientpositive/llap/constraints_optimization.q.out 
> 96caa4d6dd 
> 
> 
> Diff: https://reviews.apache.org/r/69202/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vineet Garg
> 
>

Re: Review Request 69202: HIVE-20804 Further improvements to group by optimization with constraints

[Jesús Camacho Rodríguez]

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




ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
Lines 350 (patched)


You do not need a list anymore, you can use the bitset and it is faster.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
Lines 351 (patched)


Same as above.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
Lines 356 (patched)


This would become originalGroupSet.nth(i)



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
Lines 357 (patched)


Instead of using contains here, you can use _get(int bitIndex)_.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
Lines 358 (patched)


This would be currentKey.get(backtrackedCol)



ql/src/test/queries/clientpositive/constraints_optimization.q
Lines 355 (patched)


Can we add two more tests:
- One with column swapping before GroupBy (probably if you use group by 
b,c,a and table contains a,b,c, it should work and add the Project in between 
the TS and the GroupBy).
- One with a join and a group by on one column for other table that is also 
the join key of the table where all columns are coming from (as in the 
whiteboard).


- Jesús Camacho Rodríguez


On Nov. 7, 2018, 1:49 a.m., Vineet Garg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69202/
> ---
> 
> (Updated Nov. 7, 2018, 1:49 a.m.)
> 
> 
> Review request for hive and Jesús Camacho Rodríguez.
> 
> 
> Bugs: HIVE-20804
> https://issues.apache.org/jira/browse/HIVE-20804
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> See Jira
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelOptUtil.java 
> 9aa30129b6 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
>  b7c31bdfca 
>   ql/src/test/queries/clientpositive/constraints_optimization.q 70ab8509c5 
>   ql/src/test/results/clientpositive/llap/constraints_optimization.q.out 
> 96caa4d6dd 
> 
> 
> Diff: https://reviews.apache.org/r/69202/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vineet Garg
> 
>

Re: Review Request 69202: HIVE-20804 Further improvements to group by optimization with constraints

[Vineet Garg]

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

(Updated Nov. 7, 2018, 1:49 a.m.)


Review request for hive and Jesús Camacho Rodríguez.


Bugs: HIVE-20804
https://issues.apache.org/jira/browse/HIVE-20804


Repository: hive-git


Description
---

See Jira


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelOptUtil.java 
9aa30129b6 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
 b7c31bdfca 
  ql/src/test/queries/clientpositive/constraints_optimization.q 70ab8509c5 
  ql/src/test/results/clientpositive/llap/constraints_optimization.q.out 
96caa4d6dd 


Diff: https://reviews.apache.org/r/69202/diff/4/

Changes: https://reviews.apache.org/r/69202/diff/3-4/


Testing
---


Thanks,

Vineet Garg

Re: Review Request 69202: HIVE-20804 Further improvements to group by optimization with constraints

[Jesús Camacho Rodríguez]

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




ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelOptUtil.java
Lines 772 (patched)


Can we use the RelMdExpressionLineage instead? This will not handle the 
join case when you group by the column that is key of the join but belongs to 
other input. Using RelMdExpressionLineage should be trivial, rest of logic 
remains the same.


- Jesús Camacho Rodríguez


On Nov. 7, 2018, midnight, Vineet Garg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69202/
> ---
> 
> (Updated Nov. 7, 2018, midnight)
> 
> 
> Review request for hive and Jesús Camacho Rodríguez.
> 
> 
> Bugs: HIVE-20804
> https://issues.apache.org/jira/browse/HIVE-20804
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> See Jira
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelOptUtil.java 
> 9aa30129b6 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
>  b7c31bdfca 
>   ql/src/test/queries/clientpositive/constraints_optimization.q 70ab8509c5 
>   ql/src/test/results/clientpositive/llap/constraints_optimization.q.out 
> 96caa4d6dd 
> 
> 
> Diff: https://reviews.apache.org/r/69202/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vineet Garg
> 
>

Re: Review Request 69202: HIVE-20804 Further improvements to group by optimization with constraints

[Vineet Garg]

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

(Updated Nov. 7, 2018, midnight)


Review request for hive and Jesús Camacho Rodríguez.


Changes
---

Rewrote the logic to backtrack column set


Bugs: HIVE-20804
https://issues.apache.org/jira/browse/HIVE-20804


Repository: hive-git


Description
---

See Jira


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelOptUtil.java 
9aa30129b6 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
 b7c31bdfca 
  ql/src/test/queries/clientpositive/constraints_optimization.q 70ab8509c5 
  ql/src/test/results/clientpositive/llap/constraints_optimization.q.out 
96caa4d6dd 


Diff: https://reviews.apache.org/r/69202/diff/3/

Changes: https://reviews.apache.org/r/69202/diff/2-3/


Testing
---


Thanks,

Vineet Garg

Re: Review Request 69202: HIVE-20804 Further improvements to group by optimization with constraints

[Vineet Garg]

> On Nov. 1, 2018, 8:33 p.m., Jesús Camacho Rodríguez wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelOptUtil.java
> > Lines 774 (patched)
> > 
> >
> > Cardinality and NDV may change. This just checks whether column 
> > origin/lineage can be backtrack to single input. We should use 
> > RelMdColumnOrigins or RelMdExpressionLineage instead of writing this new 
> > class.

The reason I decided not to use RelMdColumnOrigins or RelMdExpressionLineage is 
because both of them operate on single column/rex node. To get to column origin 
for a set of columns/RexRefs we will need to call them repeatadely. I feel this 
is big overkill and therefore wrote another class to take on set of columns.

I agree about the name being misleading. I'll rename the class to better 
communicate its purpose.


> On Nov. 1, 2018, 8:33 p.m., Jesús Camacho Rodríguez wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
> > Lines 346 (patched)
> > 
> >
> > This should never be empty?

It could be empty if 'CardinalityChange' couldn't backtrack. However if can 
never be null.


> On Nov. 1, 2018, 8:33 p.m., Jesús Camacho Rodríguez wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
> > Lines 359 (patched)
> > 
> >
> > This logic does not seem correct. What happens when a Project permutes 
> > input references and the GBy columns would not be in same order as the key? 
> > For instance consider GBy key is (0,1,2) which maps into columns (2,0,4) in 
> > TS. Key is (0,4). You would remove column 1 from GBy, but you should remove 
> > column 0. Using ColumnOrigins or ExpressionLineage providers to get the 
> > mapping would solve the issue.

The whole for loop actually takes care of this case. CardinalityChange returns 
you backtracked (if it could) columns (2,0,4) in this case. Then we loop over 
this backtracked columns and remove the corresponding column from the GBy key 
(based on the position) if not part of key. For instance in this case first 
column in backtracked columns (i.e. 2) is not part of key therefore 
corresponding column (i.e. 0) will be removed from GBy key.


- Vineet


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


On Nov. 5, 2018, 10:57 p.m., Vineet Garg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69202/
> ---
> 
> (Updated Nov. 5, 2018, 10:57 p.m.)
> 
> 
> Review request for hive and Jesús Camacho Rodríguez.
> 
> 
> Bugs: HIVE-20804
> https://issues.apache.org/jira/browse/HIVE-20804
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> See Jira
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelOptUtil.java 
> 9aa30129b6 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
>  b7c31bdfca 
>   ql/src/test/queries/clientpositive/constraints_optimization.q 70ab8509c5 
>   ql/src/test/results/clientpositive/llap/constraints_optimization.q.out 
> 96caa4d6dd 
> 
> 
> Diff: https://reviews.apache.org/r/69202/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vineet Garg
> 
>

Re: Review Request 69202: HIVE-20804 Further improvements to group by optimization with constraints

[Vineet Garg]

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

(Updated Nov. 5, 2018, 10:57 p.m.)


Review request for hive and Jesús Camacho Rodríguez.


Changes
---

Addressed review comments


Bugs: HIVE-20804
https://issues.apache.org/jira/browse/HIVE-20804


Repository: hive-git


Description
---

See Jira


Diffs (updated)
-

  ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelOptUtil.java 
9aa30129b6 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
 b7c31bdfca 
  ql/src/test/queries/clientpositive/constraints_optimization.q 70ab8509c5 
  ql/src/test/results/clientpositive/llap/constraints_optimization.q.out 
96caa4d6dd 


Diff: https://reviews.apache.org/r/69202/diff/2/

Changes: https://reviews.apache.org/r/69202/diff/1-2/


Testing
---


Thanks,

Vineet Garg

Re: Review Request 69202: HIVE-20804 Further improvements to group by optimization with constraints

[Jesús Camacho Rodríguez]

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




ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelOptUtil.java
Lines 774 (patched)


Cardinality and NDV may change. This just checks whether column 
origin/lineage can be backtrack to single input. We should use 
RelMdColumnOrigins or RelMdExpressionLineage instead of writing this new class.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelOptUtil.java
Lines 809 (patched)


You can use _shift_ method in ImmutableBitSet for these cases.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelOptUtil.java
Lines 823 (patched)


Not used?



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelOptUtil.java
Lines 833 (patched)


colSet.contains(ImmutableBitSet.of(i)) -> colSet.get(i)



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelOptUtil.java
Lines 834 (patched)


This will create a copy in every iteration. You could create a 
ImmutableBitSet.Builder from colset outside of the loop and then clear it here.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
Lines 321 (patched)


Method name should change, tbh it is not about cardinality, but about 
whether the rows can continue being identified uniquely.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
Lines 346 (patched)


This should never be empty?



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
Lines 359 (patched)


This logic does not seem correct. What happens when a Project permutes 
input references and the GBy columns would not be in same order as the key? For 
instance consider GBy key is (0,1,2) which maps into columns (2,0,4) in TS. Key 
is (0,4). You would remove column 1 from GBy, but you should remove column 0. 
Using ColumnOrigins or ExpressionLineage providers to get the mapping would 
solve the issue.


- Jesús Camacho Rodríguez


On Oct. 29, 2018, 6:45 p.m., Vineet Garg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69202/
> ---
> 
> (Updated Oct. 29, 2018, 6:45 p.m.)
> 
> 
> Review request for hive and Jesús Camacho Rodríguez.
> 
> 
> Bugs: HIVE-20804
> https://issues.apache.org/jira/browse/HIVE-20804
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> See Jira
> 
> 
> Diffs
> -
> 
>   ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelOptUtil.java 
> 9aa30129b6 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
>  b7c31bdfca 
>   ql/src/test/queries/clientpositive/constraints_optimization.q 70ab8509c5 
>   ql/src/test/results/clientpositive/llap/constraints_optimization.q.out 
> 96caa4d6dd 
> 
> 
> Diff: https://reviews.apache.org/r/69202/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Vineet Garg
> 
>