github-actions[bot] commented on code in PR #69784:
URL: https://github.com/apache/doris/pull/69784#discussion_r4402937970
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/ConstantPropagation.java:
##########
@@ -250,13 +254,22 @@ public Plan visitLogicalAggregate(LogicalAggregate<?
extends Plan> aggregate, Ca
}
if (replacedExpr.isConstant() && !replacedExpr.equals(expr)) {
- // The constant is materialized above the aggregate, but it
keeps the exprId of the aggregate
- // output instead of getting a fresh one and refreshing the
plan above: when the aggregate still
- // outputs that column, the value is the same constant, and
when it removed the column, this
- // project item is the only definition of the exprId. Renaming
the exprId would break the
- // consumers which rely on the identity of the column, e.g.
the materialized view matching, and
- // would make the plan above be rewritten for nothing.
- projectBuilder.add(new Alias(expr.getExprId(), replacedExpr,
expr.getName()));
+ // The constant is materialized above the aggregate. It keeps
the exprId of the aggregate output
+ // when the nullability of the column does not change, or when
the child does not produce the
+ // column any more (then this project item is the only
definition of the exprId): the consumers
+ // which rely on the identity of the column, e.g. the
materialized view matching, keep working.
+ // When the child still produces the column and the constant
has another nullability, the exprId
+ // has to be renamed and the references above have to be
refreshed: otherwise the plan keeps two
+ // definitions of the same exprId with different nullability,
and AdjustNullable widens the
+ // column on the way down.
+ ExprId outputExprId = expr.getExprId();
+ if (childOutputExprIds.contains(outputExprId) &&
replacedExpr.nullable() != expr.nullable()) {
+ ExprId freshExprId =
replacedExprIds.computeIfAbsent(outputExprId,
Review Comment:
[P1] Do not record a remap for an unchanged aggregate
For a duplicate-key table with nullable `k`, `SELECT k, COUNT(*) FROM t
WHERE k = 5 GROUP BY k` reaches this reduced shape:
```text
ResultSink(k#1, cnt#2)
Aggregate(group=[k#1], output=[k#1, COUNT(*) AS cnt#2])
Filter(k#1 = 5)
Scan(k#1 nullable)
```
`visitLogicalAggregate` restores the sole constant group key at lines
228–229 and keeps all aggregate outputs unchanged. This branch nevertheless
inserts `#1 -> fresh#3` into `replacedExprIds`. The unchanged-aggregate return
at lines 280–282 then discards `projectBuilder`, so no node defines `#3`.
`visitLogicalSink` (or an intervening project's `ExprIdRewriter`) rewrites the
parent to consume `k#3` while the aggregate still outputs `k#1`, producing an
invalid plan for this ordinary grouped query. Defer publishing the mapping
until the defining project is retained, or retain the project when a fresh ID
is required. Add a sole nullable-key case: the new two-key tests always change
the aggregate and miss this return path.
--
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]
