github-actions[bot] commented on code in PR #65237:
URL: https://github.com/apache/doris/pull/65237#discussion_r3526791378
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SetPreAggStatus.java:
##########
@@ -204,15 +203,16 @@ public Plan visitLogicalProject(LogicalProject<? extends
Plan> logicalProject,
@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);
- }
Review Comment:
This can break valid aggregates whose child reaches any generic visitor
path. `visitLogicalAggregate` now pushes a frame and then calls `super.visit`,
but unhandled nodes still go through `SetPreAggStatus.visit(Plan, Stack)`,
which clears the entire stack after visiting children. That includes
limit/topN/sort barriers: after `LimitSortToTopN`/`SplitLimit`, `select k1,
sum(v7) from (select k1, v7 from preagg_t1 limit 10) s group by k1` reaches
this code as aggregate -> project -> limit -> limit -> scan, so the limit visit
clears the frame and this `context.pop()` throws. It also includes non-Olap
leaves such as `LogicalFileScan`/`LogicalTVFRelation`, which delegate through
the relation visitor to the same generic `visit`, so aggregates over
external/TVF relations can fail without a limit. Please make the clear scoped
so generic children stop preagg collection without removing the aggregate's own
frame, and add regressions for both an aggregate-over-limited-subquery and an
aggregate
over a non-Olap relation.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SetPreAggStatus.java:
##########
@@ -204,15 +203,16 @@ public Plan visitLogicalProject(LogicalProject<? extends
Plan> logicalProject,
@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();
Review Comment:
This can break valid aggregates whose child reaches any generic visitor
path. `visitLogicalAggregate` now pushes a frame and then calls `super.visit`,
but unhandled nodes still go through `SetPreAggStatus.visit(Plan, Stack)`,
which clears the entire stack after visiting children. That includes
limit/topN/sort barriers: after `LimitSortToTopN`/`SplitLimit`, `select k1,
sum(v7) from (select k1, v7 from preagg_t1 limit 10) s group by k1` reaches
this code as aggregate -> project -> limit -> limit -> scan, so the limit visit
clears the frame and this `context.pop()` throws. It also includes non-Olap
leaves such as `LogicalFileScan`/`LogicalTVFRelation`, which delegate through
the relation visitor to the same generic `visit`, so aggregates over
external/TVF relations can fail without a limit. Please make the clear scoped
so generic children stop preagg collection without removing the aggregate's own
frame, and add regressions for both an aggregate-over-limited-subquery and an
aggregate
over a non-Olap relation.
--
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]
