----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68310/#review208467 -----------------------------------------------------------
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java Lines 2219 (patched) <https://reviews.apache.org/r/68310/#comment292640> Should we control all constraint related optimization with one flag and therefore rename this one to e.g. hive.optimize.constraints.rules or something? Or do you think it is better for each rule to have seperate conf? ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveJoinConstraintsRule.java Lines 115 (patched) <https://reviews.apache.org/r/68310/#comment292629> Just a minor comment about readability. leftFK, rightFK name gets a bit confusing may be comment to describe what it represents e.g. they represents corresponding left, right input which is potential FK ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveJoinConstraintsRule.java Lines 117 (patched) <https://reviews.apache.org/r/68310/#comment292639> Not sure what this logic is trying to do. Can you explain? Both of the inputs are being referenced above so how can we still proceed? ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveJoinConstraintsRule.java Lines 203 (patched) <https://reviews.apache.org/r/68310/#comment292630> I didn't understand this part. Irrespective of what input side is being referenced we assume leftInput to be FK. Lets say rightFK is true i.e. right side input is being referenced then wouldn't assuing leftInput as FK side cause issues because we will end up removing right side (PK side) when there are references to it ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveJoinConstraintsRule.java Lines 207 (patched) <https://reviews.apache.org/r/68310/#comment292631> same as above ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveJoinConstraintsRule.java Lines 215 (patched) <https://reviews.apache.org/r/68310/#comment292632> This whole step 2 can be encapsulated in a utility method. I can see this being quite useful for other constraint optimizations ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveJoinConstraintsRule.java Lines 253 (patched) <https://reviews.apache.org/r/68310/#comment292633> Order by should be safe here as well? (assuming it is represented by seperate rel node) ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveJoinConstraintsRule.java Lines 261 (patched) <https://reviews.apache.org/r/68310/#comment292634> Did't understand this step. Irrespective of the mode or fk-pk relationship OUTER join is being transformed into INNER? ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveJoinConstraintsRule.java Lines 329 (patched) <https://reviews.apache.org/r/68310/#comment292517> isn't calling mq.getTableReferences(rightInput) straightforward and cheaper rather than calling it for join and leftInput and taking the difference? ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveJoinConstraintsRule.java Lines 373 (patched) <https://reviews.apache.org/r/68310/#comment292560> Why remove when size is 1? Shouldn't it removed when size is 0? ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java Lines 1661 (patched) <https://reviews.apache.org/r/68310/#comment292391> A comment would be nice to comment what optimizations hive currently does for REFERENTIAL_CONSTRAINTS, ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java Lines 2059 (patched) <https://reviews.apache.org/r/68310/#comment292392> Why is HiveProjectJoinTransposeRule required only for JoinConstraintRule? Sounds like it could be beneficial in general. ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java Lines 2877 (patched) <https://reviews.apache.org/r/68310/#comment292390> Rather than adding 'REFERENTIAL_CONSTRAINTS' profilesCBO separately, may be add it once for both DRUID and JDBC type of tables (i.e. in parent IF condition) Edit: It looks like it is being added for all kind of tables, so many be just add it once (at the end) instead of adding it for each type of table. ql/src/test/results/clientpositive/druid/druidmini_mv.q.out Line 169 (original), 169 (patched) <https://reviews.apache.org/r/68310/#comment292644> Before it was virtual column vc now instead if is column c? What caused this change and is it correct? ql/src/test/results/clientpositive/list_bucket_dml_2.q.out Line 364 (original), 364 (patched) <https://reviews.apache.org/r/68310/#comment292642> Curious what caused this change? I don't see anything relevant. - Vineet Garg On Sept. 5, 2018, 9:11 p.m., Jesús Camacho Rodríguez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68310/ > ----------------------------------------------------------- > > (Updated Sept. 5, 2018, 9:11 p.m.) > > > Review request for hive and Ashutosh Chauhan. > > > Bugs: HIVE-17040 > https://issues.apache.org/jira/browse/HIVE-17040 > > > Repository: hive-git > > > Description > ------- > > HIVE-17040 > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java > 40ea3ac0c5cd0943a4e9dbe2b0e8b952070a8a67 > itests/src/test/resources/testconfiguration.properties > a3a70ecd498bbfc6146cfbfbeb1f265032f440ef > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveJoinConstraintsRule.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveProjectJoinTransposeRule.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveProjectMergeRule.java > 07518df9ec1cea1c331846bbe636cf1a039e762f > ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java > df40a2878d328b76f7de74e2c2db0a029ba4610f > ql/src/test/queries/clientpositive/join_constraints_optimization.q > PRE-CREATION > ql/src/test/queries/clientpositive/materialized_view_rewrite_1.q > ee4844277e5dd1d9ea3911ad2e33a9c3f2481344 > ql/src/test/queries/clientpositive/materialized_view_rewrite_3.q > 4aadd5fb0a4bc74c5bc4088b3d4e62c81bf9cf8b > ql/src/test/queries/clientpositive/materialized_view_rewrite_4.q > dc20b68ba9a9e0bbcb9c414b10b00f3228fe63fe > ql/src/test/queries/clientpositive/materialized_view_rewrite_5.q > 0e4fdf49ac04935f6f14c2ceaa0969008b34f926 > ql/src/test/queries/clientpositive/materialized_view_rewrite_6.q > 4f05f76330cb74be50187e3b175d4675f5ea8763 > ql/src/test/queries/clientpositive/materialized_view_rewrite_7.q > 59ed5757569a8dde70fe04eb9ec5e8c91b5931bf > ql/src/test/queries/clientpositive/materialized_view_rewrite_no_join_opt.q > PRE-CREATION > > ql/src/test/queries/clientpositive/materialized_view_rewrite_no_join_opt_2.q > PRE-CREATION > ql/src/test/queries/clientpositive/materialized_view_rewrite_part_1.q > 5a2e74c8a005ae8422c00a998fa3c07183749176 > ql/src/test/results/clientpositive/ambiguitycheck.q.out > 80c9582fec9754fe56400064ab3f88e3e9ea2da7 > > ql/src/test/results/clientpositive/beeline/materialized_view_create_rewrite.q.out > 7813aac29465b5193789464fcd32771741a98071 > ql/src/test/results/clientpositive/druid/druidmini_mv.q.out > 806262d72e687bbdd09b47380eed77c14764c2a5 > ql/src/test/results/clientpositive/list_bucket_dml_2.q.out > bd8e215c2207c48ce2e446fcc10333ec0fb4648c > ql/src/test/results/clientpositive/list_bucket_dml_4.q.out > 520d48e3d9fa67301852efc9ed1c92494fd528a0 > ql/src/test/results/clientpositive/list_bucket_dml_9.q.out > fbd4fde1bd8d2f7379585afe07c018139b7d64e8 > ql/src/test/results/clientpositive/list_bucket_query_multiskew_1.q.out > e324cab738c22e31089f437d6e7d8a65160dc5b9 > ql/src/test/results/clientpositive/list_bucket_query_multiskew_2.q.out > ec1e54060cb7d380bda4d965fff540d002bd456a > ql/src/test/results/clientpositive/list_bucket_query_multiskew_3.q.out > 889f23c6da7e3c7b955b83d44afd1bd048468b49 > ql/src/test/results/clientpositive/list_bucket_query_oneskew_1.q.out > dcff8a50370b4ebb07300d4a7d9c34aa84f4ae17 > ql/src/test/results/clientpositive/list_bucket_query_oneskew_2.q.out > 268051e2acbbabd02206c5a21690fb563a3dcd2b > ql/src/test/results/clientpositive/llap/acid_bucket_pruning.q.out > 3951b71227d93469347264362492e64554efee32 > ql/src/test/results/clientpositive/llap/bucketpruning1.q.out > 260ba1cbddee7f0946f0cdec1070359ab2a1d2aa > ql/src/test/results/clientpositive/llap/current_date_timestamp.q.out > 6831fb2573788033393544b835f1a56d69fb1712 > ql/src/test/results/clientpositive/llap/join_constraints_optimization.q.out > PRE-CREATION > > ql/src/test/results/clientpositive/llap/materialized_view_create_rewrite.q.out > 71adebb2acad1545c48d45835cd5876434b141b5 > > ql/src/test/results/clientpositive/llap/materialized_view_create_rewrite_dummy.q.out > ce1c281bea0bd2a96e67057ee990fa5b45850905 > > ql/src/test/results/clientpositive/llap/materialized_view_create_rewrite_multi_db.q.out > 98f74379f6275aed90273929c65144e4bbd51a97 > ql/src/test/results/clientpositive/llap/materialized_view_rewrite_1.q.out > 4d8fa52aa94a77faa9d1d1872afcb016e5a3c438 > ql/src/test/results/clientpositive/llap/materialized_view_rewrite_2.q.out > 8e54abe61d48ebd3439ee215e88c2186601a4cd3 > ql/src/test/results/clientpositive/llap/materialized_view_rewrite_3.q.out > d7536e408798d38970a30226f7248d69a10b0903 > ql/src/test/results/clientpositive/llap/materialized_view_rewrite_4.q.out > 3fd4c59ee68c3b0df56d070df900fbea31e3f6aa > ql/src/test/results/clientpositive/llap/materialized_view_rewrite_5.q.out > 9992409f6aa36f6be18f6b07a6de380b2be18a9a > ql/src/test/results/clientpositive/llap/materialized_view_rewrite_6.q.out > 544c395c0125608b3954c7cc9d51ee088d2bdb51 > ql/src/test/results/clientpositive/llap/materialized_view_rewrite_7.q.out > 1e44104ac5d4cb42247b44057c7b649d7d6c1e32 > > ql/src/test/results/clientpositive/llap/materialized_view_rewrite_no_join_opt.q.out > PRE-CREATION > > ql/src/test/results/clientpositive/llap/materialized_view_rewrite_no_join_opt_2.q.out > PRE-CREATION > > ql/src/test/results/clientpositive/llap/materialized_view_rewrite_part_1.q.out > 3a2ad3daf0b5dbbaa4f591a9609059d0ded5300b > > ql/src/test/results/clientpositive/llap/materialized_view_rewrite_part_2.q.out > d39180ecd8294209ffcd3dc44e2571a4f314018a > ql/src/test/results/clientpositive/macro.q.out > 70281acc0b7eb66048903bbdac53baf4b7a2388a > > > Diff: https://reviews.apache.org/r/68310/diff/2/ > > > Testing > ------- > > > Thanks, > > Jesús Camacho Rodríguez > >
