Xianqing He has posted comments on this change. ( http://gerrit.cloudera.org:8080/15047 )
Change subject: IMPALA-8361: Propagate predicates of outer-joined InlineView ...................................................................... Patch Set 12: (26 comments) I'm really sorry that because I'm not familiar with gerrit, I found that none of the replies were sent out. Thank you for reviewing my code. I apologize again for not replying before 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 an improvement that tries > "This is an improvement that tries to propagate" Done http://gerrit.cloudera.org:8080/#/c/15047/7//COMMIT_MSG@11 PS7, Line 11: : For example: : SELECT * : FROM functional.alltypessmall a : LEFT JOIN ( : SELECT id, upper(string_col) AS upper_val, : length(string_col) AS len : FROM functional.alltypestiny : ) b ON a.id = b.id > Please take some time to write the commit message. It's very important for Done http://gerrit.cloudera.org:8080/#/c/15047/7//COMMIT_MSG@11 PS7, Line 11: : For example: : SELECT * : FROM functional.alltypessmall a : LEFT JOIN ( : SELECT id, upper(string_col) AS upper_val, : length(string_col) AS len : FROM functional.alltypestiny : ) b ON a.id = b.id > Agree with Quanlong regarding improving the commit message. In addition, I Done 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 Done http://gerrit.cloudera.org:8080/#/c/15047/9//COMMIT_MSG@24 PS9, Line 24: some predicates that : must be evaluted at a join node 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 Done http://gerrit.cloudera.org:8080/#/c/15047/9//COMMIT_MSG@30 PS9, Line 30: . > nit: need space Done 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: return true; > Pls add javadoc comment Done 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, see canEvalOnClauseConjunct() for more info. > I think this part of the comment about on-clause predicates should be moved Done http://gerrit.cloudera.org:8080/#/c/15047/11/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1643 PS11, Line 1643: e.getIds(exprTids, null); > Maybe rename this to exprTids or similar, so that it's clearer how it is di Done 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? If some conjuncts are pushed to having predicates, it may appear duplicate predicates, eg AGGREGATE [FINALIZE] | group by: id | having: id = 12, id = 12 Here's a simple fix for this situation. 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 Done 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 unassigned conjuncts that can be eva > "Get the unassigned conjuncts that can be evaluated in inline view and retu Done http://gerrit.cloudera.org:8080/#/c/15047/7/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1163 PS7, Line 1163: * in 'evalInInlineViewPreds'. : * If a conjunct is not an On-clause predicate and is safe to > If a conjunct is not an On-clause predicate and is safe to propagate it ins Done http://gerrit.cloudera.org:8080/#/c/15047/7/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1167 PS7, Line 1167: er an > I think something like "evalInViewPreds" may be more clear for its meaning. Done http://gerrit.cloudera.org:8080/#/c/15047/7/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1176 PS7, Line 1176: e.getIds(tids, null); : if (tids.isEmpt > Just be curious, can we add test coverage for this case? Done http://gerrit.cloudera.org:8080/#/c/15047/7/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1190 PS7, Line 1190: r > nit: remove space here Done 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 Done http://gerrit.cloudera.org:8080/#/c/15047/10/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/10/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1178 PS10, Line 1178: evalInInlineViewPreds.add(e); > Can we simply fix IMPALA-9356 by adding this to evalAfterJoinPreds? For inline view we can fix by this, but real table also has this question. I have updated IMPALA-9356. Need I fix inline view by this? 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 does not evalute to : // true with null > "since it does not evaluate to true with null slots". Done 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: Done 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: d=b.id > This looks strange to me. rand() returns a random value between 0 and 1 so Yes, this is a bug. I reported https://issues.apache.org/jira/browse/IMPALA-9356 http://gerrit.cloudera.org:8080/#/c/15047/9/testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test@2402 PS9, Line 2402: a > nit: put the space after the comma Done http://gerrit.cloudera.org:8080/#/c/15047/9/testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test@2442 PS9, Line 2442: FROM functional.a > Should we move the function inside the view? This can be propagated without Done http://gerrit.cloudera.org:8080/#/c/15047/10/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/10/testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test@2339 PS10, Line 2339: ==== > Why removing the test coverage for local views? I deleted it by mistake, I will add it back 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 i This is an existing problem, and I reported the improvement IMPALA-9305. 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? More tests in inline-view.test -- 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: 12 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: Tim Armstrong <[email protected]> Gerrit-Reviewer: Xianqing He <[email protected]> Gerrit-Comment-Date: Thu, 19 Mar 2020 01:37:57 +0000 Gerrit-HasComments: Yes
