Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/18234 )
Change subject: IMPALA-11008: fix incorrect to propagate inferred predicates ...................................................................... Patch Set 4: (7 comments) Thanks for fixing this subtle bug! http://gerrit.cloudera.org:8080/#/c/18234/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18234/4//COMMIT_MSG@15 PS4, Line 15: select * from (select id is not null and col is null as a from : (select A.id, B.col from A left join B on A.id = B.id) t ) t : where a = 1 nit: could you format this a bit for readability? E.g. select * from ( select id is not null and col is null as a from (select A.id, B.col from A left join B on A.id = B.id) t ) v where a = 1 http://gerrit.cloudera.org:8080/#/c/18234/4//COMMIT_MSG@18 PS4, Line 18: (B.id is not null and : B.col is null = 1) nit: for readability, could you format this a bit? E.g. "(B.id is not null and B.col is null) = 1" http://gerrit.cloudera.org:8080/#/c/18234/4//COMMIT_MSG@20 PS4, Line 20: (A.id is not null and B.col is null = 1) nit: could you format this a bit as above? http://gerrit.cloudera.org:8080/#/c/18234/4/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/18234/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2009 PS4, Line 2009: if (hasOuterJoinedTuple && isTrueWithNullSlots(srcConjunct)) continue; Is it possible to reject the conjunct here? These codes aim to reject conjuncts that can't be pushed down to the nullable side. Unfortunatly, isTrueWithNullSlots() can't cover this case. We probably need a more strict check, ie. only substitute slots belong to this tuple with nulls (instead of substituting all slots). http://gerrit.cloudera.org:8080/#/c/18234/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2043 PS4, Line 2043: // It is incorrect to propagate predicates inferred from equi-join conjuncts : // into a plan subtree that is on the nullable side of an outer join if the : // predicate is not null-filtering conditions for the nullable side. : // For example: : // select * from (select id is not null and col is null as a from (select A.id, : // B.col from A left join B on A.id = B.id) t ) t where a = 1 : // In this query the inferred predicate (B.id is not null and B.col is null = 1) : // should not be evaluated at the scanner of B. Could you also add some comments about "ojsmap". I think we want to substitue the non-outer-join slots first and do some checks on the conjunct before the final substitution. http://gerrit.cloudera.org:8080/#/c/18234/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2647 PS4, Line 2647: registerOjEqualSlots(outerSlot, innerSlot); Should we only do this for outer joins? Now we also register anti join slots. http://gerrit.cloudera.org:8080/#/c/18234/4/testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test File testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test: http://gerrit.cloudera.org:8080/#/c/18234/4/testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test@1617 PS4, Line 1617: | other predicates: CASE WHEN t1.id IS NOT NULL AND t2.some_nulls IS NULL THEN TRUE ELSE NULL END IS NOT NULL Here we are missing a runtime filter, which causes the test failure: https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/15787/testReport/junit/org.apache.impala.planner/PlannerTest/testPredicatePropagation/ -- To view, visit http://gerrit.cloudera.org:8080/18234 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e64230f6d0c2b9ef1560186ceba349a5920ccdf Gerrit-Change-Number: 18234 Gerrit-PatchSet: 4 Gerrit-Owner: Xianqing He <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Thu, 17 Feb 2022 12:43:20 +0000 Gerrit-HasComments: Yes
