> On Sept. 12, 2018, 9:40 p.m., Vineet Garg wrote: > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java > > Lines 2219 (patched) > > <https://reviews.apache.org/r/68310/diff/2/?file=2082619#file2082619line2219> > > > > 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?
I had not given it much thought. We could have different configuration properties, since disabling a rule would only be for debugging purposes and probably better to do it selectively. I changed the name to hive.optimize.constraints.join , so they all use same prefix hive.optimize.constraints . > On Sept. 12, 2018, 9:40 p.m., Vineet Garg wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveJoinConstraintsRule.java > > Lines 115 (patched) > > <https://reviews.apache.org/r/68310/diff/2/?file=2082621#file2082621line115> > > > > 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 Added comment and changed variable names. > On Sept. 12, 2018, 9:40 p.m., Vineet Garg wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveJoinConstraintsRule.java > > Lines 117 (patched) > > <https://reviews.apache.org/r/68310/diff/2/?file=2082621#file2082621line117> > > > > 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? I added a bit of explanation below the condition, I think it should be clearer now. > On Sept. 12, 2018, 9:40 p.m., Vineet Garg wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveJoinConstraintsRule.java > > Lines 203 (patched) > > <https://reviews.apache.org/r/68310/diff/2/?file=2082621#file2082621line203> > > > > 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 This is a left outer join, either we can 1) transform it into inner join if operator on top of it references columns from both sides, or 2) remove it if it only references columns from the left side. However, the case that you describe, i.e., left outer join with operator on top only referencing columns from right input, was not covered indeed! I have changed the logic slightly to take that into account. > On Sept. 12, 2018, 9:40 p.m., Vineet Garg wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveJoinConstraintsRule.java > > Lines 215 (patched) > > <https://reviews.apache.org/r/68310/diff/2/?file=2082621#file2082621line215> > > > > This whole step 2 can be encapsulated in a utility method. I can see > > this being quite useful for other constraint optimizations Done, moved to HiveRelOptUtil. > On Sept. 12, 2018, 9:40 p.m., Vineet Garg wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveJoinConstraintsRule.java > > Lines 253 (patched) > > <https://reviews.apache.org/r/68310/diff/2/?file=2082621#file2082621line253> > > > > Order by should be safe here as well? (assuming it is represented by > > seperate rel node) Correct, I expect it to be less common, but it may happen. I have changed the method accordingly. > On Sept. 12, 2018, 9:40 p.m., Vineet Garg wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveJoinConstraintsRule.java > > Lines 261 (patched) > > <https://reviews.apache.org/r/68310/diff/2/?file=2082621#file2082621line261> > > > > Did't understand this step. Irrespective of the mode or fk-pk > > relationship OUTER join is being transformed into INNER? We transform into INNER to extract the conditions correctly and be able to do the corresponding verifications (we cannot pull conditions from below if it is an outer join). If the join is finally transformed, we use the join that we have already created. Otherwise, we will not use it at all. > On Sept. 12, 2018, 9:40 p.m., Vineet Garg wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveJoinConstraintsRule.java > > Lines 329 (patched) > > <https://reviews.apache.org/r/68310/diff/2/?file=2082621#file2082621line329> > > > > isn't calling mq.getTableReferences(rightInput) straightforward and > > cheaper rather than calling it for join and leftInput and taking the > > difference? The names of the tables are unique, which is important to know the lineage of a predicate in point 5. For instance : A JOIN (B JOIN A). If we extract from right input, we would get B#0, A#0 for unique identifiers for the tables. If we extract from the join itself, we would get A#0, B#0, A#1. > On Sept. 12, 2018, 9:40 p.m., Vineet Garg wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveJoinConstraintsRule.java > > Lines 373 (patched) > > <https://reviews.apache.org/r/68310/diff/2/?file=2082621#file2082621line373> > > > > Why remove when size is 1? Shouldn't it removed when size is 0? This is a map which will contain references to self too. For instance, a = {a, b, c}, b = {a, b, c}, c = {a, b, c}. Hence, we remove it when size is 1. > On Sept. 12, 2018, 9:40 p.m., Vineet Garg wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java > > Lines 1661 (patched) > > <https://reviews.apache.org/r/68310/diff/2/?file=2082624#file2082624line1664> > > > > A comment would be nice to comment what optimizations hive currently > > does for REFERENTIAL_CONSTRAINTS, Not sure this is the right place to comment on that, as it just represents that there are referential constraints defined for the query. > On Sept. 12, 2018, 9:40 p.m., Vineet Garg wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java > > Lines 2059 (patched) > > <https://reviews.apache.org/r/68310/diff/2/?file=2082624#file2082624line2062> > > > > Why is HiveProjectJoinTransposeRule required only for > > JoinConstraintRule? Sounds like it could be beneficial in general. This was not run in that block of rules as it adds overhead and we run RelFieldTrimmer afterwards, which is supposed to prune whatever is not needed. That is why I did not add it unless we enable the join removal rules, for which it is useful. > On Sept. 12, 2018, 9:40 p.m., Vineet Garg wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java > > Lines 2877 (patched) > > <https://reviews.apache.org/r/68310/diff/2/?file=2082624#file2082624line2888> > > > > 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. Done. > On Sept. 12, 2018, 9:40 p.m., Vineet Garg wrote: > > ql/src/test/results/clientpositive/druid/druidmini_mv.q.out > > Line 169 (original), 169 (patched) > > <https://reviews.apache.org/r/68310/diff/2/?file=2082637#file2082637line169> > > > > Before it was virtual column vc now instead if is column c? What caused > > this change and is it correct? It is correct, as the materialized view only has the same filter a=3, and thus a will always be 3. This has to do with the combination of rules in L2121 (8 and 9) and running them in a loop. > On Sept. 12, 2018, 9:40 p.m., Vineet Garg wrote: > > ql/src/test/results/clientpositive/list_bucket_dml_2.q.out > > Line 364 (original), 364 (patched) > > <https://reviews.apache.org/r/68310/diff/2/?file=2082638#file2082638line364> > > > > Curious what caused this change? I don't see anything relevant. This has to do with the combination of rules in L2121 (8 and 9), as name for the columns in the generated SQL from the plan is heavily dependent on order of rules, etc. - Jesús ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68310/#review208467 ----------------------------------------------------------- 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 > >
