> 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 > >
