> On Oct. 14, 2016, 4:17 p.m., Ashutosh Chauhan wrote:
> > ql/src/test/results/clientpositive/limit_pushdown3.q.out, lines 827-836
> > <https://reviews.apache.org/r/52862/diff/1/?file=1537749#file1537749line827>
> >
> >     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
> > <https://reviews.apache.org/r/52862/diff/1/?file=1537756#file1537756line698>
> >
> >     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
> > <https://reviews.apache.org/r/52862/diff/1/?file=1537748#file1537748line698>
> >
> >     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
> > <https://reviews.apache.org/r/52862/diff/1/?file=1537731#file1537731line349>
> >
> >     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
> > <https://reviews.apache.org/r/52862/diff/1/?file=1537731#file1537731line436>
> >
> >     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
> > <https://reviews.apache.org/r/52862/diff/1/?file=1537731#file1537731line767>
> >
> >     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
> > <https://reviews.apache.org/r/52862/diff/1/?file=1537733#file1537733line115>
> >
> >     (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
> > <https://reviews.apache.org/r/52862/diff/1/?file=1537732#file1537732line171>
> >
> >     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 
<time_unit>.

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 <time_unit> argument to Hive, since there is no equivalent. We can 
safely ignore this argument since the information about the <time_unit> 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
> > <https://reviews.apache.org/r/52862/diff/1/?file=1537723#file1537723line202>
> >
> >     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
> > <https://reviews.apache.org/r/52862/diff/1/?file=1537731#file1537731line657>
> >
> >     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 <time_unit> in the input.

I added additional comments to the class.


> On Oct. 14, 2016, 4:17 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelBuilder.java,
> >  line 97
> > <https://reviews.apache.org/r/52862/diff/1/?file=1537700#file1537700line97>
> >
> >     Add comment empty Rel = Rel + limit 0

Added the comment.


> On Oct. 14, 2016, 4:17 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveAggregateJoinTransposeRule.java,
> >  line 136
> > <https://reviews.apache.org/r/52862/diff/1/?file=1537716#file1537716line136>
> >
> >     Should this be based on whether its an outer join or not?

Actually this is a new parameter for the original method; the list should be 
empty. After calling the method, it will be the *list of boolean values for 
each join key position indicating whether the operator filters out nulls or 
not*. The change to the method was added to deal with IS NOT DISTINCT FROM 
(CALCITE-1200). The call to the method is taken from the original 
AggregateJoinTransposeRule.java .


> On Oct. 14, 2016, 4:17 p.m., Ashutosh Chauhan wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/HiveRelMdSelectivity.java,
> >  line 99
> > <https://reviews.apache.org/r/52862/diff/1/?file=1537727#file1537727line99>
> >
> >     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.

I think we cannot get rid of it: it is actually thrown, coming from 
HiveRelOptUtil.splitJoinCondition .


- Jesús


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


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 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
>  62d3ead9852baf263b0e62b0d8e33bfc31659325 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/HiveRelMdPredicates.java
>  e468573841227de0a849d3dad13461fc4c8bb03c 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/HiveRelMdSelectivity.java
>  0d03ebb887c0349d003d510266dfe2510378c610 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/HiveRelMdSize.java
>  31adb41495750901dab1eff64bb63290c702afce 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/HiveRelMdUniqueKeys.java
>  07181507e8566c683e4fade64c7533d6aee85dd0 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ASTBuilder.java
>  9a5becb525fc44a966b4f52ce15d43f80cd5b370 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ASTConverter.java
>  8d738aa754c436f9ae8f61d93fb3b827bc182d2c 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ExprNodeConverter.java
>  46b936acac0811a18a0d910f0ba5c6b1176038c2 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/PlanModifierForASTConv.java
>  9db7727f572eeab6f9372b55a6de2c78a050f9ed 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/RexNodeConverter.java
>  479070b758393b8efeba008b4f9ded2f0d11e783 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/SqlFunctionConverter.java
>  f150132b7b150dafad4733c14ea136f64e55f292 
>   
> ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/TypeConverter.java
>  ba41518d9d7d4c34fb07017b4710ed8a787bd4eb 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java 
> e6ab9473b196f953e91a68ad4a4255f53884b78e 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckProcFactory.java 
> 82080eb44da7ff0bf1a50399933e4ea61b2235bc 
>   
> ql/src/test/org/apache/hadoop/hive/ql/optimizer/calcite/TestCBOMaxNumToCNF.java
>  277ac1e96dc509fda0d7f2aa7cf503f10726cd30 
>   
> ql/src/test/org/apache/hadoop/hive/ql/optimizer/calcite/TestCBORuleFiredOnlyOnce.java
>  2830f1f30992788eafd6f74a673311bf6f4240e7 
>   ql/src/test/results/clientpositive/druid_basic2.q.out 
> 32059054ab862e30739fcb9af7f0634d5ccb8e81 
>   ql/src/test/results/clientpositive/druid_intervals.q.out 
> 984bb7964a1b34a136e3a92978d297b74f6c1e7e 
>   ql/src/test/results/clientpositive/druid_timeseries.q.out 
> 8d974a4259fe3917e73101fdd41b062b1aa2dff0 
>   ql/src/test/results/clientpositive/druid_topn.q.out 
> 17bdaed09d39d132dd7d1803ef6442b73e63ab13 
>   ql/src/test/results/clientpositive/explain_logical.q.out 
> 4a25a38585e3fa29619dece56593ea058cb78066 
>   ql/src/test/results/clientpositive/groupby_sort_1_23.q.out 
> e70f912fdbe676cc0d37f268a3621bc78db90f7e 
>   ql/src/test/results/clientpositive/groupby_sort_skew_1_23.q.out 
> fc5298452bd29b5bb2de96344654bb2254fc07f0 
>   ql/src/test/results/clientpositive/limit_pushdown.q.out 
> 6aaf9b8c3e98d76d5e64f2843089d33d03ae9488 
>   ql/src/test/results/clientpositive/limit_pushdown3.q.out 
> 8ccda6a9ed9c332c544e17d6c7c5df654acf2857 
>   ql/src/test/results/clientpositive/llap/explainuser_4.q.out 
> 4ea14888afeb83824fe97667adf5d59429d02d6b 
>   ql/src/test/results/clientpositive/llap/limit_pushdown.q.out 
> 3fe4837b10403defc7ecc999e5fbcccc1382e43f 
>   ql/src/test/results/clientpositive/llap/lineage3.q.out 
> 257c547dd1da7368e00e6d4625a37814969b64bd 
>   ql/src/test/results/clientpositive/llap/table_access_keys_stats.q.out 
> 91bdff37ef30760019dfe3f1bc444b2dad06bfff 
>   ql/src/test/results/clientpositive/llap/tez_dynpart_hashjoin_1.q.out 
> 3c6ef9adce8bd0d1cb9f9f57b63ed23e8c872897 
>   ql/src/test/results/clientpositive/llap/tez_vector_dynpart_hashjoin_1.q.out 
> c3aebc7f6f775118c3ae1ac0dcd49250baa83284 
>   ql/src/test/results/clientpositive/offset_limit_ppd_optimizer.q.out 
> 14cde7828dc92972f69bdf53b2f31df28b51e00b 
>   ql/src/test/results/clientpositive/perf/query75.q.out 
> 0c722486c98c70c68208d7d14197d50c4c6bf54d 
>   ql/src/test/results/clientpositive/spark/groupby_sort_1_23.q.out 
> c6a79828710085b2936b57a03133b0e0c5b995d7 
>   ql/src/test/results/clientpositive/spark/groupby_sort_skew_1_23.q.out 
> a43812486f49aee89fa2d5d608e9e77e7e2aa4ab 
>   ql/src/test/results/clientpositive/spark/limit_pushdown.q.out 
> 67c6e70ca6d1af8177e1e618664089cefbe92a08 
>   ql/src/test/results/clientpositive/spark/table_access_keys_stats.q.out 
> e26ccecd4eacafd144dedf7f2b9e6b2b16babba0 
>   ql/src/test/results/clientpositive/tez/explainanalyze_4.q.out 
> 3426d192ab4c6b46a74c0ff32e987441fe376742 
>   
> ql/src/test/results/clientpositive/tez/partition_column_names_with_leading_and_trailing_spaces.q.out
>  92fdbe1edca4a4cf3b2f28c319bc1bc8a1a7c2d8 
> 
> Diff: https://reviews.apache.org/r/52862/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jesús Camacho Rodríguez
> 
>

Reply via email to