> On Dec. 7, 2019, 3:03 a.m., Jesús Camacho Rodríguez wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/TopNKeyProcessor.java > > Lines 93 (patched) > > <https://reviews.apache.org/r/71820/diff/6/?file=2181560#file2181560line116> > > > > _replaceChild_?
OperatorFactory.getAndMakeChild adds the newly created operator to parent operator's children operators. At this point we have two children the original children and the newly created TNK op. > On Dec. 7, 2019, 3:03 a.m., Jesús Camacho Rodríguez wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/TopNKeyProcessor.java > > Lines 95 (patched) > > <https://reviews.apache.org/r/71820/diff/6/?file=2181560#file2181560line118> > > > > _replaceParent_? I think calling `child.replaceParent(parents.get(0), newOperator)` has a risk of ArrayIndexOutOfBoundsException. It should not happen but I added ``` child.getParentOperators().clear(); child.getParentOperators().add(newOperator); ``` instead. > On Dec. 7, 2019, 3:03 a.m., Jesús Camacho Rodríguez wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/TopNKeyPushdownProcessor.java > > Lines 18 (patched) > > <https://reviews.apache.org/r/71820/diff/6/?file=2181561#file2181561line18> > > > > Let's move all these classes (TopNKeyPushdownProcessor, > > TopNKeyProcessor, CommonKeyPrefix, etc.) into a new package > > _org.apache.hadoop.hive.ql.optimizer.topnkey_. Thoughts? It makes sense. And we can always change this package in the future if needed. > On Dec. 7, 2019, 3:03 a.m., Jesús Camacho Rodríguez wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/TopNKeyPushdownProcessor.java > > Lines 105 (patched) > > <https://reviews.apache.org/r/71820/diff/6/?file=2181561#file2181561line105> > > > > It seems the logic in this method cannot throw a Semantic Exception? It calls `moveDown` and `pushdown`. Those ones can throw `SemanticException` because they call `Operator.removeChildAndAdoptItsChildren(topNKey)` > On Dec. 7, 2019, 3:03 a.m., Jesús Camacho Rodríguez wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/TopNKeyPushdownProcessor.java > > Lines 278 (patched) > > <https://reviews.apache.org/r/71820/diff/6/?file=2181561#file2181561line278> > > > > Should this method be moved to _CommonKeyPrefix_ too? This is not a factory method of `CommonKeyPrefix`. It used when pushing through LOJ. I beleive it will be used when pushing through ROJ and FOJ so I would like to refactor this in a follow-up patch which implements those scenarios. Anyway I removed `static` because it is used only in this class. - Krisztian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71820/#review218963 ----------------------------------------------------------- On Dec. 9, 2019, 10:21 a.m., Krisztian Kasa wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71820/ > ----------------------------------------------------------- > > (Updated Dec. 9, 2019, 10:21 a.m.) > > > Review request for hive, Jesús Camacho Rodríguez and Zoltan Haindrich. > > > Bugs: HIVE-20150 > https://issues.apache.org/jira/browse/HIVE-20150 > > > Repository: hive-git > > > Description > ------- > > TopNKey pushdown > ================ > 1. Apply patch: > https://issues.apache.org/jira/secure/attachment/12941630/HIVE-20150.11.patch > 2. TopNKey introduction depends only from Reduce Sink with topn property >= 0 > 3. Implement TopNKey operator pushdown through: projection, group by, redeuce > sink, left outer join, other topnkey > 4. Add sort order and null sort order direction check when determining if the > topnkey op can be pushed > 5. Implement handling cases when topnkey op and the parent op has a common > key prefix only. > 6. turn off topnkey optimization by default > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java e7724f9084 > data/conf/perf-reg/tez/hive-site.xml ab945f5f95 > ql/src/java/org/apache/hadoop/hive/ql/optimizer/TopNKeyProcessor.java > 0d6cf3c755 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/topnkey/CommonKeyPrefix.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/topnkey/TopNKeyPushdownProcessor.java > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java bf58bd8bb8 > > ql/src/test/org/apache/hadoop/hive/ql/optimizer/topnkey/TestCommonKeyPrefix.java > PRE-CREATION > ql/src/test/queries/clientpositive/topnkey.q 057b6a45ba > ql/src/test/queries/clientpositive/vector_topnkey.q 85c5880cd6 > ql/src/test/results/clientpositive/llap/bucket_groupby.q.out 0c051c926b > ql/src/test/results/clientpositive/llap/check_constraint.q.out 9f2c9a1cd0 > ql/src/test/results/clientpositive/llap/constraints_optimization.q.out > b6d210becf > ql/src/test/results/clientpositive/llap/enforce_constraint_notnull.q.out > 9343e078b7 > ql/src/test/results/clientpositive/llap/explainuser_1.q.out 283a665a20 > ql/src/test/results/clientpositive/llap/explainuser_2.q.out 0219af8833 > ql/src/test/results/clientpositive/llap/external_jdbc_table_perf.q.out > 545cce75a9 > ql/src/test/results/clientpositive/llap/filter_union.q.out 0df77762a0 > ql/src/test/results/clientpositive/llap/limit_pushdown.q.out 3fdd77d802 > ql/src/test/results/clientpositive/llap/limit_pushdown3.q.out efa8c38d7c > ql/src/test/results/clientpositive/llap/llap_decimal64_reader.q.out > ffe5f6fb22 > ql/src/test/results/clientpositive/llap/offset_limit.q.out 23f2de46e5 > ql/src/test/results/clientpositive/llap/offset_limit_ppd_optimizer.q.out > 4ecb7bc46d > ql/src/test/results/clientpositive/llap/orc_struct_type_vectorization.q.out > 0eac389eb7 > > ql/src/test/results/clientpositive/llap/parquet_complex_types_vectorization.q.out > 4362fb6f2e > > ql/src/test/results/clientpositive/llap/parquet_map_type_vectorization.q.out > 24468c9a1b > > ql/src/test/results/clientpositive/llap/parquet_struct_type_vectorization.q.out > 45890a1890 > ql/src/test/results/clientpositive/llap/semijoin_reddedup.q.out 0e9723b8f3 > ql/src/test/results/clientpositive/llap/subquery_ALL.q.out d910c1a79d > ql/src/test/results/clientpositive/llap/subquery_ANY.q.out 91472d631e > ql/src/test/results/clientpositive/llap/topnkey.q.out 1e77587f82 > ql/src/test/results/clientpositive/llap/vector_cast_constant.q.out > cc2dc47280 > ql/src/test/results/clientpositive/llap/vector_char_2.q.out f7e76e5a8b > > ql/src/test/results/clientpositive/llap/vector_groupby_grouping_sets_limit.q.out > 6fd15e7101 > ql/src/test/results/clientpositive/llap/vector_groupby_reduce.q.out > d6325982e3 > ql/src/test/results/clientpositive/llap/vector_mr_diff_schema_alias.q.out > 4d417b9c3d > ql/src/test/results/clientpositive/llap/vector_reduce_groupby_decimal.q.out > 97a211cfc6 > ql/src/test/results/clientpositive/llap/vector_string_concat.q.out > a8019be7aa > ql/src/test/results/clientpositive/llap/vector_topnkey.q.out c140bdfd37 > ql/src/test/results/clientpositive/llap/vectorization_limit.q.out > 7326adf522 > ql/src/test/results/clientpositive/perf/tez/cbo_query14.q.out e9308cd709 > ql/src/test/results/clientpositive/perf/tez/cbo_query77.q.out 02caf99f7d > ql/src/test/results/clientpositive/perf/tez/constraints/cbo_query14.q.out > 43e1b2b5c2 > ql/src/test/results/clientpositive/perf/tez/constraints/cbo_query77.q.out > 2f75361df1 > ql/src/test/results/clientpositive/perf/tez/constraints/query10.q.out > bb3b1b6660 > ql/src/test/results/clientpositive/perf/tez/constraints/query14.q.out > 228b20a8d7 > ql/src/test/results/clientpositive/perf/tez/constraints/query15.q.out > 5268ed3ecf > ql/src/test/results/clientpositive/perf/tez/constraints/query17.q.out > d96222d9e1 > ql/src/test/results/clientpositive/perf/tez/constraints/query25.q.out > adabb76e04 > ql/src/test/results/clientpositive/perf/tez/constraints/query26.q.out > 824bbe6769 > ql/src/test/results/clientpositive/perf/tez/constraints/query27.q.out > abbd02d6c9 > ql/src/test/results/clientpositive/perf/tez/constraints/query29.q.out > c308771dfb > ql/src/test/results/clientpositive/perf/tez/constraints/query35.q.out > 23b3399123 > ql/src/test/results/clientpositive/perf/tez/constraints/query37.q.out > 187ad5c5b5 > ql/src/test/results/clientpositive/perf/tez/constraints/query40.q.out > 070b5cb1f5 > ql/src/test/results/clientpositive/perf/tez/constraints/query43.q.out > b5a6c746d1 > ql/src/test/results/clientpositive/perf/tez/constraints/query45.q.out > 3f5dbf4beb > ql/src/test/results/clientpositive/perf/tez/constraints/query49.q.out > b384aea779 > ql/src/test/results/clientpositive/perf/tez/constraints/query5.q.out > d3f79820f2 > ql/src/test/results/clientpositive/perf/tez/constraints/query50.q.out > 8c9754967f > ql/src/test/results/clientpositive/perf/tez/constraints/query60.q.out > 06a5689938 > ql/src/test/results/clientpositive/perf/tez/constraints/query66.q.out > be612609cf > ql/src/test/results/clientpositive/perf/tez/constraints/query69.q.out > d7469ae5a9 > ql/src/test/results/clientpositive/perf/tez/constraints/query7.q.out > b2eccdbe90 > ql/src/test/results/clientpositive/perf/tez/constraints/query76.q.out > ce4f7cb061 > ql/src/test/results/clientpositive/perf/tez/constraints/query77.q.out > 95ab61bed2 > ql/src/test/results/clientpositive/perf/tez/constraints/query8.q.out > 170bccf406 > ql/src/test/results/clientpositive/perf/tez/constraints/query80.q.out > b18f89373c > ql/src/test/results/clientpositive/perf/tez/constraints/query82.q.out > 8dd6ae9f0f > ql/src/test/results/clientpositive/perf/tez/constraints/query99.q.out > c77a73f4d5 > ql/src/test/results/clientpositive/perf/tez/query10.q.out b346a5c5fb > ql/src/test/results/clientpositive/perf/tez/query14.q.out 069fad2b4a > ql/src/test/results/clientpositive/perf/tez/query15.q.out 3670a718b3 > ql/src/test/results/clientpositive/perf/tez/query17.q.out df70fbc46e > ql/src/test/results/clientpositive/perf/tez/query25.q.out d006795c79 > ql/src/test/results/clientpositive/perf/tez/query26.q.out a1bf3b099b > ql/src/test/results/clientpositive/perf/tez/query27.q.out 6f49de2344 > ql/src/test/results/clientpositive/perf/tez/query29.q.out 5066893829 > ql/src/test/results/clientpositive/perf/tez/query35.q.out 265c51bb72 > ql/src/test/results/clientpositive/perf/tez/query37.q.out 2724fd44dc > ql/src/test/results/clientpositive/perf/tez/query40.q.out 4b65c82e00 > ql/src/test/results/clientpositive/perf/tez/query43.q.out eb19d41926 > ql/src/test/results/clientpositive/perf/tez/query45.q.out 4538a6540d > ql/src/test/results/clientpositive/perf/tez/query49.q.out 9c34eccceb > ql/src/test/results/clientpositive/perf/tez/query5.q.out 38fba27a8e > ql/src/test/results/clientpositive/perf/tez/query50.q.out 6e34831de6 > ql/src/test/results/clientpositive/perf/tez/query60.q.out e77c89ba69 > ql/src/test/results/clientpositive/perf/tez/query66.q.out 7ddcc21f92 > ql/src/test/results/clientpositive/perf/tez/query69.q.out d11b5494e0 > ql/src/test/results/clientpositive/perf/tez/query7.q.out c17ec8aeb9 > ql/src/test/results/clientpositive/perf/tez/query76.q.out c0d60e88cc > ql/src/test/results/clientpositive/perf/tez/query77.q.out ab2b3dc570 > ql/src/test/results/clientpositive/perf/tez/query8.q.out 0af8fdf3df > ql/src/test/results/clientpositive/perf/tez/query80.q.out 47844158fa > ql/src/test/results/clientpositive/perf/tez/query82.q.out c7721acffe > ql/src/test/results/clientpositive/perf/tez/query99.q.out c01122f435 > ql/src/test/results/clientpositive/tez/topnkey.q.out cf2ecf7133 > ql/src/test/results/clientpositive/tez/vector_topnkey.q.out d179013e28 > ql/src/test/results/clientpositive/topnkey.q.out cecbe89b1c > > > Diff: https://reviews.apache.org/r/71820/diff/7/ > > > Testing > ------- > > Run q tests using TestMiniLlapLocalCliDriver > topnkey.q > vector_topnkey.q > > > Thanks, > > Krisztian Kasa > >
