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 5: (7 comments) > (7 comments) > > Thanks for digging into this! > > If I understand this solution correctly, the key change of this > patch is inside SingleNodePlanner#migrateConjunctsToInlineView(). > 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. This patch picks up the > predicates that being skipped due to not satisfying > isLastOjMaterializedByTupleIds() and duplicates them if they > satisfy !isTrueWithNullSlots(). > > I left some comments for refactoring. Hope we can make this part > clearer. I refactored this modification according to your comments, thanks! http://gerrit.cloudera.org:8080/#/c/15047/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15047/4//COMMIT_MSG@7 PS4, Line 7: Propagate predicates of outer-joined InlineView : > This is an improvement rather than a fix. I think this is better: "Propagat Done http://gerrit.cloudera.org:8080/#/c/15047/4//COMMIT_MSG@10 PS4, Line 10: the outer join into inline view. : I refactored the code "Analyzer#canEvalPredicate()" and add : "SingleNode > Not all the predicates of the nullable side could be duplicated into the in Done http://gerrit.cloudera.org:8080/#/c/15047/4/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/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1623 PS4, Line 1623: // Ignore 'tid' because it is not outer-joined. : if (rhsRef == null) continue; : // Check whether the last join to outer-join 'tid' is materialized by tupleIds. : if (!tupleIds.containsAll(rhsRef.getAllTableRefIds())) return false; : } : return true; : } : : public boolean canEvalOnClauseConjunct(List<TupleId> tupleIds, Expr e) { : if (isAntiJoinedConjunct(e)) return canEvalAntiJoinedConjunct(e, tupleIds); : : List<TupleId> tids = new ArrayList<>(); : e.getIds(tids, null); : if (tids.isEmpty()) return true; : : if (isIjConjunct(e) || isSjConjunct(e)) { : if (!containsOuterJoinedTid(tids)) return true; : // If the predicate references an outer-joined tuple, then evaluate it at : // the join that the On-clause belongs to. : TableRef onClauseTableRef = null; : if (isIjConjunct(e)) { : onClauseTableRef = globalState_.ijClauseByConjunct.get(e.getId()); : } else { : onClauseTableRef = globalState_.sjClauseByConjunct.get(e.getId()); : } : Preconditions.checkNotNull(onClauseTableRef); : return tupleIds.containsAll(onClauseTableRef.getAllTableRefIds()); : } : : if (isFullOuterJoined(e)) return c > The new overload of canEvalPredicate() is not quite clear in my opinion. We Done http://gerrit.cloudera.org:8080/#/c/15047/4/fe/src/main/java/org/apache/impala/planner/AggregationNode.java File fe/src/main/java/org/apache/impala/planner/AggregationNode.java: http://gerrit.cloudera.org:8080/#/c/15047/4/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@184 PS4, Line 184: conjuncts_ = orderConjunctsByCost(conjuncts_); > Can we move this into MultiAggregateInfo#collectConjuncts()? Done http://gerrit.cloudera.org:8080/#/c/15047/4/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/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1191 PS4, Line 1191: if (LOG. isTraceEnabled()) { : LOG.trace(String.format("Can propagate %s to inline view %s", : e.debugString(), inlineViewRef.getExplicitAlias())); : } : } : continue; : } catch (InternalException ex) { : // Expr evaluation failed in the backend. Skip 'e' since we cannot : // determine whether propagation is safe. : LOG.warn("Skipping propagation of conjunct because backend evaluation failed: " : + e.toSql(), ex); : continue; : } : } : if (LOG. isTraceEnabled()) { : LOG.trace(String.format("Can evaluate %s in inline view %s", e.debugString(), : > Althouth the first if-clause is semantically equal to the old logics, it's Done http://gerrit.cloudera.org:8080/#/c/15047/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1219 PS4, Line 1219: * makes the *output* of the computation visible to the enclosing scope, so that : * filters from the enclosing scope can be safe > "Propagate the conjuncts evaluating the nullable side of outer-join. Don't Done http://gerrit.cloudera.org:8080/#/c/15047/4/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/4/testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test@1002 PS4, Line 1002: 73 > This is actually 73 after adding the predicate. Note that the test passes s Done -- 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: 5 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: Tue, 21 Jan 2020 05:50:58 +0000 Gerrit-HasComments: Yes
