> On June 3, 2019, 7:31 a.m., Jesús Camacho Rodríguez wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HivePlannerContext.java > > Lines 73 (patched) > > <https://reviews.apache.org/r/70717/diff/2/?file=2146900#file2146900line73> > > > > Can we add this to unwrap as for the other objects? That way other > > classes will not be dependent on any specific context implementation.
of course! > On June 3, 2019, 7:31 a.m., Jesús Camacho Rodríguez wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/HiveRelMdRowCount.java > > Lines 152 (patched) > > <https://reviews.apache.org/r/70717/diff/2/?file=2146903#file2146903line152> > > > > 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? I was going into this direction because of perf considerations(as you also sad: "it would seem expensive at first" ), but sure - I can move those things over here ;) actually doing it here for every reltype will be much easier to implement... I've added a HiveRelMdRuntimeRowCount class to separate the runtime related stuff. > On June 3, 2019, 7:31 a.m., Jesús Camacho Rodríguez wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/signature/RelTreeSignature.java > > Lines 52 (patched) > > <https://reviews.apache.org/r/70717/diff/2/?file=2146906#file2146906line52> > > > > 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_. this is something I was about to address ; seems like I forgot to add the my FIXME to address later. as it is now...it's kinda problematic from storage standpoint as well because it will need O(n^2) space to store them ; I'll get back to this or open a separate ticket if it becomes complicated. > On June 3, 2019, 7:31 a.m., Jesús Camacho Rodríguez wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java > > Lines 3478 (patched) > > <https://reviews.apache.org/r/70717/diff/2/?file=2146910#file2146910line3478> > > > > Why do we link the condition instead of the operator? here the linking makes a connection between the AST tree and the Operator produced from it. note that `condn` is an `ASTNode` for which the `FilterOperator` is created > On June 3, 2019, 7:31 a.m., Jesús Camacho Rodríguez wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java > > Lines 3479 (patched) > > <https://reviews.apache.org/r/70717/diff/2/?file=2146910#file2146910line3479> > > > > 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? we can also link other things as well..I think the almost the same is happening here ; but not with a number - but by linking it to the appropriate logic group; right now I don't think that would be simpler/etc. the goal is to make direct or indirect linking between the Calcite Rel nodes and between the Operators. but...I've gone thru your comment say 5-6 times; and I'm not 100% sure that I follow what you mean; so I try to rephrase it what I've understood from your suggestion: * mark every rel with some number * during ast transformation keep that number * during operator transformation keep that number I would think that doing `link(rel, ast)` and `link(ast, op)` we end up with the same result without using some extra thing. I must note that this wasn't really the critical part of the patch...so I've entered this in the early stages; I'll revise how this works - how it could be made more general > On June 3, 2019, 7:31 a.m., Jesús Camacho Rodríguez wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java > > Lines 1013 (patched) > > <https://reviews.apache.org/r/70717/diff/2/?file=2146911#file2146911line1017> > > > > Why does this happen? Can we leave a comment? definetly; this is to mark the TS ops which are doing PPD filtering invalid for Calcite level usage. > On June 3, 2019, 7:31 a.m., Jesús Camacho Rodríguez wrote: > > ql/src/java/org/apache/hadoop/hive/ql/plan/mapper/AuxOpTreeSignature.java > > Lines 37 (patched) > > <https://reviews.apache.org/r/70717/diff/2/?file=2146912#file2146912line37> > > > > Can we add some comments for this class? (Utility, etc.) sure > On June 3, 2019, 7:31 a.m., Jesús Camacho Rodríguez wrote: > > ql/src/test/org/apache/hadoop/hive/ql/plan/mapping/TestReOptimization.java > > Lines 290 (patched) > > <https://reviews.apache.org/r/70717/diff/2/?file=2146925#file2146925line290> > > > > This is commented out? yes; and unfortunately it has to be in case the join order is changed the subTree-s are not matching anymore because of PPD have added some things to the filter condition like `x in RS_33` - Zoltan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70717/#review215561 ----------------------------------------------------------- On June 4, 2019, 7:51 p.m., Zoltan Haindrich wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70717/ > ----------------------------------------------------------- > > (Updated June 4, 2019, 7:51 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 > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 089b88cdb0 > 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/HiveDefaultRelMetadataProvider.java > 0a2714255e > > 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/HiveRelOptUtil.java > 1312561b16 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/HiveRelWriterImpl.java > efc00f9b5b > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/RelWriterImplCopy.java > PRE-CREATION > > 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/stats/HiveRelMdRuntimeRowCount.java > PRE-CREATION > > 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/TestRelSignature.java > PRE-CREATION > > 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/3/ > > > Testing > ------- > > > Thanks, > > Zoltan Haindrich > >
