englefly commented on code in PR #69111:
URL: https://github.com/apache/doris/pull/69111#discussion_r4297720517
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/ConstantPropagation.java:
##########
@@ -115,9 +152,15 @@ public Plan visitLogicalHaving(LogicalHaving<? extends
Plan> having, CascadesCon
}
}
+ /** Whether this expression is an output of a CTE producer body, whose
exprId has to stay stable. */
+ private boolean isCteProducerOutput(Expression expr) {
+ return expr instanceof Slot &&
cteProducerOutputExprIds.contains(((Slot) expr).getExprId());
+ }
+
@Override
public Plan visitLogicalProject(LogicalProject<? extends Plan> project,
CascadesContext context) {
project = visitChildren(this, project, context);
+ project = (LogicalProject<? extends Plan>)
exprIdRewriter.rewriteExpr(project, replacedExprIds);
Review Comment:
Fixed in `9e1c57d1f0a`.
Confirmed the mechanism: `ExprIdRewriter` drove its rules through
`ExpressionRewrite#bottomUp`, and `ExpressionBottomUpRewriter` keeps a
`RequiresShortCircuitEvaluation` subtree opaque
(`ExpressionBottomUpRewriter.java:79-83`), while `MergeUtils.selectBranch`
wraps every MERGE assignment in a `ShortCircuitIf` (`MergeUtils.java:44-47`). A
slot inside the guarded branch therefore kept the replaced exprId.
`ExprIdRewriter` now replaces exprIds **structurally** instead of through
the bottom-up expression rewriter: it rewrites the children of every expression
first and then applies the id replacement to the node, so the guarded branches
are entered as well. Nothing is evaluated — replacing an exprId only rebuilds
the expression, which is exactly why the short-circuit guard must not stop it.
The change is local to `ExprIdRewriter` (`ExprIdRewriter.java:50-67`,
`ReplaceRule#replaceExprIds`), the `ReplaceRule` used by the group-key rules is
unchanged, and the expression-level behaviour for unguarded expressions is
identical (children first, same `lastExprId` chain, same object identity when
nothing is mapped).
Regression test `ExprIdRewriterTest#testGuardedBranchIsRewritten` builds
`Alias(#9, ShortCircuitIf(v#3 = 1, k#1 + 1, k#1))` and asserts that no
reference to `k#1` survives and `k#2` is present. It fails before this commit
with
```text
if((v#3 = 1), (k#1 + 1), k#1) ==> expected: <false> but was: <true>
```
Full MERGE planning still needs a MERGE-capable mocked env, so the gate is
at the expression tree the rewrite produces.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/RewriteCteChildren.java:
##########
@@ -99,22 +109,138 @@ public Plan visitLogicalCTEAnchor(LogicalCTEAnchor<?
extends Plan, ? extends Pla
cascadesContext.addPlanProcesses(outerCascadesCtx.getPlanProcesses());
cascadesContext.getStatementContext().getRewrittenCteConsumer().put(cteAnchor.getCteId(),
outer);
}
+ Set<LogicalCTEConsumer> cteConsumers = collectCteConsumers(outer,
cteAnchor.getCteId());
+ cascadesContext.getCteIdToConsumers().put(cteAnchor.getCteId(),
cteConsumers);
+ if (cteConsumers.isEmpty()) {
+ return outer;
+ }
+ List<Slot> oldProducerOutputs = cteAnchor.child(0).getOutput();
+ Plan producer = cteAnchor.child(0).accept(this, cascadesContext);
+ Map<ExprId, ExprId> producerExprIdRemap =
computeProducerExprIdRemap(oldProducerOutputs, producer.getOutput());
+ if (!producerExprIdRemap.isEmpty()) {
+ // The producer subtree is rewritten in its own context, so a rule
of it may rename the exprIds of the
+ // producer outputs, while the consumers, which are rewritten
before the producer, still map to the
+ // previous exprIds. Refresh both maps of every consumer of this
CTE with the new producer slots.
+ Map<ExprId, Slot> producerOutputSlots =
producer.getOutput().stream()
+ .collect(Collectors.toMap(Slot::getExprId,
Function.identity()));
Review Comment:
Fixed in `9e1c57d1f0a`.
Reproduced with your shape: the producer output of `WITH c AS (SELECT k, v,
v, count(*) cnt, v + 1 z FROM t WHERE k = 5 GROUP BY k, v) SELECT l.*, r.* FROM
c l CROSS JOIN c r` keeps the repeated `v#1` (both positions are the same
producer slot and `LogicalCTEConsumer` is multimap based), and the
`Collectors.toMap` lookup threw before the refresher ran:
```text
java.lang.IllegalStateException: Duplicate key 1 (attempted merging values
v#1 and v#1)
```
The lookup now keeps the first entry per exprId
(`RewriteCteChildren.java:121-128`), which is the same producer slot, so the
refresh works for such a CTE. Regression test
`ConstantPropagationFreshExprIdTest#testMaterializedCteWithRepeatedOutputSlot`
asserts the plan rewrites, the consumer maps stay valid and the query stays
plannable; the failure above is what it reports before this commit.
Two related notes from the same review round:
* your `RewriteCteChildren.java:180` finding is obsolete at this head - the
name/lineage matcher it refers to was replaced by the published remap, and both
constant group keys are remapped through it (`ConstantPropagation` publishes
every renamed exprId of the producer body);
* the fresh exprId branch of `replaceNameExpressionConstants` that the PR
body mentioned was unreachable (the method only receives an `Alias`, and
replacing the constants of an `Alias` yields an `Alias` again, which returns at
the guard above it), so it is removed rather than kept as untested code.
--
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]
