github-actions[bot] commented on code in PR #65846:
URL: https://github.com/apache/doris/pull/65846#discussion_r3643114331
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SetPreAggStatus.java:
##########
@@ -94,8 +94,22 @@ public static class PreAggInfoContext {
private Map<Slot, Expression> replaceMap = new HashMap<>();
- private void setReplaceMap(Map<Slot, Expression> replaceMap) {
- this.replaceMap = replaceMap;
+ private void setReplaceMap(Map<Slot, Expression> newReplaceMap) {
+ // merge instead of replace: sibling projects under a join share
one
+ // PreAggInfoContext, and a full replacement would lose mappings
from
+ // the sibling. merge keeps all entries; new entries shadow old
ones
+ // by putAll semantics so a chain of projects still resolves
correctly.
+ //
+ // Before merging, resolve the new aliases' producers through the
+ // existing replaceMap so that upper-layer aliases reference base
table
+ // columns directly instead of intermediate computed aliases.
+ Map<Slot, Expression> merged = new HashMap<>(this.replaceMap);
Review Comment:
[P2] Avoid copying the accumulated alias map at every project
Every `LogicalProject`, including an alias-free one, reaches this method,
and this copy grows with all aliases seen in earlier sibling/nested projects. A
plan with N projected branches therefore copies 0+1+...+(N-1) entries and
allocates N increasingly large maps during this rule. The context owns
`replaceMap` and no snapshot escapes, so please resolve only the current
entries into a small temporary map against the unchanged old map, then `putAll`
them into `this.replaceMap` (and return immediately when the new map is empty).
That preserves the intended old-map-only resolution semantics while keeping
accumulation linear.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SetPreAggStatus.java:
##########
@@ -326,28 +340,49 @@ 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);
Review Comment:
[P1] Validate mixed aggregates relative to the scan being enabled
The merged map now survives the alias-free project that aggregate
normalization inserts, exposing a scan-ownership bug in this global key/value
split. A reduced plan is:
```text
Aggregate(sum(if(a > 0, r.v7, 0)))
Project(a, r.v7)
Join(a = r.k1)
Project(abs(l.k1) AS a) -> Scan(l AGG_KEYS)
Scan(r AGG_KEYS)
```
The new map composition restores `a -> abs(l.k1)`. While deciding `l`,
candidate selection therefore sees local `l.k1`, this split sees that key plus
foreign SUM column `r.v7`, and the mixed helper accepts both without checking
ownership. If `l` has two partial rows for one logical key and `r.v7=10`,
leaving `l` ON joins the right row twice and returns 20 instead of 10. Please
make mixed validation relative to the current scan—do not use a foreign
aggregate value to justify exposing this scan's partial rows—and add a
two-table derived-key regression with repeated aggregate-key data.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SetPreAggStatus.java:
##########
@@ -326,28 +340,49 @@ 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 = PreAggStatus.off(
+ String.format("can't turn preAgg on for
aggregate function %s", aggFunc));
+ }
+ } else if (splitSlots.second.isEmpty()) {
+ // only key slots
+ // Volatile expressions (e.g., random()) must be
rejected here:
+ // with pre-agg ON they would be evaluated per partial
row instead of
+ // per merged logical row, changing DISTINCT/MAX/MIN
semantics.
+ if (aggFunc.containsVolatileExpression()) {
Review Comment:
[P1] Check volatility before per-scan candidate filtering
This guard is reached only for aggregate functions selected for the current
scan, and it never covers the separately collected filter/join/grouping lists.
That leaves two result-changing bypasses. For example:
```text
Aggregate(max(r.k1 + random()))
Join(l.k1 = r.k1)
Scan(l AGG_KEYS)
Scan(r AGG_KEYS)
```
The MAX turns `r` OFF, but for `l` it is whitelisted as an otherwise
duplicate-insensitive other-table aggregate; the candidate set is empty and
`createPreAggStatus` returns ON before this check. Partial `l` rows then
duplicate the joined `r` row and create extra random samples. Likewise,
`Filter(random() < 0.5) -> Scan` passes because its input-slot set is empty, so
two partial SUM rows can independently yield 1 or 2 instead of only 0 or their
merged sum. Volatile grouping aliases and join conjuncts follow the same
slot-only path. Please enforce row stability centrally—before per-scan
candidate filtering—for the alias-expanded aggregate, filter, join, and
grouping expressions, and add cases that assert both scans are OFF.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SetPreAggStatus.java:
##########
@@ -94,8 +94,22 @@ public static class PreAggInfoContext {
private Map<Slot, Expression> replaceMap = new HashMap<>();
- private void setReplaceMap(Map<Slot, Expression> replaceMap) {
- this.replaceMap = replaceMap;
+ private void setReplaceMap(Map<Slot, Expression> newReplaceMap) {
+ // merge instead of replace: sibling projects under a join share
one
+ // PreAggInfoContext, and a full replacement would lose mappings
from
+ // the sibling. merge keeps all entries; new entries shadow old
ones
+ // by putAll semantics so a chain of projects still resolves
correctly.
+ //
+ // Before merging, resolve the new aliases' producers through the
+ // existing replaceMap so that upper-layer aliases reference base
table
+ // columns directly instead of intermediate computed aliases.
+ Map<Slot, Expression> merged = new HashMap<>(this.replaceMap);
+ for (Map.Entry<Slot, Expression> entry : newReplaceMap.entrySet())
{
+ Expression resolvedProducer = ExpressionUtils.replace(
Review Comment:
[P1] Preserve legal project chains when alias expansion hits expression
limits
This eager composition can reconstruct an expression that the normal project
merger deliberately leaves split. For example:
```text
Aggregate(count(DISTINCT x14))
Project(x13 + x13 AS x14)
...
Project(k1 + k1 AS x1)
Scan(preagg_t1)
```
With fourteen `xN = x(N-1) + x(N-1)` layers, every project expression has
width two. `PlanUtils.tryMergeProjections` catches `EXPRESSION_EXCEEDS_LIMIT`
and preserves the legal chain, but this loop expands widths 2, 4, ..., 16384;
rebuilding through `withChildren` exceeds the default 10000-child limit and
throws here. The old non-composing map did not materialize that tree. Please
keep lineage compact, or honor the same limit and conservatively turn
preaggregation OFF instead of failing the query, and add a nested-project
planning regression.
--
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]
