Xianqing He 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 7: (11 comments) 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 Done 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 a non null-rejecting predicate into a plan : subtree that is on the nullable side of an outer join. > nit. Suggest to reword to: Done http://gerrit.cloudera.org:8080/#/c/19116/6//COMMIT_MSG@17 PS6, Line 17: GlobalState#outerJoinedTupleIds indicates whether a table is on the : nullable side of an outer join. > nit. Suggest to reword to Done 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: ingTidSet.isEmp > nit. Suggest to rename to nullRejectingTids to make the code more readable. Done http://gerrit.cloudera.org:8080/#/c/19116/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4119 PS6, Line 4119: ch (tableRef.getJoinOp()) { > May need to add a comment for this line of code like: find out all null-rej Done http://gerrit.cloudera.org:8080/#/c/19116/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4120 PS6, Line 4120: se INNER_JOIN: { > If getNonnullableTids(ids, nonnullableTids) at line 4119 does find a subset Done http://gerrit.cloudera.org:8080/#/c/19116/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4166 PS6, Line 4166: removeOuter > nit. We probably should not say "outer joined" here since they are just arg Done http://gerrit.cloudera.org:8080/#/c/19116/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4169 PS6, Line 4169: ); > nit. nullRejectingTids Done http://gerrit.cloudera.org:8080/#/c/19116/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4169 PS6, Line 4169: implifyOuterJoinsB > nit. maybe gatherNullRejectingTids(). Done http://gerrit.cloudera.org:8080/#/c/19116/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@4190 PS6, Line 4190: : /** > nit. The following probably is a more straightforward statement. Done 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 This bug has no effect on 2-way joins because if we convert the 2-way outer join to an inner join the tuple id must be null-rejecting. There have been 2-way join tests at the beginning of this file. -- 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: 7 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-Reviewer: Xianqing He <[email protected]> Gerrit-Comment-Date: Mon, 13 Mar 2023 06:29:46 +0000 Gerrit-HasComments: Yes
