Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14813 )
Change subject: IMPALA-9162: Do not apply inferred predicate to outer joins ...................................................................... Patch Set 4: (7 comments) I'm agree to fix the bug first since it's emergent. Leave some minor commments about the implemenetation. http://gerrit.cloudera.org:8080/#/c/14813/4/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java File fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java: http://gerrit.cloudera.org:8080/#/c/14813/4/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java@333 PS4, Line 333: public static Pair<SlotRef, SlotRef> getEqSlotRefs(Expr e) { This function looks unused. http://gerrit.cloudera.org:8080/#/c/14813/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/14813/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1270 PS4, Line 1270: InlineViewRef viewRef Looks like we just need the Analyzer. http://gerrit.cloudera.org:8080/#/c/14813/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1276 PS4, Line 1276: getSlotRefs Why not using getEqSlots instead since inferred predicates generated in migrateConjunctsToInlineView are all equivalent predicates? http://gerrit.cloudera.org:8080/#/c/14813/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1280 PS4, Line 1280: join nit: joined http://gerrit.cloudera.org:8080/#/c/14813/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1281 PS4, Line 1281: if (viewRef.getAnalyzer().isOuterJoined(leftParent) || : viewRef.getAnalyzer().isOuterJoined(rightParent)) { I think it could be right if only one part is outer joined. But since this is an inferred predicate, I'm agree to be conservative. Could you add a comment about this? http://gerrit.cloudera.org:8080/#/c/14813/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1283 PS4, Line 1283: iter.remove(); Can we log a WARNING here? So we know something is wrong before here causing us infer such a predicate. http://gerrit.cloudera.org:8080/#/c/14813/4/testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test File testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test: http://gerrit.cloudera.org:8080/#/c/14813/4/testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test@2271 PS4, Line 2271: right There are no predicates can be incorrectly assigned to "t". What about changing this to "full outer join"? -- To view, visit http://gerrit.cloudera.org:8080/14813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie9521bd768c4b333069c34d5c1e11b10ea535827 Gerrit-Change-Number: 14813 Gerrit-PatchSet: 4 Gerrit-Owner: Aman Sinha <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 09 Dec 2019 02:14:33 +0000 Gerrit-HasComments: Yes
