Aman Sinha 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 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/22411/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22411/4//COMMIT_MSG@13 PS4, Line 13: Can you add in the commit message an example query which is benefited by this change ? http://gerrit.cloudera.org:8080/#/c/22411/4/fe/src/main/java/org/apache/impala/analysis/TupleIsNullPredicate.java File fe/src/main/java/org/apache/impala/analysis/TupleIsNullPredicate.java: http://gerrit.cloudera.org:8080/#/c/22411/4/fe/src/main/java/org/apache/impala/analysis/TupleIsNullPredicate.java@216 PS4, Line 216: public static List<TupleIsNullPredicate> getUniqueBoundTupleIsNullPredicates( nit: this is a public method, so would be good to add a short comment. What do 'exprs' and 'tids' refer to in this context ? http://gerrit.cloudera.org:8080/#/c/22411/4/fe/src/main/java/org/apache/impala/analysis/TupleIsNullPredicate.java@222 PS4, Line 222: if (expr != null) { Why would expr be null here ? I would think exprs list is only non-null values. http://gerrit.cloudera.org:8080/#/c/22411/4/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java File fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java: http://gerrit.cloudera.org:8080/#/c/22411/4/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java@105 PS4, Line 105: List<TupleIsNullPredicate> inputTupleIsNullPreds) throws ImpalaException { nit: update the javadoc above with a short description of this new parameter. http://gerrit.cloudera.org:8080/#/c/22411/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java: http://gerrit.cloudera.org:8080/#/c/22411/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@2364 PS4, Line 2364: getInputTupleIsNullPreds Not sure why this says 'Input' in the name. This method is only looking at planNode, not its input. That makes me think .. is this method doing the right thing ? Is planNode already the 'input' that we are interested in ? -- 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: 4 Gerrit-Owner: Steve Carlin <scar...@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Comment-Date: Tue, 18 Feb 2025 06:24:49 +0000 Gerrit-HasComments: Yes
