----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72234/#review220061 -----------------------------------------------------------
I did a first pass and left few comments. A few important comments: - It seems we are not folding or optimizing properly some of the update/merge queries, e.g., those that contain enforce_constraint. I see pushdown not working or constant folding not being exercise. - Failures in last ptest run seem legit, they will need exploration. ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelFactories.java Lines 213 (patched) <https://reviews.apache.org/r/72234/#comment308336> Why do we need to canonize over here? Can we move this logic to the operator creation method? We can have a new _create_ method without the last parameter, e.g., LogicalSortExchange has _create(RelNode input, RelDistribution distribution, RelCollation collation)_ ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveSortExchange.java Line 57 (original), 62 (patched) <https://reviews.apache.org/r/72234/#comment308335> Should we include the distribution in the collation too? Even if we are focusing on sort right now, it makes sense to set both correctly. ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveSortExchange.java Line 72 (original), 77 (patched) <https://reviews.apache.org/r/72234/#comment308337> Can we remove this method and make keys final? ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveProjectSortExchangeTransposeRule.java Lines 52 (patched) <https://reviews.apache.org/r/72234/#comment308338> nit. Indentation seems too much in this file? ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveProjectSortTransposeRule.java Line 81 (original), 96 (patched) <https://reviews.apache.org/r/72234/#comment308339> Can we move this method to _HiveRelOptUtil_? It seems it belongs there. Please add a comment to it. ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java Lines 900 (patched) <https://reviews.apache.org/r/72234/#comment308340> We should include the distribution positions too. ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveSortPullUpConstantsRuleBase.java Lines 53 (patched) <https://reviews.apache.org/r/72234/#comment308342> Why do we need to use generics here? It seems an interface that is implemented by those RelNode may be a more straightforward option, since most of the logic in the class does not need it. In any case, if you choose to use generics, please pass class T in operands in the constructor instead of a param too to enforce that it is the same matching class. ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java Lines 3976 (patched) <https://reviews.apache.org/r/72234/#comment308343> What is _begin_ and what is _end_ doing? Can we add some comments? ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java Lines 5300 (patched) <https://reviews.apache.org/r/72234/#comment308344> Can we add some comments to this class? ql/src/test/results/clientpositive/auto_join29.q.out Lines 38 (patched) <https://reviews.apache.org/r/72234/#comment308345> This does not produce any results anymore. Could you confirm it is correct? Same for the queries below. ql/src/test/results/clientpositive/ctas_char.q.out Lines 65 (patched) <https://reviews.apache.org/r/72234/#comment308346> Is this correct? Did insert (sort) run with multiple reducers? I would expect we are running with a single reducer. There are also similar changes in files above. Can we verify references/sort spec/plan/exec is correct? ql/src/test/results/clientpositive/input4_limit.q.out Line 50 (original) <https://reviews.apache.org/r/72234/#comment308347> Reducer is gone. Is this expected? ql/src/test/results/clientpositive/llap/check_constraint.q.out Lines 2098 (patched) <https://reviews.apache.org/r/72234/#comment308349> In this case, CBO plan looks worse. Missing pushdown optimization and simplification? ql/src/test/results/clientpositive/llap/check_constraint.q.out Lines 2209 (patched) <https://reviews.apache.org/r/72234/#comment308350> This predicate was not there initially since it is folded to true. We are missing the folding optimization here. Can you check why? ql/src/test/results/clientpositive/sort_acid.q.out Lines 1 (patched) <https://reviews.apache.org/r/72234/#comment308348> Can we add this new test only to MiniLlapLocalCliDriver? - Jesús Camacho Rodríguez On March 23, 2020, 3:30 p.m., Krisztian Kasa wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72234/ > ----------------------------------------------------------- > > (Updated March 23, 2020, 3:30 p.m.) > > > Review request for hive and Jesús Camacho Rodríguez. > > > Bugs: HIVE-22785 > https://issues.apache.org/jira/browse/HIVE-22785 > > > Repository: hive-git > > > Description > ------- > > Update/delete/merge statements not optimized through CBO > > > Diffs > ----- > > > itests/hive-blobstore/src/test/results/clientpositive/map_join_on_filter.q.out > 653faab00a > itests/src/test/resources/testconfiguration.properties f71ed3d240 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelDistribution.java > e5f4c8492e > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelFactories.java > 04b3888a25 > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelJson.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelJsonImpl.java > 0d45eb0c61 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveSortExchange.java > 880cae70f9 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveProjectSortExchangeTransposeRule.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveProjectSortTransposeRule.java > 871c411e70 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java > 53d68e872a > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveSortExchangePullUpConstantsRule.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveSortLimitPullUpConstantsRule.java > e51b2b6ebc > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveSortPullUpConstantsRuleBase.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ASTConverter.java > e03e96ff12 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/PlanModifierForASTConv.java > 31619c0314 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/opconventer/HiveSortExchangeVisitor.java > 68227db1ee > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/opconventer/JoinVisitor.java > 0286d54ea0 > ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java 6589eeb39b > ql/src/java/org/apache/hadoop/hive/ql/parse/RewriteSemanticAnalyzer.java > 31068cb8c3 > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java > 841f92befc > ql/src/test/queries/clientpositive/authorization_view_disable_cbo_1.q > be50b69830 > ql/src/test/queries/clientpositive/sort_acid.q PRE-CREATION > ql/src/test/results/clientpositive/acid_view_delete.q.out 3771c3ba63 > ql/src/test/results/clientpositive/authorization_view_disable_cbo_1.q.out > b609982bb9 > ql/src/test/results/clientpositive/auto_join0.q.out 665cf28dea > ql/src/test/results/clientpositive/auto_join15.q.out 6ea9db28f0 > ql/src/test/results/clientpositive/auto_join20.q.out 6bbcb47084 > ql/src/test/results/clientpositive/auto_join21.q.out b0af07c93e > ql/src/test/results/clientpositive/auto_join23.q.out 9dcfc1aa6f > ql/src/test/results/clientpositive/auto_join28.q.out 6b27398f1e > ql/src/test/results/clientpositive/auto_join29.q.out ade39bdc1a > ql/src/test/results/clientpositive/auto_join31.q.out 73651d36f9 > ql/src/test/results/clientpositive/cbo_rp_auto_join0.q.out 72e041e767 > ql/src/test/results/clientpositive/correlationoptimizer14.q.out b8d764841b > ql/src/test/results/clientpositive/ctas.q.out 23cc8b967e > ql/src/test/results/clientpositive/ctas_char.q.out e0fc050876 > ql/src/test/results/clientpositive/ctas_date.q.out 44f112651c > ql/src/test/results/clientpositive/ctas_varchar.q.out d9a9519fbf > ql/src/test/results/clientpositive/identity_project_remove_skip.q.out > 1176c7c0d4 > ql/src/test/results/clientpositive/input4_limit.q.out 34f4821c7d > ql/src/test/results/clientpositive/input_part7.q.out 15a388e88a > ql/src/test/results/clientpositive/join0.q.out cda0f3f0a5 > ql/src/test/results/clientpositive/join15.q.out 664f2e0614 > ql/src/test/results/clientpositive/join20.q.out 9327cf453a > ql/src/test/results/clientpositive/join21.q.out d25eb5cb09 > ql/src/test/results/clientpositive/join23.q.out 4f8f0d8c14 > ql/src/test/results/clientpositive/join40.q.out b0942ea3e7 > ql/src/test/results/clientpositive/llap/acid_no_buckets.q.out 699398bc9d > ql/src/test/results/clientpositive/llap/acid_vectorization_original.q.out > 2e43110b35 > ql/src/test/results/clientpositive/llap/auto_join0.q.out 31b776fd5d > ql/src/test/results/clientpositive/llap/auto_join21.q.out df866d8ce6 > ql/src/test/results/clientpositive/llap/auto_join29.q.out 9c3a0b1388 > ql/src/test/results/clientpositive/llap/auto_join30.q.out d81dadb799 > ql/src/test/results/clientpositive/llap/check_constraint.q.out b4acc55f15 > ql/src/test/results/clientpositive/llap/ctas.q.out 275bebd3b9 > ql/src/test/results/clientpositive/llap/dynamic_semijoin_reduction_3.q.out > 2522677d95 > > ql/src/test/results/clientpositive/llap/dynpart_sort_optimization_acid.q.out > 57dbbe34c3 > ql/src/test/results/clientpositive/llap/enforce_constraint_notnull.q.out > 74d0190dfe > ql/src/test/results/clientpositive/llap/explainuser_1.q.out b82a055079 > ql/src/test/results/clientpositive/llap/identity_project_remove_skip.q.out > a8a5e38602 > ql/src/test/results/clientpositive/llap/insert_into_default_keyword.q.out > bdf1a65cb1 > ql/src/test/results/clientpositive/llap/join0.q.out 0a81c5bca3 > ql/src/test/results/clientpositive/llap/llap_acid.q.out b8a4058f6c > ql/src/test/results/clientpositive/llap/llap_acid_fast.q.out b91c3fffa7 > ql/src/test/results/clientpositive/llap/orc_predicate_pushdown.q.out > 29deff9eca > ql/src/test/results/clientpositive/llap/parquet_predicate_pushdown.q.out > 3a287bcda8 > ql/src/test/results/clientpositive/llap/runtime_stats_merge.q.out > edcdfb4175 > ql/src/test/results/clientpositive/llap/semijoin.q.out 46ff455ecc > ql/src/test/results/clientpositive/llap/sort_acid.q.out PRE-CREATION > ql/src/test/results/clientpositive/llap/sqlmerge.q.out ad21ef626d > ql/src/test/results/clientpositive/llap/sqlmerge_stats.q.out 9e31c64f83 > ql/src/test/results/clientpositive/llap/vector_acid4.q.out cfe1cc7f7c > ql/src/test/results/clientpositive/llap/vector_join30.q.out 2fa252d942 > ql/src/test/results/clientpositive/llap/vector_leftsemi_mapjoin.q.out > 951f824c30 > ql/src/test/results/clientpositive/llap/vector_mapjoin_complex_values.q.out > ec8b1d9bc8 > ql/src/test/results/clientpositive/llap/vector_partitioned_date_time.q.out > 2c9c7aa8ac > ql/src/test/results/clientpositive/mapjoin_filter_on_outerjoin.q.out > ef7ea4ecf9 > ql/src/test/results/clientpositive/mapjoin_test_outer.q.out dbc87d5f8d > ql/src/test/results/clientpositive/no_hooks.q.out ffff0752fc > ql/src/test/results/clientpositive/parallel_join0.q.out cda0f3f0a5 > ql/src/test/results/clientpositive/pcs.q.out 15c6c212fd > ql/src/test/results/clientpositive/ppd_join4.q.out 67d8e21133 > ql/src/test/results/clientpositive/print_header.q.out 48d7455196 > ql/src/test/results/clientpositive/sort_acid.q.out PRE-CREATION > ql/src/test/results/clientpositive/spark/auto_join0.q.out 300c087686 > ql/src/test/results/clientpositive/spark/auto_join15.q.out 096d9857a0 > ql/src/test/results/clientpositive/spark/auto_join20.q.out 1b5150d419 > ql/src/test/results/clientpositive/spark/auto_join21.q.out 6cd15fd348 > ql/src/test/results/clientpositive/spark/auto_join23.q.out 268eb76741 > ql/src/test/results/clientpositive/spark/auto_join28.q.out fc55988949 > ql/src/test/results/clientpositive/spark/auto_join29.q.out 4bb41d7f10 > ql/src/test/results/clientpositive/spark/auto_join30.q.out 0d3c6df069 > ql/src/test/results/clientpositive/spark/auto_join31.q.out a20b6abb12 > ql/src/test/results/clientpositive/spark/ctas.q.out d6738a2b8f > ql/src/test/results/clientpositive/spark/dynamic_rdd_cache.q.out bef3125869 > ql/src/test/results/clientpositive/spark/identity_project_remove_skip.q.out > 11dcc6d860 > ql/src/test/results/clientpositive/spark/join0.q.out ed11931ba1 > ql/src/test/results/clientpositive/spark/join15.q.out e586f33a74 > ql/src/test/results/clientpositive/spark/join20.q.out 481eef2d40 > ql/src/test/results/clientpositive/spark/join21.q.out 4c117cd4b5 > ql/src/test/results/clientpositive/spark/join23.q.out 6823bdb935 > ql/src/test/results/clientpositive/spark/join40.q.out 3efa147330 > ql/src/test/results/clientpositive/spark/mapjoin_filter_on_outerjoin.q.out > 54f08850de > ql/src/test/results/clientpositive/spark/mapjoin_test_outer.q.out > 7850255f2c > ql/src/test/results/clientpositive/spark/parallel_join0.q.out 3acb2ec941 > ql/src/test/results/clientpositive/spark/ppd_join4.q.out bde0232ac6 > ql/src/test/results/clientpositive/spark/semijoin.q.out 691d0933ab > ql/src/test/results/clientpositive/spark/spark_explainuser_1.q.out > 563a6a684f > ql/src/test/results/clientpositive/spark/union_ppr.q.out 14a63b7e95 > > ql/src/test/results/clientpositive/tez/acid_vectorization_original_tez.q.out > 02b4f6bf32 > ql/src/test/results/clientpositive/tez/explainanalyze_5.q.out 5088a3d155 > ql/src/test/results/clientpositive/union_ppr.q.out 89985d0b1a > > > Diff: https://reviews.apache.org/r/72234/diff/4/ > > > Testing > ------- > > mvn test -Dtest.output.overwrite -DskipSparkTests -Dtest=TestCliDriver > -Dqfile=sort_acid.q -pl itests/qtest -Pitests > > > Thanks, > > Krisztian Kasa > >
