Re: Review Request 52862: HIVE-13316: Upgrade to Calcite 1.10

[Jesús Camacho Rodríguez]

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

(Updated Oct. 17, 2016, 10:02 a.m.)


Review request for hive and Ashutosh Chauhan.


Changes
---

Addressing comments


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


Repository: hive-git


Description
---

HIVE-13316: Upgrade to Calcite 1.10


Diffs (updated)
-

  
druid-handler/src/java/org/apache/hadoop/hive/druid/HiveDruidQueryBasedInputFormat.java
 3df14528478def4cf303775ae83829e937c22dc7 
  
druid-handler/src/java/org/apache/hadoop/hive/druid/serde/DruidGroupByQueryRecordReader.java
 226060fcc27fd6993f2e490637d5345e4aeb877d 
  
druid-handler/src/java/org/apache/hadoop/hive/druid/serde/DruidSelectQueryRecordReader.java
 70b493c475f3b1b77092939827e5425295336975 
  druid-handler/src/java/org/apache/hadoop/hive/druid/serde/DruidSerDe.java 
8f53d4a73b49911b26a2b929f63f68699fa8ac82 
  
druid-handler/src/java/org/apache/hadoop/hive/druid/serde/DruidTimeseriesQueryRecordReader.java
 812ae03050e0f0998aaf3f7b6f7669f2db53a56f 
  
druid-handler/src/java/org/apache/hadoop/hive/druid/serde/DruidTopNQueryRecordReader.java
 0b8797644b3b97f42ef1c66984986d645f27a640 
  pom.xml 5d13344e137563efa12f36f63ad2d619cafd4400 
  ql/pom.xml 2a93bb7a56ac10294acd0c9fefbcd9e921577fc7 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveDefaultRelMetadataProvider.java
 c0609d7773a1e49cc85a1d542caa16d74ac76efe 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HivePlannerContext.java 
890aea17682099c290e72fd62aae3eb49b44235e 
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelBuilder.java 
1c64d64dd7e012f6060dfd6b18581d6309647ef8 
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelOptUtil.java 
4c154d056b2a90615d3086f26f52c9a2fef93fde 
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRexUtil.java 
15707c1de821ea2cf7ff1f1b54788f9161f19194 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveTypeSystemImpl.java 
10fdcc6559e6d55caf7519a753fe5aa7a707a60f 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/cost/HiveDefaultCostModel.java
 badb8ca88b5729e138d214569d45d1a30c0b6e36 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/cost/HiveRelMdCost.java 
ed45ab3befec3f0846a61380871181b5ecd0ec8f 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/druid/DruidIntervalUtils.java
 82ab4d74d7acfba0f17aef46d9f72f24458faf9f 
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/druid/DruidQuery.java 
43982aaa278cc854ff8ecc899239915a348c7396 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/druid/DruidQueryType.java
 228b307e9a64ca65361ec61528f34193e37ae338 
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/druid/DruidRules.java 
f68ffa52627c02434ec6a132acbc60034a42f9ce 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/druid/DruidSchema.java 
3b3f68ac5e986abea467d85ce067cd656a68c335 
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/druid/DruidTable.java 
728829116f0a91ade902c132494dd0ccfcd1cae5 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/druid/HiveDruidConf.java
 0686dfffb738617ea493b4115b999796d1e8ca8c 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveAggregate.java
 dc6b152dda01079cc1f84755d9cca510bf3f272c 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveDateGranularity.java
 b3f8d9ba8dc76372f7816b46a6f0a2efb7b3d73b 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveFloorDate.java
 PRE-CREATION 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveAggregateJoinTransposeRule.java
 e9a4d88dd856ced98f806f0b20557dff5cd6cc75 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveAggregateProjectMergeRule.java
 8af8a0d7fd327b05f47818638f56ef2ad8ad51da 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveFilterProjectTSTransposeRule.java
 f81c21b89d70cf1283ee2f026d2c431d5cdc9509 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveFilterProjectTransposeRule.java
 d43c2c66d5e63652d058ffbffa350e8baefa80d4 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePreFilteringRule.java
 7c2a7e5e74c5fc5350f06329e4b1623ffa8135d4 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveReduceExpressionsRule.java
 2fc68ae54318b63cc9b35baee5dca4735bbad1d8 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveReduceExpressionsWithStatsRule.java
 ec488fe4b789ce0e57e1321e51f3779e6042d7b4 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
 b0cb8df6b7651c7cc9a5ce38c04e41b9a463c13e 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/HiveRelMdCollation.java
 18fe6505c37a8f6af5e914201624476aa0688d5c 
  
