github-actions[bot] commented on code in PR #69111:
URL: https://github.com/apache/doris/pull/69111#discussion_r4279440834
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/ConstantPropagation.java:
##########
@@ -218,11 +256,16 @@ public Plan visitLogicalAggregate(LogicalAggregate<?
extends Plan> aggregate, Ca
}
if (newGroupByExprs.equals(oldGroupByExprs) &&
newOutputExprs.equals(oldOutputExprs)) {
+ // the aggregate is not changed, the plan above keeps referencing
the output slots of the aggregate,
+ // so neither the constant project nor its fresh exprIds are used
return aggregate;
}
aggregate = aggregate.withGroupByAndOutput(newGroupByExprs,
newOutputExprs);
if (containsConstantOutput) {
+ for (Pair<ExprId, ExprId> freshExprId : freshExprIds) {
+ replacedExprIds.put(freshExprId.first, freshExprId.second);
Review Comment:
[P1] Propagate fresh producer IDs across the CTE rewrite boundary
For a twice-consumed materialized CTE such as `WITH c AS (SELECT k,v,SUM(x)
s FROM t WHERE k=5 GROUP BY k,v) SELECT l.k,l.v,l.s,r.k,r.v,r.s FROM c l CROSS
JOIN c r` (with nonunique rows and nonuniform `v`), the reduced input is:
```text
CTEAnchor
Producer: Aggregate(group=k#1,v#2; output=k#1,v#2,SUM(x)#3)
Filter(k#1=5) -> Scan
CrossJoin(ConsumerL(map k#1 -> lk), ConsumerR(map k#1 -> rk))
```
`RewriteCteChildren` rewrites the consumer subtree first, then runs this
rule separately on `producer.child()`. Removing the constant group key now
produces `Project(5 AS k#fresh,v#2,s#3)`, but this invocation-local map never
visits those consumers. Their correspondence maps still reference `k#1`. Later
`AdjustNullable` only updates attributes by the same ExprId, and physical CTE
translation looks up consumer slots using the fresh producer outputs, so
neither consumer's `k` gets a SlotRef and planning fails. Carry the producer
remap across the subtree boundary and update both consumer maps (or preserve
the boundary's output contract), with a materialized-CTE physical planning
regression test.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/ConstantPropagation.java:
##########
@@ -295,15 +339,18 @@ public Plan visitLogicalSink(LogicalSink<? extends Plan>
sink, CascadesContext c
// // the select will add a parent plan: result sink. the result sink
contains a output slot reference, and its
// // data type is varchar(30), but if replace the slot reference
with a varchar literal '1',
// // then the data type info varchar(30) will lost, because varchar
literal '1' data type is always varchar(1),
- // // so t's column will get a error type. so we don't rewrite logical
sink then.
+ // // so t's column will get a error type. so we don't replace the
slots of the sink by literals.
// Pair<ImmutableEqualSet<Slot>, Map<Slot, Literal>> childEqualTrait
// = getChildEqualSetAndConstants(sink, context);
// List<NamedExpression> newOutputExprs =
sink.getOutputExprs().stream()
// .map(expr ->
// replaceNameExpressionConstants(expr, context,
childEqualTrait.first, childEqualTrait.second))
// .collect(ImmutableList.toImmutableList());
// return newOutputExprs.equals(sink.getOutputExprs()) ? sink :
sink.withOutputExprs(newOutputExprs);
- return sink;
+ // The sink still has to follow the fresh exprIds of the constants
materialized below it, otherwise it
+ // would reference a slot which its child does not output any more.
Only the exprId is remapped here, so
+ // the name and the data type declared by the sink are kept.
+ return (LogicalSink<? extends Plan>) exprIdRewriter.rewriteExpr(sink,
replacedExprIds);
Review Comment:
[P2] Refresh Blackhole sink outputs when their child IDs change
`ExprIdRewriter` dispatches through `ExpressionRewrite.buildRules()`, which
has no `LogicalBlackholeSink` rule, so this call returns that sink unchanged. A
reachable case is cloud `WARM UP SELECT k,v,s FROM vw`, with file caching
enabled and `vw` defined as `SELECT k,v,SUM(x) s FROM t WHERE k=5 GROUP BY k,v`
over nonunique rows with varying `v`. The view preserves its child ExprIds and
is inlined before constant propagation, giving this reduced rewrite:
```text
BlackholeSink(k#1,v#2,s#3)
Aggregate(group=k#1,v#2) -> Filter(k#1=5) -> Scan
=> BlackholeSink(k#1,v#2,s#3)
Project(5 AS k#fresh,v#2,s#3) -> Aggregate(group=v#2) -> ...
```
`BindSink` takes these outputs directly from the child, and the sink's
`withChildren` retains them. The old `k#1` is therefore absent from the
rewritten child, and final slot validation in `CheckAfterRewrite` rejects the
warm-up query. Rewrite `LogicalSink.getOutputExprs()` generically through
`withOutputExprs`, or include Blackhole in the helper dispatch, while
preserving sink metadata. Add an aggregate-view warm-up planning case.
--
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]
