----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68868/#review209136 -----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/RelOptHiveTable.java Lines 223 (patched) <https://reviews.apache.org/r/68868/#comment293427> Should it be key.contains(columns) ? ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/RelOptHiveTable.java Lines 293 (patched) <https://reviews.apache.org/r/68868/#comment293428> Can we add a comment here? 'They should all be nullable' ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/RelOptHiveTable.java Line 285 (original), 311 (patched) <https://reviews.apache.org/r/68868/#comment293429> Can we either return a Pair, or make it void and that it sets both _keys_ and _nonNullablekeys_? Currently it is a bit weird that one of them is set via return of the method, and the second one from the method. ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java Line 363 (original), 414 (patched) <https://reviews.apache.org/r/68868/#comment293434> return... ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/EstimateUniqueKeys.java Lines 53 (patched) <https://reviews.apache.org/r/68868/#comment293436> This should be a new metadata provider or a modification of the existing _RelMdUniqueKeys_ but introducing a new boolean parameter _acceptEstimatedResults_. However, this would need changes in Calcite side. Please, leave a TODO comment though. ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/EstimateUniqueKeys.java Lines 59 (patched) <https://reviews.apache.org/r/68868/#comment293432> You can make all these methods private and leave a single point of entry to the method (the one that has a RelNode parameter). ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/EstimateUniqueKeys.java Lines 288 (patched) <https://reviews.apache.org/r/68868/#comment293433> We may have Volcano node too, we need to include it. ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/HiveRelMdRowCount.java Line 320 (original), 320 (patched) <https://reviews.apache.org/r/68868/#comment293435> We do not use metadata providers anymore for unique keys estimation, which means some features will be disabled, e.g., caching of metadata results. I am not sure whether this may cause an increase in compilation time for several queries, specially those containing many joins, but it would be worth leaving a comment and maybe monitoring it in future. ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/HiveRelMdRowCount.java Lines 331 (patched) <https://reviews.apache.org/r/68868/#comment293431> Why is this neeeded now? _getUniqueKeys_ did not have a handler for HepRelVertex, hence I would expect this would have returned null keys in any case. Also note that we may use other planners now, hence we may have other special nodes such as the Volcano nodes. - Jesús Camacho Rodríguez On Sept. 28, 2018, 1:23 a.m., Vineet Garg wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68868/ > ----------------------------------------------------------- > > (Updated Sept. 28, 2018, 1:23 a.m.) > > > Review request for hive and Jesús Camacho Rodríguez. > > > Bugs: HIVE-17043 > https://issues.apache.org/jira/browse/HIVE-17043 > > > Repository: hive-git > > > Description > ------- > > This patch implements/test the following optimizations > * Removal of group by on primary keys > * Reduction of group by keys on primary keys > * is NOT NULL filter removal if NOT NULL constraint is defined > > > Diffs > ----- > > itests/src/test/resources/testconfiguration.properties def356176b > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveDefaultRelMetadataProvider.java > 635d27e723 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/RelOptHiveTable.java > 42e60de6a8 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveAggregateJoinTransposeRule.java > f43ef01293 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveRelFieldTrimmer.java > 5857f730a8 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/EstimateUniqueKeys.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/HiveRelMdColumnUniqueness.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/HiveRelMdRowCount.java > 1ca1937ed9 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/HiveRelMdUniqueKeys.java > 3bf62c535c > ql/src/test/queries/clientpositive/constraints_optimization.q PRE-CREATION > ql/src/test/results/clientpositive/llap/constraints_optimization.q.out > PRE-CREATION > > > Diff: https://reviews.apache.org/r/68868/diff/1/ > > > Testing > ------- > > > Thanks, > > Vineet Garg > >
