Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15047 )
Change subject: IMPALA-8361: Propagate predicates of outer-joined InlineView ...................................................................... Patch Set 11: Code-Review+1 (5 comments) Thank you for the patch! The optimisation makes sense to me. I have some minor comments about comments and tests, but otherwise looks good to me. http://gerrit.cloudera.org:8080/#/c/15047/11/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/15047/11/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1588 PS11, Line 1588: * - For On-clause predicates: I think this part of the comment about on-clause predicates should be moved to the commend of "canEvalOnClauseConjunct" http://gerrit.cloudera.org:8080/#/c/15047/11/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1643 PS11, Line 1643: List<TupleId> tids = new ArrayList<>(); Maybe rename this to exprTids or similar, so that it's clearer how it is different from tupleIds. http://gerrit.cloudera.org:8080/#/c/15047/11/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java File fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java: http://gerrit.cloudera.org:8080/#/c/15047/11/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@617 PS11, Line 617: * returned by getResultTupleId(). Document that it only returns unique expressions? http://gerrit.cloudera.org:8080/#/c/15047/11/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/15047/11/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1186 PS11, Line 1186: it's not evaluted to true : // with null slots "since it does not evaluate to true with null slots". http://gerrit.cloudera.org:8080/#/c/15047/11/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/15047/11/testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test@1055 PS11, Line 1055: select a.id, b.id Can you add a similar test, except with b.int_col in the ON clause? -- 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: 11 Gerrit-Owner: Xianqing He <hexianqing...@126.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Xianqing He <hexianqing...@126.com> Gerrit-Comment-Date: Mon, 09 Mar 2020 22:40:58 +0000 Gerrit-HasComments: Yes
