Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/15047 )
Change subject: IMPALA-8361: Propagate predicates of outer-joined InlineView ...................................................................... Patch Set 9: (7 comments) http://gerrit.cloudera.org:8080/#/c/15047/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15047/9//COMMIT_MSG@24 PS9, Line 24: , nit: need space http://gerrit.cloudera.org:8080/#/c/15047/9//COMMIT_MSG@24 PS9, Line 24: the predicates that : must be evaluted at a join node but can also be safely evaluted by the : outer-joined inline view. I think you mean "some predicates that must be evaluted at a join node can also be safely evaluted by the outer-joined inline view". http://gerrit.cloudera.org:8080/#/c/15047/9//COMMIT_MSG@30 PS9, Line 30: . nit: need space http://gerrit.cloudera.org:8080/#/c/15047/9/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java: http://gerrit.cloudera.org:8080/#/c/15047/9/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@985 PS9, Line 985: picked up by getBoundPredicates() I think this comment is stale after merging this patch. Could you update it to "picked up by getBoundPredicates() and migrateConjunctsToInlineView()"? http://gerrit.cloudera.org:8080/#/c/15047/9/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/15047/9/testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test@2368 PS9, Line 2368: predicates: rand() = 12 This looks strange to me. rand() returns a random value between 0 and 1 so "rand() = 12" will always be false. All rows should be rejected by the WHERE clause. If "rand() = 12" is evaluated in only one side, the other side can still produce rows. So the outer join will still have results. However, looks like the original planner has the same plan. Could you create a JIRA for this? I think it's a bug. It's worth to mention it in the above comments. http://gerrit.cloudera.org:8080/#/c/15047/9/testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test@2402 PS9, Line 2402: , nit: put the space after the comma http://gerrit.cloudera.org:8080/#/c/15047/9/testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test@2442 PS9, Line 2442: upper(b.string_col) Should we move the function inside the view? This can be propagated without this patch. Maybe change it to SELECT * FROM functional.alltypestiny a LEFT JOIN (SELECT upper(b.string_col) as string_col, b.id FROM functional.alltypestiny a LEFT JOIN functional.alltypestiny b ON a.id=b.id) b ON a.id=b.id WHERE b.string_col='1'; -- To view, visit http://gerrit.cloudera.org:8080/15047 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6c23a45aeb5dd1aa06a95c9aa8628ecbe37ef2c1 Gerrit-Change-Number: 15047 Gerrit-PatchSet: 9 Gerrit-Owner: Xianqing He <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Xianqing He <[email protected]> Gerrit-Comment-Date: Tue, 04 Feb 2020 03:49:33 +0000 Gerrit-HasComments: Yes
