Aman Sinha 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 5: (7 comments) Thanks for the follow-up review. I have addressed your review comments. 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 Pair<SlotRef, SlotRef> getEqSlotRefs() { > This function looks unused. I have removed it in the updated patch and instead check for EQ in the non-static function. 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: Analyzer analyzer, Li > Looks like we just need the Analyzer. Done http://gerrit.cloudera.org:8080/#/c/14813/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1276 PS4, Line 1276: getEqSlotRe > Why not using getEqSlots instead since inferred predicates generated in mig Changed this to use the getEqSlotRefs() (I need the 'SlotRefs', not just the SlotId) 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 Done http://gerrit.cloudera.org:8080/#/c/14813/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1281 PS4, Line 1281: // Note: strictly, we may be ok to check only for the null producing : // side but we are being conservative here to check both si > I think it could be right if only one part is outer joined. But since this Done http://gerrit.cloudera.org:8080/#/c/14813/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1283 PS4, Line 1283: // additional testing we could potentially relax this. > Can we log a WARNING here? So we know something is wrong before here causin Done 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: full > There are no predicates can be incorrectly assigned to "t". What about chan Here 't' is the null producing side, right ? In any case, I changed this to Full Outer Join since that gives broader coverage. -- 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: 5 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 03:18:03 +0000 Gerrit-HasComments: Yes
