github-actions[bot] commented on code in PR #65846:
URL: https://github.com/apache/doris/pull/65846#discussion_r3703111163
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SetPreAggStatus.java:
##########
@@ -371,51 +514,128 @@ private Pair<Set<SlotReference>, Set<SlotReference>>
splitKeyValueSlots(Set<Slot
return Pair.of(keySlots, valueSlots);
}
- private PreAggStatus checkAggWithKeyAndValueSlots(AggregateFunction
aggFunc,
- Set<SlotReference> keySlots, Set<SlotReference> valueSlots) {
+ private PreAggStatus checkAggWithKeyAndValueSlots(AggregateFunction
aggFunc, Set<Slot> outputSlots) {
Expression child = aggFunc.child(0);
List<Expression> conditionExps = new ArrayList<>();
List<Expression> returnExps = new ArrayList<>();
- // ignore cast
- while (child instanceof Cast) {
- if (!((Cast) child).getDataType().isNumericType()) {
- return PreAggStatus.off(String.format("%s is not numeric
CAST.", child.toSql()));
- }
- child = child.child(0);
- }
- // step 1: extract all condition exprs and return exprs
+ // Only peel casts that are proven order-preserving for MAX/MIN:
+ // 1. Injective numeric→numeric casts (widening integral/decimal)
+ // 2. Numeric→float casts (nondecreasing, e.g. BIGINT→DOUBLE)
+ // sum(cast(x)) and sum(x) are not interchangeable
+ // due to overflow/precision, so SUM must stay OFF.
+ if (aggFunc instanceof Max || aggFunc instanceof Min) {
+ child = peelCastForMaxMin(child);
+ }
+ // Reject remaining cast.
+ if (child instanceof Cast) {
+ return PreAggStatus.off(String.format("%s is not supported.",
child.toSql()));
+ }
+ // step 1: extract all condition exprs and return exprs.
+ // child is guaranteed to be Cast-free here (rejected above), but
+ // individual IF/CaseWhen return expressions may still have their
+ // own Cast wrappers. Only strip those for MAX/MIN: sum(cast(x))
+ // and cast(sum(x)) are not interchangeable due to overflow.
if (child instanceof If) {
conditionExps.add(child.child(0));
- returnExps.add(removeCast(child.child(1)));
- returnExps.add(removeCast(child.child(2)));
+ returnExps.add((aggFunc instanceof Max || aggFunc instanceof
Min)
+ ? peelCastForMaxMin(child.child(1)) : child.child(1));
+ returnExps.add((aggFunc instanceof Max || aggFunc instanceof
Min)
+ ? peelCastForMaxMin(child.child(2)) : child.child(2));
} else if (child instanceof CaseWhen) {
CaseWhen caseWhen = (CaseWhen) child;
// WHEN THEN
for (WhenClause whenClause : caseWhen.getWhenClauses()) {
conditionExps.add(whenClause.getOperand());
- returnExps.add(removeCast(whenClause.getResult()));
+ returnExps.add((aggFunc instanceof Max || aggFunc
instanceof Min)
+ ? peelCastForMaxMin(whenClause.getResult())
+ : whenClause.getResult());
}
// ELSE
-
returnExps.add(removeCast(caseWhen.getDefaultValue().orElse(new
NullLiteral())));
+ returnExps.add((aggFunc instanceof Max || aggFunc instanceof
Min)
+ ? peelCastForMaxMin(
+ caseWhen.getDefaultValue().orElse(new
NullLiteral()))
+ : caseWhen.getDefaultValue().orElse(new
NullLiteral()));
} else {
- // currently, only IF and CASE WHEN are supported
- returnExps.add(removeCast(child));
+ // Non-IF/CASE — conditionExps stays empty and returns OFF
below.
+ returnExps.add(peelCastForMaxMin(child));
+ }
+
+ // step 1.5: ownership — every return expression must reference
only
+ // this scan's own columns. PREAGG ON exposes this scan's partial
+ // (unmerged) rows; under join fan-out a return that references a
+ // foreign value column would then be evaluated once per partial
row
+ // and double-counted. So a foreign slot (value or key) in any
return
+ // forces this scan OFF — never use a foreign column to justify ON.
+ //
+ // Exception: MAX/MIN are idempotent — max(x, x) = x — so
repeating a
+ // foreign value across partial rows cannot change the aggregate
+ // result. The fence is over-conservative for them: a foreign
return
+ // branch is safe once the condition is row-stable (step 2) and the
+ // return slot still matches the aggregate type (enforced by
+ // KeyAndValueSlotsAggChecker). Keep the fence for non-idempotent
+ // aggregates (SUM, COUNT, ...) where a repeated foreign value
would
+ // be double-counted.
+ if (!(aggFunc instanceof Max || aggFunc instanceof Min)) {
+ for (Expression returnExp : returnExps) {
+ if (returnExp instanceof SlotReference &&
!outputSlots.contains(returnExp)) {
+ return PreAggStatus.off(
+ String.format("return expression %s references
column not owned by this scan.",
+ returnExp.toSql()));
+ }
+ }
+ }
+ if (conditionExps.isEmpty()) {
+ return PreAggStatus.off(
+ String.format("can't turn preAgg on for aggregate
function %s", aggFunc));
}
- // step 2: check condition expressions
+ // step 2: check condition expressions — all condition inputs must
+ // be key columns (from any table), not value columns. A global
+ // splitKeyValueSlots check handles this correctly for both the
+ // mixed-path (called with local key/value sets) and the value-only
+ // path (foreign key conditions in IF/CaseWhen).
Set<Slot> inputSlots =
ExpressionUtils.getInputSlotSet(conditionExps);
- if (!keySlots.containsAll(inputSlots)) {
+ Pair<Set<SlotReference>, Set<SlotReference>> condSplit =
+ splitKeyValueSlots(inputSlots);
+ if (!condSplit.second.isEmpty()) {
Review Comment:
[P1] Keep DISTINCT SUM off when a condition slot is unclassified
```text
Aggregate(sum(DISTINCT if(t.c > 0, r.v7, 0)))
Join(r.k1 = t.k1)
Scan(r AGG_KEYS; v7 SUM)
Aggregate(t.k1, sum(t.v7) AS c)
Scan(t AGG_KEYS)
```
The nested aggregate output `t.c` has no `OriginalColumn`. Previously the
condition check required the known key set to contain every input and stayed
OFF; now `splitKeyValueSlots` silently drops `t.c`, sees no classified value
slot, and reaches `visitSum`, which does not reject `DISTINCT`. With two
exact-full-key `r` rowsets containing `v7=1`, OFF merges storage SUM to `2`,
while ON exposes `{1,1}` and the later DISTINCT returns `1`. Please reject
`sum.isDistinct()` on this route (or preserve conservative handling of
unclassified conditions) and add a nested-aggregate IF/CASE result regression
with duplicate full keys.
--
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]