ql/src/java/org/apache/hadoo

Re: Review Request 52862: HIVE-13316: Upgrade to Calcite 1.10

[Jesús Camacho Rodríguez]

> On Oct. 14, 2016, 4:17 p.m., Ashutosh Chauhan wrote:
> > ql/src/test/results/clientpositive/limit_pushdown3.q.out, lines 827-836
> > 
> >
> > limit 0 optimization not kicking in?

As above.


> On Oct. 14, 2016, 4:17 p.m., Ashutosh Chauhan wrote:
> > ql/src/test/results/clientpositive/offset_limit_ppd_optimizer.q.out, lines 
> > 698-707
> > 
> >
> > limit 0 optimization not kicking in?

As above.


> On Oct. 14, 2016, 4:17 p.m., Ashutosh Chauhan wrote:
> > ql/src/test/results/clientpositive/limit_pushdown.q.out, lines 698-707
> > 
> >
> > limit 0 optimization not kicking in?

Yes, this is a known issue. If plan has no RS, global limit is not set. In this 
case, new version of Calcite removes the sort because there is no need to sort 
if the limit is 0. Thus, the optimization does not kick in. That is mentioned 
in HIVE-14866; we can tackle it there.


> On Oct. 14, 2016, 4:17 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ASTConverter.java,
> >  lines 349-356
> > 
> >
> > Can these two branches be merged as (r.assignableFrom(TableScan)) ?

Original implementation of DruidQuery in Calcite does not extend TableScan. I 
tried to change it but it was not straightforward: it was causing problems with 
the execution in Calcite, if I remember correctly related to schema discovery 
(tests failing, etc). Thus, I decided to not spend more time on it. Maybe I can 
leave a comment and it is something that can be explored for next Calcite 
release...

In this case, we have two branches in the if clause because the Schema 
constructor is different for both classes.


> On Oct. 14, 2016, 4:17 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ASTConverter.java,
> >  lines 436-437
> > 
> >
> > DruidQuery extends from TableScan. So, additional || is not needed.

As above.


> On Oct. 14, 2016, 4:17 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ASTConverter.java,
> >  lines 767-773
> > 
> >
> > Can caller just call Schema(TableScan) for this as well?

As above.


> On Oct. 14, 2016, 4:17 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/PlanModifierForASTConv.java,
> >  lines 115-118
> > 
> >
> > (rel instanceof TableScan) ?

As above.


> On Oct. 14, 2016, 4:17 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ExprNodeConverter.java,
> >  lines 171-178
> > 
> >
> > Add comment why these functions are specially handled here.

The reason is the mismatch between Hive and Calcite for EXTRACT/FLOOR functions 
types, i.e., EXTRACT and FLOOR in Calcite include parameters for the 
.

This logic is called in ReduceExpressions when we need to fold some of the 
expressions (as the Executor is based on ExprNode conversion). Thus, if we do 
not add these special cases, we might end up with an Exception when we try to 
translate a  argument to Hive, since there is no equivalent. We can 
safely ignore this argument since the information about the  is 
already implicit in the function name of EXTRACT/FLOOR (you can check 
_HiveExtractDate_ or _HiveFloorDate_ classes to verify this).

I have added a more detailed comment to the class.


> On Oct. 14, 2016, 4:17 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java,
> >  line 202
> > 
> >
> > Can we merge this function with one on HiveTableScan by overriding 
> > trimFields (TableScan)

See comments below about DruidQuery/TableScan relationship.


> On Oct. 14, 2016, 4:17 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ASTConverter.java,
> >  lines 657-664
> > 
> >
> > Add comment on why these functions need to be specially handled here 
> > and not inside RexVisitior.

See explanation below about ExprNodeConverter. In any case, for this particular 
case, I have just realized that we can remove _convertExtractDate_ and 
_convertFloorDate_ methods and just ignore the  in the input.

