Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/15047 )
Change subject: IMPALA-8361: Propagate predicates of outer-joined InlineView ...................................................................... Patch Set 7: (3 comments) http://gerrit.cloudera.org:8080/#/c/15047/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15047/7//COMMIT_MSG@11 PS7, Line 11: I refactored the code "Analyzer#canEvalPredicate()" and add : "SingleNodePlanner#getConjunctsToInlineView()" to get conjucts can be : correctly to migrate or propagate into inline view. : Before this change, we skip predicate b.upper_val='1' since : canEvalPredicate() returns false on it. Because it doesn't satisfy : isLastOjMaterializedByTupleIds(). So we can't migrate it inside the : inline view. However, we can be more aggressive. It's correct to : duplicate(not migrate) this predicate inside the inline view since : it's not evaluted to true with null slots. > Please take some time to write the commit message. It's very important for Agree with Quanlong regarding improving the commit message. In addition, I would suggest NOT putting the entire Explain plan in the commit message. These can be quite verbose and is best described in the JIRA. http://gerrit.cloudera.org:8080/#/c/15047/7/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/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1618 PS7, Line 1618: public boolean isLastOjMaterializedByTupleIds(List<TupleId> tupleIds, Expr e) { Pls add javadoc comment http://gerrit.cloudera.org:8080/#/c/15047/7/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/7/testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test@1001 PS7, Line 1001: | | having: int_col = 17 Could you clarify why the HAVING predicate got added..considering that it is already pushed to the Scan node. This predicate looks redundant. -- 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: 7 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: Xianqing He <hexianqing...@126.com> Gerrit-Comment-Date: Thu, 23 Jan 2020 07:20:57 +0000 Gerrit-HasComments: Yes
