----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70717/#review215561 -----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HivePlannerContext.java Lines 73 (patched) <https://reviews.apache.org/r/70717/#comment302292> Can we add this to unwrap as for the other objects? That way other classes will not be dependent on any specific context implementation. ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveFilter.java Lines 62 (patched) <https://reviews.apache.org/r/70717/#comment302407> Each time that a rule will copy this node, it will assume that the row count will be the same. But this would depend on the condition or the input that are provided in the input parameter. If we keep row count in the node, probably we should do another lookup when we copy a StatEnhancedHiveFilter. See another alternative in comment below. ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/HiveRelMdRowCount.java Lines 152 (patched) <https://reviews.apache.org/r/70717/#comment302408> Another option is to do the StatsSource check here. This would prevent having to deal with a new Filter implementation that keeps the row count state within. Although it would seem expensive at first, I would assume the Janino cache should mitigate the effect. Also, we will only access these in the replaning phase. What do you think? ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/RelTreeSignature.java Lines 52 (patched) <https://reviews.apache.org/r/70717/#comment302418> Wondering whether this should be JSON too. If I am understanding it right, you are using this signature to store the information too. If that is the case, then we could call _HiveRelOptUtil.toJsonString_. ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java Lines 3478 (patched) <https://reviews.apache.org/r/70717/#comment302415> Why do we link the condition instead of the operator? ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java Lines 3479 (patched) <https://reviews.apache.org/r/70717/#comment302416> Also, what do you think of passing an identifier (unique integer?) when we generate the AST from Calcite plan, e.g., within a hint, so the SemanticAnalyzer can just grab the identifier and establish the relationship. Would that help to link easily the rest of the tree and not only filter? ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java Lines 1013 (patched) <https://reviews.apache.org/r/70717/#comment302417> Why does this happen? Can we leave a comment? ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/AuxOpTreeSignature.java Lines 37 (patched) <https://reviews.apache.org/r/70717/#comment302413> Can we add some comments for this class? (Utility, etc.) ql/src/test/org/apache/hadoop/hive/ql/plan/mapping/TestReOptimization.java Lines 290 (patched) <https://reviews.apache.org/r/70717/#comment302409> This is commented out? - Jesús Camacho Rodríguez On May 28, 2019, 5:17 p.m., Zoltan Haindrich wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70717/ > ----------------------------------------------------------- > > (Updated May 28, 2019, 5:17 p.m.) > > > Review request for hive and Jesús Camacho Rodríguez. > > > Bugs: HIVE-21145 > https://issues.apache.org/jira/browse/HIVE-21145 > > > Repository: hive-git > > > Description > ------- > > . > > > Diffs > ----- > > itests/src/test/resources/testconfiguration.properties 3d8bae8609 > ql/src/java/org/apache/hadoop/hive/ql/exec/Operator.java 1337da2070 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HivePlannerContext.java > a44735e873 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelFactories.java > 5b21bf88e3 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveFilter.java > ea8394c2c5 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/HiveRelMdRowCount.java > 58a74900ef > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/ASTConverter.java > a93d21ac2a > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/OpTreeSignatureFactory.java > 80a3edff61 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/RelTreeSignature.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/RuntimeStatsMap.java > 195a8b18e9 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/SignatureUtils.java > 6c86f9adf1 > ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java 4011d99c92 > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java > a3da075817 > ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java be6c9dcf6f > ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/AuxOpTreeSignature.java > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/CachingStatsSource.java > 6b440e35cf > ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/EmptyStatsSource.java > 624f107ab6 > ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/MapBackedStatsSource.java > fb2b5f8a88 > > ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/MetastoreStatsConnector.java > f9ee46d66a > > ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PersistedRuntimeStats.java > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/PlanMapper.java > e932304cb1 > ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/StatsSource.java > e8d51c91df > ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/StatsSources.java > 823cb87c03 > ql/src/java/org/apache/hadoop/hive/ql/stats/OperatorStats.java 0c56c82879 > > ql/src/test/org/apache/hadoop/hive/ql/optimizer/calcite/TestCBORuleFiredOnlyOnce.java > a6da2647fe > > ql/src/test/org/apache/hadoop/hive/ql/optimizer/signature/TestRuntimeStatsPersistence.java > 627c2d874c > ql/src/test/org/apache/hadoop/hive/ql/plan/mapping/TestOperatorCmp.java > 59cfcd7c6d > ql/src/test/org/apache/hadoop/hive/ql/plan/mapping/TestReOptimization.java > a42b334ac1 > ql/src/test/org/apache/hadoop/hive/ql/plan/mapping/TestStatEstimations.java > e5233ced3f > ql/src/test/queries/clientpositive/retry_failure_reorder.q PRE-CREATION > ql/src/test/queries/clientpositive/runtime_stats_merge.q e69410159a > ql/src/test/results/clientpositive/llap/retry_failure_reorder.q.out > PRE-CREATION > ql/src/test/results/clientpositive/llap/runtime_stats_merge.q.out > e1a747d9af > > > Diff: https://reviews.apache.org/r/70717/diff/2/ > > > Testing > ------- > > > Thanks, > > Zoltan Haindrich > >
