This is an automated email from the ASF dual-hosted git repository.
morrySnow pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new 16a745d5125 [refactor](fe) Create PreAggInfoContext top-down from
aggregate instead of bottom-up from scan in SetPreAggStatus (#65237)
16a745d5125 is described below
commit 16a745d51255933cf83ea0a57e709ddbc5268022
Author: starocean999 <[email protected]>
AuthorDate: Wed Jul 8 17:20:49 2026 +0800
[refactor](fe) Create PreAggInfoContext top-down from aggregate instead of
bottom-up from scan in SetPreAggStatus (#65237)
related pr: #48502
Problem Summary:
The `SetPreAggStatus` rule previously created `PreAggInfoContext` at
`LogicalOlapScan` nodes (bottom-up): the first encountered AGG_KEYS scan
pushed a new context onto the stack, subsequent scans and intermediate
nodes
(filter/join/project) filled in information, and `LogicalAggregate`
popped
and consumed the context. This bottom-up approach had two issues:
1. **Nested aggregate information loss**: When a query contains nested
aggregates (e.g., an outer aggregate over an inner aggregate), the inner
aggregate's `visitLogicalAggregate` would pop the only context on the
stack, leaving the outer aggregate with no context for scans directly
under it.
2. **Unclear context ownership**: Context lifecycle was split across
scan
(creation) and aggregate (consumption), making the ownership boundary
implicit and fragile — intermediate plan nodes that call
`context.clear()`
via the base `visit()` method could inadvertently destroy the context.
This PR refactors the context propagation to a **top-down** model:
- `visitLogicalAggregate` now creates a fresh `PreAggInfoContext`,
pushes it
onto the stack, visits children, then pops and stores it.
- `visitLogicalOlapScan` only fills relation IDs into an existing
context;
it no longer creates contexts.
- The `retainAll` filtering is no longer needed since each aggregate
creates
its own isolated context with only the scan IDs from its subtree.
- Nested aggregates each get their own independent context, correctly
isolating preagg information per aggregate level.
---
.../nereids/rules/rewrite/SetPreAggStatus.java | 46 +++++++++++++++-------
.../nereids_rules_p0/set_preagg/set_preagg.groovy | 25 +++++++++++-
2 files changed, 54 insertions(+), 17 deletions(-)
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SetPreAggStatus.java
b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SetPreAggStatus.java
index 9b152efbb20..aef5f660056 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SetPreAggStatus.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SetPreAggStatus.java
@@ -54,10 +54,13 @@ import
org.apache.doris.nereids.trees.plans.visitor.CustomRewriter;
import org.apache.doris.nereids.trees.plans.visitor.DefaultPlanRewriter;
import org.apache.doris.nereids.util.ExpressionUtils;
import org.apache.doris.qe.ConnectContext;
+import org.apache.doris.qe.SessionVariable;
import com.google.common.base.Preconditions;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
import java.util.ArrayList;
import java.util.HashMap;
@@ -75,6 +78,7 @@ import java.util.stream.Collectors;
*/
public class SetPreAggStatus extends
DefaultPlanRewriter<Stack<SetPreAggStatus.PreAggInfoContext>>
implements CustomRewriter {
+ private static final Logger LOG =
LogManager.getLogger(SetPreAggStatus.class);
private Map<RelationId, PreAggInfoContext> olapScanPreAggContexts = new
HashMap<>();
/**
@@ -141,8 +145,11 @@ public class SetPreAggStatus extends
DefaultPlanRewriter<Stack<SetPreAggStatus.P
@Override
public Plan visit(Plan plan, Stack<PreAggInfoContext> context) {
+ // push null sentinel to stop preagg collection for children reached
via generic visitor,
+ // while keeping the aggregate's own frame intact on the stack
+ context.push(null);
Plan newPlan = super.visit(plan, context);
- context.clear();
+ context.pop();
return newPlan;
}
@@ -161,10 +168,9 @@ public class SetPreAggStatus extends
DefaultPlanRewriter<Stack<SetPreAggStatus.P
logicalOlapScan.getTable().getName()))) {
return logicalOlapScan.withPreAggStatus(PreAggStatus.on());
} else {
- if (context.empty()) {
- context.push(new PreAggInfoContext());
+ if (!context.empty() && context.peek() != null) {
+
context.peek().addRelationId(logicalOlapScan.getRelationId());
}
- context.peek().addRelationId(logicalOlapScan.getRelationId());
return logicalOlapScan;
}
} else {
@@ -175,7 +181,7 @@ public class SetPreAggStatus extends
DefaultPlanRewriter<Stack<SetPreAggStatus.P
@Override
public Plan visitLogicalFilter(LogicalFilter<? extends Plan>
logicalFilter, Stack<PreAggInfoContext> context) {
LogicalFilter plan = (LogicalFilter) super.visit(logicalFilter,
context);
- if (!context.empty()) {
+ if (!context.empty() && context.peek() != null) {
context.peek().addFilterConjuncts(plan.getExpressions());
}
return plan;
@@ -185,7 +191,7 @@ public class SetPreAggStatus extends
DefaultPlanRewriter<Stack<SetPreAggStatus.P
public Plan visitLogicalJoin(LogicalJoin<? extends Plan, ? extends Plan>
logicalJoin,
Stack<PreAggInfoContext> context) {
LogicalJoin plan = (LogicalJoin) super.visit(logicalJoin, context);
- if (!context.empty()) {
+ if (!context.empty() && context.peek() != null) {
context.peek().addJoinInfo(plan);
}
return plan;
@@ -195,7 +201,7 @@ public class SetPreAggStatus extends
DefaultPlanRewriter<Stack<SetPreAggStatus.P
public Plan visitLogicalProject(LogicalProject<? extends Plan>
logicalProject,
Stack<PreAggInfoContext> context) {
LogicalProject plan = (LogicalProject) super.visit(logicalProject,
context);
- if (!context.empty()) {
+ if (!context.empty() && context.peek() != null) {
context.peek().setReplaceMap(plan.getAliasToProducer());
}
return plan;
@@ -204,23 +210,33 @@ public class SetPreAggStatus extends
DefaultPlanRewriter<Stack<SetPreAggStatus.P
@Override
public Plan visitLogicalAggregate(LogicalAggregate<? extends Plan>
logicalAggregate,
Stack<PreAggInfoContext> context) {
+ PreAggInfoContext preAggInfoContext = new PreAggInfoContext();
+ context.push(preAggInfoContext);
Plan plan = super.visit(logicalAggregate, context);
- if (!context.isEmpty()) {
- PreAggInfoContext preAggInfoContext = context.pop();
-
preAggInfoContext.olapScanIds.retainAll(logicalAggregate.child().getInputRelations());
-
preAggInfoContext.addAggregateFunctions(logicalAggregate.getAggregateFunctions());
-
preAggInfoContext.addGroupByExpresssions(logicalAggregate.getGroupByExpressions());
- for (RelationId id : preAggInfoContext.olapScanIds) {
- olapScanPreAggContexts.put(id, preAggInfoContext);
+ PreAggInfoContext popped = context.pop();
+ if (popped != preAggInfoContext) {
+ if (SessionVariable.isFeDebug()) {
+ Preconditions.checkState(popped == preAggInfoContext,
+ "PreAggInfoContext stack mismatch in
visitLogicalAggregate");
+ } else {
+ LOG.warn("PreAggInfoContext stack mismatch in
visitLogicalAggregate: "
+ + "expected {} but got {}. Skipping preagg for this
aggregate.",
+ preAggInfoContext, popped);
+ return plan;
}
}
+ popped.addAggregateFunctions(logicalAggregate.getAggregateFunctions());
+
popped.addGroupByExpresssions(logicalAggregate.getGroupByExpressions());
+ for (RelationId id : popped.olapScanIds) {
+ olapScanPreAggContexts.put(id, popped);
+ }
return plan;
}
@Override
public Plan visitLogicalRepeat(LogicalRepeat<? extends Plan> repeat,
Stack<PreAggInfoContext> context) {
repeat = (LogicalRepeat<? extends Plan>) super.visit(repeat, context);
- if (!context.isEmpty()) {
+ if (!context.isEmpty() && context.peek() != null) {
context.peek().addGroupingScalarFunctionExpresssion(repeat.getGroupingId().get());
context.peek().addGroupingScalarFunctionExpresssions(
repeat.getOutputExpressions().stream()
diff --git
a/regression-test/suites/nereids_rules_p0/set_preagg/set_preagg.groovy
b/regression-test/suites/nereids_rules_p0/set_preagg/set_preagg.groovy
index 888892bc9c4..f47b2b40fc2 100644
--- a/regression-test/suites/nereids_rules_p0/set_preagg/set_preagg.groovy
+++ b/regression-test/suites/nereids_rules_p0/set_preagg/set_preagg.groovy
@@ -271,7 +271,7 @@ suite("set_preagg") {
""")
contains "(preagg_t1), PREAGGREGATION: ON"
contains "(preagg_t2), PREAGGREGATION: ON"
- contains "(preagg_t3), PREAGGREGATION: OFF"
+ contains "(preagg_t3), PREAGGREGATION: ON"
}
explain {
@@ -291,7 +291,7 @@ suite("set_preagg") {
""")
contains "(preagg_t1), PREAGGREGATION: ON"
contains "(preagg_t2), PREAGGREGATION: ON"
- contains "(preagg_t3), PREAGGREGATION: OFF"
+ contains "(preagg_t3), PREAGGREGATION: ON"
}
explain {
@@ -328,4 +328,25 @@ suite("set_preagg") {
contains "(preagg_t1), PREAGGREGATION: OFF. Reason: No valid aggregate
on scan."
contains "(preagg_t1), PREAGGREGATION: ON"
}
+
+ // Aggregate over limited subquery: Limit between aggregate and scan goes
+ // through generic visitor path, which should block preagg collection
+ // without clearing the aggregate's own frame.
+ explain {
+ sql("""
+ select k1, sum(v7)
+ from (select k1, v7 from preagg_t1 limit 10) s
+ group by k1;
+ """)
+ notContains "(preagg_t1), PREAGGREGATION: ON"
+ }
+
+ // Aggregate over non-Olap relation (numbers TVF).
+ // The TVF scan goes through the generic visitor path, which should not
+ // clear the aggregate's frame and cause EmptyStackException.
+ explain {
+ sql("""
+ select count(*) from (select * from numbers("number"="10")) t;
+ """)
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
