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 7: (11 comments) The patchs LGTM. Left some comments on code readability and tests. http://gerrit.cloudera.org:8080/#/c/15047/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15047/7//COMMIT_MSG@9 PS7, Line 9: This is improvement that propagate "This is an improvement that tries to propagate" 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 others to understand your work. Your commit message will be read many times so it worth your time to polish it. It should contain what the problem was, and how it was fixed. Talking too much about codes doesn't help for understanding it. We have some guidelines in the wiki: https://cwiki.apache.org/confluence/display/IMPALA/Contributing+to+Impala Here are some examples for planner fixes: https://github.com/apache/impala/commit/df5c4061456abb947cec8add81b361b60c5d3ad8 https://github.com/apache/impala/commit/c665fc1e06d53c7b70611ad3993c712c7fe35cb2 https://github.com/apache/impala/commit/ae8295118191486f31da4d8d3c9d0f7e7e5d4b3a https://github.com/apache/impala/commit/f25a899924856f705ecb581b237a149003279473 https://github.com/apache/impala/commit/5e9b4e2fd2a459a3b61e8939e3085ad8e1a5795e 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@1630 PS7, Line 1630: Please add a function comment like other "canEval*" functions. http://gerrit.cloudera.org:8080/#/c/15047/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1631 PS7, Line 1631: public boolean canEvalOnClauseConjunct(List<TupleId> tupleIds, Expr e) { Add Preconditions.checkState(e.isOnClauseConjunct()) at the beginning of this function. http://gerrit.cloudera.org:8080/#/c/15047/7/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/7/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1162 PS7, Line 1162: Get the conjuncts evaluating in inline view. "Get the unassigned conjuncts that can be evaluated in inline view and return them in 'preds'." http://gerrit.cloudera.org:8080/#/c/15047/7/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1163 PS7, Line 1163: * The unassigned conjuncts can be correctly evaluated by inline view, add them to preds : * and if propagation is safe, add them to evalAfterJoinPreds. If a conjunct is not an On-clause predicate and is safe to propagate it inside the inline view, add it to 'evalAfterJoinPreds'. http://gerrit.cloudera.org:8080/#/c/15047/7/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1167 PS7, Line 1167: preds I think something like "evalInViewPreds" may be more clear for its meaning. http://gerrit.cloudera.org:8080/#/c/15047/7/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1176 PS7, Line 1176: if (tids.isEmpty()) { : preds.add(e); Just be curious, can we add test coverage for this case? http://gerrit.cloudera.org:8080/#/c/15047/7/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1190 PS7, Line 1190: nit: remove space here http://gerrit.cloudera.org:8080/#/c/15047/7/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1203 PS7, Line 1203: nit: remove space here http://gerrit.cloudera.org:8080/#/c/15047/7/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/7/testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test@2339 PS7, Line 2339: ==== It'd be better to add more test cases. E.g: # Local view WITH b as (SELECT id, upper(string_col) AS upper_val FROM functional.alltypestiny) SELECT * FROM functional.alltypessmall a LEFT JOIN b ON a.id = b.id WHERE b.upper_val = '1'; # Where-clause predicate that can't be propagated SELECT * FROM functional.alltypessmall a LEFT JOIN ( SELECT id, upper(string_col) AS upper_val FROM functional.alltypestiny ) b ON a.id = b.id WHERE b.upper_val is NULL; # More predicates in where-clause SELECT * FROM functional.alltypessmall a LEFT JOIN ( SELECT id + 1 as id, upper(string_col) AS upper_val, length(string_col) AS len FROM functional.alltypestiny ) b ON a.id = b.id WHERE b.upper_val is NULL and b.len = 0 and b.id > 0; -- 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: Wed, 22 Jan 2020 08:41:04 +0000 Gerrit-HasComments: Yes
