Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/19116 )
Change subject: IMPALA-11536: fix invalid predicates propagate for outer join simplification ...................................................................... Patch Set 6: (12 comments) Looks good! http://gerrit.cloudera.org:8080/#/c/19116/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19116/6//COMMIT_MSG@11 PS6, Line 11: then nit and then remove http://gerrit.cloudera.org:8080/#/c/19116/6//COMMIT_MSG@14 PS6, Line 14: This may lead to incorrect results since it : is incorrect to propagate predicates into a plan subtree that is on the : nullable side of an outer join if the predicate is not null-rejecting nit. Suggest to reword to: This may lead to incorrect results since it is incorrect to propagate a non null-rejecting predicate into a plan subtree that is on the nullable side of an outer join. http://gerrit.cloudera.org:8080/#/c/19116/6//COMMIT_MSG@17 PS6, Line 17: and determine whether it is a nullable side of an outer join according : to GlobalState#outerJoinedTupleIds nit. Suggest to reword to GlobalState#outerJoinedTupleIds indicates whether a table is on the nullable side of an outer join. http://gerrit.cloudera.org:8080/#/c/19116/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/19116/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4093 PS6, Line 4093: nonnullableTids nit. Suggest to rename to nullRejectingTids to make the code more readable. Maybe also a good idea to add a comment about this input argument. http://gerrit.cloudera.org:8080/#/c/19116/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4119 PS6, Line 4119: getNonnullableTids(ids, nonnullableTids); May need to add a comment for this line of code like: find out all null-rejecting TupleIds in "ids". http://gerrit.cloudera.org:8080/#/c/19116/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4120 PS6, Line 4120: if (TupleId.intersect(ids, nonnullableTids) || hasNullRejectingConjucts(ids)) If getNonnullableTids(ids, nonnullableTids) at line 4119 does find a subset in 'ids' that are null-rejecting, then this line becomes un-necessary. http://gerrit.cloudera.org:8080/#/c/19116/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4166 PS6, Line 4166: outer joined nit. We probably should not say "outer joined" here since they are just arguments. http://gerrit.cloudera.org:8080/#/c/19116/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4169 PS6, Line 4169: nonnullableTids nit. nullRejectingTids http://gerrit.cloudera.org:8080/#/c/19116/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4169 PS6, Line 4169: getNonnullableTids nit. maybe gatherNullRejectingTids(). http://gerrit.cloudera.org:8080/#/c/19116/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4190 PS6, Line 4190: LOG.warn("Skipping to check whether the conjunct is null-rejecting because" : + "backend evaluation failed: " + e.toSql(), ex); nit. The following probably is a more straightforward statement. LOG.warn("Fail to verify " + e.toSql + " being null-rejecting because of the backend evaluation failure", ex); http://gerrit.cloudera.org:8080/#/c/19116/6/testdata/workloads/functional-planner/queries/PlannerTest/outer-to-inner-joins.test File testdata/workloads/functional-planner/queries/PlannerTest/outer-to-inner-joins.test: http://gerrit.cloudera.org:8080/#/c/19116/6/testdata/workloads/functional-planner/queries/PlannerTest/outer-to-inner-joins.test@1030 PS6, Line 1030: FROM functional.jointbl : FULL OUTER JOIN : functional.testtbl : ON jointbl.test_id = testtbl.id : FULL OUTER JOIN These new tests with 3-way joins look good. I wonder if we can also add two more test queries that are 2-way out joins. http://gerrit.cloudera.org:8080/#/c/19116/6/testdata/workloads/functional-planner/queries/PlannerTest/outer-to-inner-joins.test@1065 PS6, Line 1065: correcttly typo. double 't'. -- To view, visit http://gerrit.cloudera.org:8080/19116 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6565c5bff0d2f24f30118ba47a2583383e83fff7 Gerrit-Change-Number: 19116 Gerrit-PatchSet: 6 Gerrit-Owner: Xianqing He <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Comment-Date: Fri, 14 Oct 2022 15:05:56 +0000 Gerrit-HasComments: Yes
