Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22411 )
Change subject: IMPALA-13716: Calcite Planner: TupleIsNullPredicate fix for analytic functions ...................................................................... Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/22411/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAnalyticRel.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAnalyticRel.java: http://gerrit.cloudera.org:8080/#/c/22411/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaAnalyticRel.java@152 PS5, Line 152: tupleIsNullPreds); > This appears to be needed because the planNode_ does not specify an outputS Ok, so that is a really good question. My understanding of the substitution map class is that it is something that is used to communicate replacement of expressions that happen on a lower PlanNode that need to get propagated to an parent PlanNode. My general attitude towards this is that since all of these relationships should be handled through the logical plan, there should be no need for expression substitution maps. So I'd really like to avoid the use of them if possible. This specific issue was fixed fairly recently within Impala. It had the feeling to me as more of a "one-off" solution that handled a specific case with the AnalyticPlanner over an outer join. Now going back to your question: Are there other problems? At this point, I haven't seen any and I've run through quite a bit of tests. I can't definitively say that there are no more problems. But this issue seemed solvable without using substitution maps and didn't really cause a coding issue. But having said that, I am willing to revisit the use of expression substitution maps if we see more problems and the code starts getting too difficult without them. -- To view, visit http://gerrit.cloudera.org:8080/22411 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaec363c2fa93a1e21bf74a40e5399e21ddd9bd60 Gerrit-Change-Number: 22411 Gerrit-PatchSet: 5 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Thu, 20 Feb 2025 00:33:06 +0000 Gerrit-HasComments: Yes
