englefly commented on code in PR #69111:
URL: https://github.com/apache/doris/pull/69111#discussion_r4290638044
##########
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:
Fixed in `fe4ae780bbf`.
Confirmed the stale maps first: with only the fresh-ID change, the reduced
plan of your shape is
```text
PRODUCER out=[k#10, v#1, s#3]
CONSUMER RelationId#0 out=[k#4, v#5, s#6]
c2p={k#4=k#0, v#5=v#1, s#6=s#3} <- k#0 is not an output of the
producer any more
p2c={k#0=[k#4], v#1=[v#5], s#3=[s#6]}
```
so the consumer maps were indeed left stale. Two notes on the failure mode
though: in this shape `PlanChecker.checkPlannerResult` still succeeded, i.e. I
could not reproduce a planning failure (the physical CTE wiring does not read
those maps for this query), but the logical plan is inconsistent and the maps
are used by CBO (`CascadesContext.addCTEConsumerGroup`), so it has to be fixed
regardless.
I took the "preserve the boundary's output contract" option rather than
carrying the remap across the boundary: the consumers are rewritten *before*
the producer and in a different `CascadesContext`
(`RewriteCteChildren.newSubtreeContext`), so the producer invocation has no way
to rebuild them. The rule now detects a producer body through
`CascadesContext.getCurrentTree()` (present only for the producer side) and
does not rename the exprIds of the producer outputs:
* a constant grouping key which is a producer output keeps its grouping key,
and its output stays the aggregate slot (no constant project, no mapping),
* a project item which is a producer output is not replaced.
Everything below the producer output keeps the fresh-exprId treatment, and
the shadow that motivated the PR (RQG case 11000258898, no CTE) is unaffected.
Regression test
`ConstantPropagationFreshExprIdTest#testMaterializedCteConsumerMapsStayValid`
asserts that every producer slot a consumer map points to is an output of the
producer and that the query stays plannable. It fails before this commit with
```text
the consumer map of RelationId#2 references k#0 which the producer does not
output
```
A materialized-CTE physical planning case (`checkPlannerResult` on `WITH c
AS (SELECT k, v, count(*) cnt FROM t WHERE k = 5 GROUP BY k, v) SELECT ... FROM
c l CROSS JOIN c r`) is part of the same 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:
Fixed in `fe4ae780bbf`.
You are right that `ExpressionRewrite.buildRules()` has no
`LogicalBlackholeSink` rule, and checking the dispatch it is not only
blackhole: `logicalExternalRowLevelDeleteSink`, `logicalJdbcTableSink` /
`logicalMaxComputeTableSink` (they have their own `PlanType`, so
`logicalConnectorTableSink()` does not match them) and the unbound sinks are
not covered either. So instead of adding one rule per sink type,
`ExprIdRewriter.rewriteExpr` now has a sink-agnostic fallback: when no
registered rule matched a `LogicalSink`, its `outputExprs` are rewritten
through the same `ExpressionRuleExecutor` and applied with `withOutputExprs`.
Every sink type, including future ones, therefore follows the fresh exprIds of
its child, while the sink metadata (name, data type, connector/table
properties) is kept — the fallback only remaps ids, exactly like the existing
per-type rules do.
Regression test
`ConstantPropagationTest#testBlackholeSinkFollowsFreshExprId` builds the
warm-up shape (`LogicalBlackholeSink` over `aggregate(group by id, gender) ->
filter(id = 5) -> scan`) and asserts that
* every output expression of the sink is an id its child really outputs, and
* the constant column is refreshed to the fresh exprId of the constant
project.
It fails before this commit with
```text
the sink expects id#0 which its child does not output: [sum_age#7, gender#1,
id#8]
```
Building an actual `WARM UP SELECT` statement would need the cloud mirror
environment, so the test constructs the same logical shape the warm-up
statement produces.
--
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]