I added additional comments to the class.


> O

Re: Review Request 52862: HIVE-13316: Upgrade to Calcite 1.10

[Ashutosh Chauhan]

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




ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelBuilder.java 
(line 97)


Add comment empty Rel = Rel + limit 0



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveAggregateJoinTransposeRule.java
 (line 136)


Should this be based on whether its an outer join or not?



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
 (line 190)


Can we merge this function with one on HiveTableScan by overriding 
trimFields (TableScan)



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/HiveRelMdPredicates.java
 (line 121)


ws



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/HiveRelMdSelectivity.java
 (line 96)


Can just get rid of this checked exception altogether from 
JoinPredicateInfo. Seems like this is just declared in method signatures there, 
but never actually thrown.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/HiveRelMdUniqueKeys.java
 (line 71)


Nice comments!
Also add, why HiveTableScan may not be found here.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ASTConverter.java
 (lines 348 - 355)


Can these two branches be merged as (r.assignableFrom(TableScan)) ?



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ASTConverter.java
 (lines 435 - 436)


DruidQuery extends from TableScan. So, additional || is not needed.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ASTConverter.java
 (lines 656 - 663)


Add comment on why these functions need to be specially handled here and 
not inside RexVisitior.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ASTConverter.java
 (lines 761 - 767)


Can caller just call Schema(TableScan) for this as well?



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ExprNodeConverter.java
 (lines 168 - 175)


Add comment why these functions are specially handled here.



ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/PlanModifierForASTConv.java
 (lines 115 - 118)


(rel instanceof TableScan) ?



ql/src/test/results/clientpositive/limit_pushdown.q.out (lines 698 - 707)


limit 0 optimization not kicking in?



ql/src/test/results/clientpositive/limit_pushdown3.q.out (lines 827 - 836)


limit 0 optimization not kicking in?



ql/src/test/results/clientpositive/offset_limit_ppd_optimizer.q.out (lines 698 
- 707)


limit 0 optimization not kicking in?


- Ashutosh Chauhan


On Oct. 14, 2016, 10:12 a.m., Jesús Camacho Rodríguez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52862/
> ---
> 
> (Updated Oct. 14, 2016, 10:12 a.m.)
> 
> 
> Review request for hive and Ashutosh Chauhan.
> 
> 
> Bugs: HIVE-13316
> https://issues.apache.org/jira/browse/HIVE-13316
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> HIVE-13316: Upgrade to Calcite 1.10
> 
> 
> Diffs
> -
> 
>   
> druid-handler/src/java/org/apache/hadoop/hive/druid/HiveDruidQueryBasedInputFormat.java
>  3df14528478def4cf303775ae83829e937c22dc7 
>   
> druid-handler/src/java/org/apache/hadoop/hive/druid/serde/DruidGroupByQueryRecordReader.java
>  226060fcc27fd6993f2e490637d5345e4aeb877d 
>   
> druid-handler/src/java/org/apache/hadoop/hive/druid/serde/DruidSelectQueryRecordReader.java
>  70b493c475f3b1b77092939827e5425295336975 
>   druid-handler/src/java/org/apache/hadoop/hive/druid/serde/DruidSerDe.java 
> 8f53d4a73b49911b26a2b929f63f68699fa8ac82 
>   
> druid-handler/src/java/org/apache/hadoop/hive/druid/serde/DruidTimeseriesQueryRecordReader.java
>  812ae03050e0f0998aaf3f7b6f7669f2db53a56f 
>   
> druid-handler/src/java/org/apache/hadoop/hive/druid/serde/DruidTopNQueryRecordReader.java
>  0b8797644b3b97f42ef1c66984986d645f27a640 
>   pom.xml 5d13344e137563efa

Review Request 52862: HIVE-13316: Upgrade to Calcite 1.10

[Jesús Camacho Rodríguez]

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

Review request for hive and Ashutosh Chauhan.


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


Repository: hive-git


Description
---

HIVE-13316: Upgrade to Calcite 1.10


Diffs
-

  
druid-handler/src/java/org/apache/hadoop/hive/druid/HiveDruidQueryBasedInputFormat.java
 3df14528478def4cf303775ae83829e937c22dc7 
  
