github-actions[bot] commented on code in PR #65846:
URL: https://github.com/apache/doris/pull/65846#discussion_r3709289333
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SetPreAggStatus.java:
##########
@@ -371,51 +523,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()) {
return PreAggStatus
.off(String.format("some columns in condition %s is
not key.", conditionExps));
}
return KeyAndValueSlotsAggChecker.INSTANCE.check(aggFunc,
returnExps);
}
- private static Expression removeCast(Expression expression) {
+ /**
+ * Peel casts that are safe for MAX/MIN (order-preserving /
nondecreasing).
+ * This is a stronger check than {@link
ExpressionUtils#getExpressionCoveredBySafetyCast}
+ * because `isInjectiveCastTo` also returns true for
IntegralType→CharacterType
+ * (e.g. BIGINT→STRING), which preserves distinctness but NOT ordering
+ * (string comparison differs from numeric comparison). For MAX/MIN we
+ * must reject such casts.
+ * <p>
+ * Safe categories:
+ * <ol>
+ * <li>Injective numeric→numeric casts (widening integral or
wider-range decimal)
+ * <li>Numeric→float casts (nondecreasing even if not injective,
e.g. BIGINT→DOUBLE)
+ * </ol>
+ */
+ private static Expression peelCastForMaxMin(Expression expression) {
while (expression instanceof Cast) {
- expression = ((Cast) expression).child();
+ Cast cast = (Cast) expression;
+ DataType sourceType = cast.child().getDataType();
+ DataType targetType = cast.getDataType();
+ // Injective + numeric → safe (widening, order-preserving).
+ if (sourceType.isInjectiveCastTo(targetType) &&
targetType.isNumericType()) {
+ expression = cast.child();
+ continue;
+ }
+ // Numeric→float → nondecreasing for MAX/MIN.
+ if (sourceType.isNumericType() &&
targetType.isFloatLikeType()) {
Review Comment:
[P1] Exclude underflowing numeric-to-FLOAT casts
Use an exact-full-key `DOUBLE MAX` column loaded in separate rowsets as
older `-1e-300` and newer `+0.0`, then evaluate:
```sql
signbit(max(if(k > 0, cast(v as float), cast(0 as float))))
```
OFF runs DOUBLE storage MAX first, selecting `+0.0`, then casts it, so
`signbit` is false. ON uses direct mode and consumes the captured rowsets
older-to-newer: `-1e-300` casts to FLOAT `-0.0`; the later `+0.0` ties under
the FLOAT comparator, and MAX retains its current `-0.0`, so `signbit` is true.
MIN is symmetric with the load assignments reversed.
This cast is total and never returns NULL, so the earlier narrowing-overflow
thread does not cover it. Nondecreasing order is insufficient when the target
tie representative is observable. Please reject underflow-capable
DOUBLE/DECIMAL-to-FLOAT peeling (or require a true MAX/MIN homomorphism), and
add duplicate-full-key signed-zero result tests.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SetPreAggStatus.java:
##########
@@ -579,7 +815,7 @@ public PreAggStatus visitSum(Sum sum, List<Expression>
returnValues) {
public PreAggStatus visitMax(Max max, List<Expression>
returnValues) {
for (Expression value : returnValues) {
if (!(isAggTypeMatched(value, AggregateType.MAX) ||
isKeySlot(value)
- || value.isNullLiteral())) {
+ || value.isLiteral())) {
Review Comment:
[P1] Keep ASOF's selected side storage-merged
```text
Aggregate(max(if(l.grp > 0, r.v9, 0)))
ASOF LEFT JOIN MATCH(l.ts >= r.ts) ON l.grp = r.grp
Scan(l)
Scan(r AGG_KEYS(grp, ts); v9 MAX)
```
Load the same `r` full key twice in separate rowsets with `v9=100` and
`v9=200`. OFF merges `r.v9` to `200` before the join. With this new
non-null-literal eligibility, `r` becomes ON; ASOF then sees two equal-time
rows and selects one before the upper MAX runs (the ASOF index explicitly
permits any equal-time row), so selecting the `100` partial returns `100`
instead of `200`. MIN is symmetric.
This differs from the earlier cast concern: `r.v9` is a direct correctly
typed MAX column; the non-commuting step is ASOF's one-row selection, not a
cast. Please carry ASOF join type/selected-side information into this decision
and keep that side OFF (or conservatively fence ASOF inputs), with
duplicate-full-key MAX/MIN result tests.
--
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]
