github-actions[bot] commented on code in PR #69787:
URL: https://github.com/apache/doris/pull/69787#discussion_r4438742583
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/MaterializedViewUtils.java:
##########
@@ -471,14 +471,125 @@ public Plan visitLogicalSink(LogicalSink<? extends Plan>
rewrittenPlan, Void con
normalizedOutputExprList.add(normalizeExpression(originalExpression,
rewrittenExpression, true));
}
- LogicalProject<Plan> project = new
LogicalProject<>(normalizedOutputExprList,
- rewrittenPlan.child());
- return rewrittenPlan.withChildren(project);
+ // Set the outputs on the sink itself. A project between the
sink and the child would only
+ // pass the child outputs through with a changed nullable
property, and the slot of such a
+ // project does not describe the expression which produces it:
a rule which merges the
+ // projects, e.g. MergeProjectsCBO, then derives the original
nullable property again and
+ // the plan no longer matches the logical properties of the
group it is inserted into.
+ return rewrittenPlan.withOutputExprs(normalizedOutputExprList);
Review Comment:
[P1] Keep aligned output slots materialized for rewritten file sinks. With
DML MV rewrite enabled, file sinks are recorded and normalized here. If an
output exprId differs, normalizeExpression creates an origin-id Alias, but this
change installs it only in LogicalFileSink.outputExprs while leaving the child
plan's output at the rewritten id. PhysicalPlanTranslator.visitPhysicalFileSink
translates fileSink.getOutput() through context.findSlotRef(), whose map is
populated by the child plan, so the origin id has no SlotRef and translation
fails. Preserve a project/child output carrying the aligned ids, or otherwise
keep the sink output ids resolvable, before accepting the rewritten plan.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/MaterializedViewUtils.java:
##########
@@ -471,14 +471,125 @@ public Plan visitLogicalSink(LogicalSink<? extends Plan>
rewrittenPlan, Void con
normalizedOutputExprList.add(normalizeExpression(originalExpression,
rewrittenExpression, true));
}
- LogicalProject<Plan> project = new
LogicalProject<>(normalizedOutputExprList,
- rewrittenPlan.child());
- return rewrittenPlan.withChildren(project);
+ // Set the outputs on the sink itself. A project between the
sink and the child would only
+ // pass the child outputs through with a changed nullable
property, and the slot of such a
+ // project does not describe the expression which produces it:
a rule which merges the
+ // projects, e.g. MergeProjectsCBO, then derives the original
nullable property again and
+ // the plan no longer matches the logical properties of the
group it is inserted into.
+ return rewrittenPlan.withOutputExprs(normalizedOutputExprList);
}
}, null);
}
+ /**
+ * 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>An output keeps its exprId. The exprId of the origin output may only
be substituted
+ * <ul>
+ * <li>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,
+ * or</li>
+ * <li>when the rewritten output describes the same column as the
origin one: the name and the type have
+ * to match and the qualifier has to be kept by both sides and be
equal, so that a same named column of
+ * another relation (e.g. the two relations of a self join) is not
accepted.</li>
+ * </ul>
+ * 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);
+ Map<ExprId, Expression> rewrittenOutputDefinitions =
collectNamedExpressionDefinitions(rewrittenPlan);
+ 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;
+ }
+ // the slot itself is not a constant, the definition of its exprId
is
+ Expression originDefinition =
originOutputDefinitions.get(originSlot.getExprId());
+ Expression rewrittenDefinition =
rewrittenOutputDefinitions.get(rewrittenSlot.getExprId());
+ if (!isSameConstant(rewrittenDefinition, originDefinition)
+ && !isSameColumn(rewrittenSlot, originSlot)) {
+ return null;
+ }
+ Expression inner = rewrittenSlot.nullable() ==
originSlot.nullable()
+ ? rewrittenSlot
+ : (originSlot.nullable() ? new Nullable(rewrittenSlot) :
new NonNullable(rewrittenSlot));
+ alignedOutputs.add(new Alias(originSlot.getExprId(), inner,
originSlot.getName()));
+ }
+ if (rewrittenPlan instanceof LogicalSink) {
+ LogicalSink<?> sink = (LogicalSink<?>) rewrittenPlan;
+ List<NamedExpression> sinkOutputs = alignedOutputs.stream()
+ .map(output -> (NamedExpression) output.toSlot())
+ .collect(ImmutableList.toImmutableList());
+ // Only the outputs of the sink have to be aligned, adding a
project below it would repeat the
+ // outputs of the child with a changed nullable property, see
normalizeSinkExpressions.
+ return sink.withOutputExprs(sinkOutputs);
+ }
+ return new LogicalProject<>(alignedOutputs, rewrittenPlan);
+ }
+
+ /**
+ * Whether the rewritten output describes the same column as the origin
one. The rewritten plan is built by the
+ * materialized view rewrite which keeps the position of the query
outputs, so the name, the type and the
+ * qualifier have to match. The qualifier has to be kept by both sides: a
missing qualifier can not rule out
+ * that the slot is the same named column of another relation, e.g. the
two relations of a self join, so an
+ * output without a qualifier is rejected and the rewritten plan is not
used. This also rejects an output which
+ * is added or removed and shifts the columns.
+ */
+ private static boolean isSameColumn(Slot rewrittenSlot, Slot originSlot) {
+ if (!rewrittenSlot.getName().equalsIgnoreCase(originSlot.getName())
+ ||
!rewrittenSlot.getDataType().equals(originSlot.getDataType())) {
+ return false;
+ }
+ List<String> rewrittenQualifier = rewrittenSlot.getQualifier();
+ List<String> originQualifier = originSlot.getQualifier();
+ return !rewrittenQualifier.isEmpty() &&
rewrittenQualifier.equals(originQualifier);
Review Comment:
[P1] Include variant path identity in the column fallback. isSameColumn
accepts different exprIds based only on name, type, and qualifier, but
SlotReference.subPath is part of the column identity and MV variant mapping
explicitly distinguishes it. A rewritten payload['b'] slot can therefore be
rebound to an origin payload['a'] exprId and pass the LogicalProperties check,
returning the wrong field. Compare subPath, and reject or compare other slot
identity metadata as appropriate, before rebinding.
--
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]