druid-handler/src/java/org/apache/hadoop/hive/druid/serde/DruidGroupByQueryRecordReader.java
 226060fcc27fd6993f2e490637d5345e4aeb877d 
  
druid-handler/src/java/org/apache/hadoop/hive/druid/serde/DruidSelectQueryRecordReader.java
 70b493c475f3b1b77092939827e5425295336975 
  druid-handler/src/java/org/apache/hadoop/hive/druid/serde/DruidSerDe.java 
8f53d4a73b49911b26a2b929f63f68699fa8ac82 
  
druid-handler/src/java/org/apache/hadoop/hive/druid/serde/DruidTimeseriesQueryRecordReader.java
 812ae03050e0f0998aaf3f7b6f7669f2db53a56f 
  
druid-handler/src/java/org/apache/hadoop/hive/druid/serde/DruidTopNQueryRecordReader.java
 0b8797644b3b97f42ef1c66984986d645f27a640 
  pom.xml 5d13344e137563efa12f36f63ad2d619cafd4400 
  ql/pom.xml 2a93bb7a56ac10294acd0c9fefbcd9e921577fc7 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveDefaultRelMetadataProvider.java
 c0609d7773a1e49cc85a1d542caa16d74ac76efe 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HivePlannerContext.java 
890aea17682099c290e72fd62aae3eb49b44235e 
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelBuilder.java 
1c64d64dd7e012f6060dfd6b18581d6309647ef8 
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelOptUtil.java 
4c154d056b2a90615d3086f26f52c9a2fef93fde 
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRexUtil.java 
15707c1de821ea2cf7ff1f1b54788f9161f19194 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveTypeSystemImpl.java 
10fdcc6559e6d55caf7519a753fe5aa7a707a60f 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/cost/HiveDefaultCostModel.java
 badb8ca88b5729e138d214569d45d1a30c0b6e36 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/cost/HiveRelMdCost.java 
ed45ab3befec3f0846a61380871181b5ecd0ec8f 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/druid/DruidIntervalUtils.java
 82ab4d74d7acfba0f17aef46d9f72f24458faf9f 
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/druid/DruidQuery.java 
43982aaa278cc854ff8ecc899239915a348c7396 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/druid/DruidQueryType.java
 228b307e9a64ca65361ec61528f34193e37ae338 
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/druid/DruidRules.java 
f68ffa52627c02434ec6a132acbc60034a42f9ce 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/druid/DruidSchema.java 
3b3f68ac5e986abea467d85ce067cd656a68c335 
  ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/druid/DruidTable.java 
728829116f0a91ade902c132494dd0ccfcd1cae5 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/druid/HiveDruidConf.java
 0686dfffb738617ea493b4115b999796d1e8ca8c 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveAggregate.java
 dc6b152dda01079cc1f84755d9cca510bf3f272c 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveDateGranularity.java
 b3f8d9ba8dc76372f7816b46a6f0a2efb7b3d73b 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveFloorDate.java
 PRE-CREATION 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveAggregateJoinTransposeRule.java
 e9a4d88dd856ced98f806f0b20557dff5cd6cc75 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveAggregateProjectMergeRule.java
 8af8a0d7fd327b05f47818638f56ef2ad8ad51da 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveFilterProjectTSTransposeRule.java
 f81c21b89d70cf1283ee2f026d2c431d5cdc9509 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveFilterProjectTransposeRule.java
 d43c2c66d5e63652d058ffbffa350e8baefa80d4 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePreFilteringRule.java
 7c2a7e5e74c5fc5350f06329e4b1623ffa8135d4 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveReduceExpressionsRule.java
 2fc68ae54318b63cc9b35baee5dca4735bbad1d8 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveReduceExpressionsWithStatsRule.java
 ec488fe4b789ce0e57e1321e51f3779e6042d7b4 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java
 b0cb8df6b7651c7cc9a5ce38c04e41b9a463c13e 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/HiveRelMdCollation.java
 18fe6505c37a8f6af5e914201624476aa0688d5c 
  
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/HiveRelMdDistribution.java
 62d3ead9852baf263b0e62b0d