yujun777 commented on code in PR #69787:
URL: https://github.com/apache/doris/pull/69787#discussion_r4421604282
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/MaterializedViewUtils.java:
##########
@@ -479,6 +479,94 @@ public Plan visitLogicalSink(LogicalSink<? extends Plan>
rewrittenPlan, Void con
}
+ /**
+ * Align the outputs of a plan rewritten by a materialized view with the
origin query plan, so that both plans
+ * can be alternatives of one memo group.
+ *
+ * <p>The exprId of an output may only be replaced when both plans define
that output as the same constant:
+ * a column which a predicate pins to a literal can be materialized with a
fresh exprId, and the two plans then
+ * still compute the same value. For every other output the exprId has to
be the same already, otherwise the two
+ * plans are not known to produce the same columns (e.g. a self join can
expose two same named columns of two
+ * relations) and the rewritten plan must not be used.
+ */
+ public static Plan alignPlanOutputWithOrigin(Plan rewrittenPlan, Plan
originPlan) {
+ List<Slot> originOutputs = originPlan.getOutput();
+ List<Slot> rewrittenOutputs = rewrittenPlan instanceof LogicalSink
+ ? ((LogicalSink<?>) rewrittenPlan).child().getOutput()
+ : rewrittenPlan.getOutput();
+ if (originOutputs.size() != rewrittenOutputs.size()) {
+ return null;
+ }
+ Map<ExprId, Expression> originOutputDefinitions =
collectNamedExpressionDefinitions(originPlan);
+ List<NamedExpression> alignedOutputs = new
ArrayList<>(originOutputs.size());
+ for (int i = 0; i < originOutputs.size(); i++) {
+ Slot originSlot = originOutputs.get(i);
+ Slot rewrittenSlot = rewrittenOutputs.get(i);
+ if (rewrittenSlot.getExprId().equals(originSlot.getExprId())) {
+ alignedOutputs.add(rewrittenSlot);
+ continue;
+ }
+ if (!isSameConstant(rewrittenSlot,
originOutputDefinitions.get(originSlot.getExprId()))
Review Comment:
This check can never pass. `rewrittenSlot` is a `Slot`, and `Slot extends
NamedExpression implements LeafExpression`, so `Expression#isConstant` takes
the `LeafExpression` branch and returns `this instanceof Literal`, which is
false for every slot (`Literal` is not a `Slot` and no subclass overrides
`isConstant`).
Even if the definition were looked up on the rewritten side instead,
`collectNamedExpressionDefinitions` could not provide one: it is top-down with
`putIfAbsent`, and for a query the sink reuses the child output slots as its
output exprs (`BindExpression#bindResultSink`), so the map entry for an output
exprId is the top-level `SlotReference` itself, not the alias which defines it.
The exprId is therefore rebound for every output which is merely a
same-named, same-typed column, and the invariant stated in the Javadoc and the
commit message ("The exprId of an output may only be replaced when both plans
define it as the same constant") is not enforced.
Could you either
1. compare the lineage-shuttled expressions of both output slots
(`ExpressionUtils.shuttleExpressionWithLineage`, as `rewriteExpression` already
does) so that the sink slot -> project alias chain is expanded, and add a unit
test for "same constant, different exprId is accepted" and "same name,
different constant is rejected", or
2. remove `isSameConstant` and `collectNamedExpressionDefinitions` and
describe the actual rule (name, type and qualifier have to match) if that is
the intended guarantee.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
