github-actions[bot] commented on code in PR #65846:
URL: https://github.com/apache/doris/pull/65846#discussion_r3642693136
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SetPreAggStatus.java:
##########
@@ -402,6 +407,10 @@ private PreAggStatus
checkAggWithKeyAndValueSlots(AggregateFunction aggFunc,
// currently, only IF and CASE WHEN are supported
returnExps.add(removeCast(child));
}
+ if (conditionExps.isEmpty()) {
Review Comment:
[P1] Validate every DISTINCT argument before using the unary helper
This guard does not reject a legal variadic shape when the first child is
`IF`/`CASE`:
```text
LogicalAggregate(count(DISTINCT if(k1 > 0, k2, 0), v7))
LogicalOlapScan(preagg_t1 AGG_KEYS)
```
`splitKeyValueSlots` sees keys plus `v7`, but `checkAggWithKeyAndValueSlots`
reads only `child(0)`. The `IF` supplies a nonempty key-only condition and
key/zero returns, so `visitCount` returns ON without ever checking the later
`v7`. With partial rows for one full key carrying `v7=1` and `v7=2`, ON exposes
two distinct tuples, whereas OFF merges them to the logical `v7=3` tuple and
returns one. Please keep this helper unary (or validate every argument
independently) and add IF/CASE-first negative cases with a later value column.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SetPreAggStatus.java:
##########
@@ -326,28 +326,33 @@ private PreAggStatus
checkAggregateFunctions(Set<AggregateFunction> aggregateFun
}
PreAggStatus preAggStatus = PreAggStatus.on();
for (AggregateFunction aggFunc : aggregateFuncs) {
- if (aggFunc.children().isEmpty()) {
+ Set<Slot> aggSlots = aggFunc.getInputSlots();
+ if (aggSlots.isEmpty()) {
preAggStatus = PreAggStatus.off(
String.format("can't turn preAgg on for aggregate
function %s", aggFunc));
- } else if (aggFunc.children().size() == 1 && aggFunc.child(0)
instanceof Slot) {
- Slot aggSlot = (Slot) aggFunc.child(0);
- if (aggSlot instanceof SlotReference
- && ((SlotReference)
aggSlot).getOriginalColumn().isPresent()) {
- if (((SlotReference)
aggSlot).getOriginalColumn().get().isKey()) {
- preAggStatus =
OneKeySlotAggChecker.INSTANCE.check(aggFunc);
+ } else {
+ Pair<Set<SlotReference>, Set<SlotReference>> splitSlots =
splitKeyValueSlots(aggSlots);
+ if (splitSlots.first.isEmpty()) {
+ // only value slots
+ if (aggFunc.children().size() == 1 && aggFunc.child(0)
instanceof SlotReference) {
+ SlotReference slotRef = (SlotReference)
aggFunc.child(0);
+ if (slotRef.getOriginalColumn().isPresent()) {
+ preAggStatus =
OneValueSlotAggChecker.INSTANCE.check(aggFunc,
+
slotRef.getOriginalColumn().get().getAggregationType());
+ } else {
+ preAggStatus = PreAggStatus.off(
+ String.format("can't turn preAgg on
for aggregate function %s", aggFunc));
+ }
} else {
- preAggStatus =
OneValueSlotAggChecker.INSTANCE.check(aggFunc,
- ((SlotReference)
aggSlot).getOriginalColumn().get().getAggregationType());
+ preAggStatus = PreAggStatus.off(
+ String.format("can't turn preAgg on for
aggregate function %s", aggFunc));
}
+ } else if (splitSlots.second.isEmpty()) {
+ // only key slots
+ preAggStatus =
KeySlotAggChecker.INSTANCE.check(aggFunc);
Review Comment:
[P1] Keep volatile key expressions out of the key-only ON path
The input-slot set is not enough to prove that the aggregate argument is
stable:
```text
LogicalAggregate(count(DISTINCT k6 + random()))
LogicalOlapScan(preagg_t1 AGG_KEYS)
```
Aggregate normalization may put the expression in a project, but
`PreAggInfoContext` substitutes its producer back for classification, so
`getInputSlots()` contains only `k6` and this branch reaches
`KeySlotAggChecker`. Its generic DISTINCT handling returns ON without checking
volatility, while the project still evaluates above the scan on whatever rows
storage returns. ON may therefore evaluate `random()` once per partial row
instead of once per fully merged logical row and change the distinct count;
`max/min(k6 + random())` have the same issue. Please reject
volatile/non-row-stable expressions before enabling this path and add negative
EXPLAIN coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SetPreAggStatus.java:
##########
@@ -326,28 +326,33 @@ private PreAggStatus
checkAggregateFunctions(Set<AggregateFunction> aggregateFun
}
PreAggStatus preAggStatus = PreAggStatus.on();
for (AggregateFunction aggFunc : aggregateFuncs) {
- if (aggFunc.children().isEmpty()) {
+ Set<Slot> aggSlots = aggFunc.getInputSlots();
+ if (aggSlots.isEmpty()) {
preAggStatus = PreAggStatus.off(
String.format("can't turn preAgg on for aggregate
function %s", aggFunc));
- } else if (aggFunc.children().size() == 1 && aggFunc.child(0)
instanceof Slot) {
- Slot aggSlot = (Slot) aggFunc.child(0);
- if (aggSlot instanceof SlotReference
- && ((SlotReference)
aggSlot).getOriginalColumn().isPresent()) {
- if (((SlotReference)
aggSlot).getOriginalColumn().get().isKey()) {
- preAggStatus =
OneKeySlotAggChecker.INSTANCE.check(aggFunc);
+ } else {
+ Pair<Set<SlotReference>, Set<SlotReference>> splitSlots =
splitKeyValueSlots(aggSlots);
+ if (splitSlots.first.isEmpty()) {
+ // only value slots
+ if (aggFunc.children().size() == 1 && aggFunc.child(0)
instanceof SlotReference) {
+ SlotReference slotRef = (SlotReference)
aggFunc.child(0);
+ if (slotRef.getOriginalColumn().isPresent()) {
+ preAggStatus =
OneValueSlotAggChecker.INSTANCE.check(aggFunc,
+
slotRef.getOriginalColumn().get().getAggregationType());
+ } else {
+ preAggStatus = PreAggStatus.off(
+ String.format("can't turn preAgg on
for aggregate function %s", aggFunc));
+ }
} else {
- preAggStatus =
OneValueSlotAggChecker.INSTANCE.check(aggFunc,
- ((SlotReference)
aggSlot).getOriginalColumn().get().getAggregationType());
+ preAggStatus = PreAggStatus.off(
+ String.format("can't turn preAgg on for
aggregate function %s", aggFunc));
}
+ } else if (splitSlots.second.isEmpty()) {
Review Comment:
[P1] Reject unclassified slots before taking the key-only branch
`splitKeyValueSlots` can omit a computed alias, so `valueSlots.isEmpty()`
does not prove that all inputs are keys. For example:
```text
LogicalAggregate(count(DISTINCT a))
Project(l.k1 + l.x AS a)
Join(l.k1 = r.rk)
Project(t1.k1, t1.v7 + 1 AS x) -> Scan(t1 AGG_KEYS)
Project(abs(t2.k1) AS rk) -> Scan(t2 AGG_KEYS)
```
The join children share one `PreAggInfoContext`; the right project
overwrites the left `x -> v7 + 1` map, and the upper project restores `k1 + x`
without recovering `x`'s producer. Since computed `x` has no `OriginalColumn`,
the aggregate reaches this branch with `k1` classified as key and `x` silently
dropped, enabling ON and exposing partial `v7` values before the project.
Please require every aggregate slot to be classified (for example, return OFF
unless `aggSlots.size() == keySlots.size() + valueSlots.size()`).
Scoping/composing project maps can additionally recover eligible ON cases. Add
a two-project join regression using a derived value alias.
--
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]
