github-actions[bot] commented on code in PR #65846:
URL: https://github.com/apache/doris/pull/65846#discussion_r3702040455
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SetPreAggStatus.java:
##########
@@ -371,51 +514,117 @@ 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.
+ for (Expression returnExp : returnExps) {
+ if (returnExp instanceof SlotReference &&
!outputSlots.contains(returnExp)) {
Review Comment:
[P2] Preserve duplicate-insensitive foreign return branches
```text
Aggregate(max(if(l.k1 > 0, l.v9, r.v9)))
Join(l.k1 = r.k1)
Scan(l AGG_KEYS; v9 MAX)
Scan(r AGG_KEYS; v9 MAX)
```
In `checkAggWithKeyAndValueSlots`, the `l` decision rejects `r.v9` and the
`r` decision rejects `l.v9`, so both scans become OFF. For each joined full key
the condition is stable, though, and MAX over the partial-row Cartesian product
is the same as MAX after each side's storage MAX; repeated foreign values
cannot change MAX/MIN. The ownership fence is necessary for SUM, but it is
over-conservative for these idempotent aggregates. Please make it
aggregate-aware and add a positive MAX/MIN join case.
##########
regression-test/suites/nereids_rules_p0/set_preagg/set_preagg.groovy:
##########
@@ -69,6 +69,32 @@ suite("set_preagg") {
properties("replication_num" = "1");
"""
+ // One-time data setup. All suites below rely on this single dataset so the
+ // whole file is self-contained and re-runnable. The data deliberately
+ // includes repeated aggregate-key material:
+ // - preagg_t1 has two rows with k1=1 / k1=-1 (abs(k1) maps both to the
+ // same derived key 1) to exercise derived-key fan-out.
+ // - k6 takes 1/2/0 so IF(k6 > 0, ...) conditions have both true/false
rows.
+ // - v7/v8 are SUM columns, v9 is a MAX column.
+ sql """
Review Comment:
[P1] Execute the setup as separate statements
`sql` forwards this whole string to one prepared-statement execution; it
does not split on semicolons. The standard regression JDBC URLs do not enable
`allowMultiQueries`, so this setup fails at the first statement boundary and
none of the new EXPLAIN/result assertions run. The DDL block above already uses
`multi_sql`, whose implementation performs the required split. Please use
`multi_sql` here (or separate `sql` calls) and regenerate the output from the
runnable suite.
--
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]
