starocean999 opened a new pull request, #65237: URL: https://github.com/apache/doris/pull/65237
related pr: https://github.com/apache/doris/pull/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. ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: https://github.com/apache/doris-website/pull/1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into --> -- 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]
