Skip to content

[refactor](fe) Create PreAggInfoContext top-down from aggregate instead of bottom-up from scan in SetPreAggStatus#65237

Draft
starocean999 wants to merge 2 commits into
apache:masterfrom
starocean999:master_0706
Draft

[refactor](fe) Create PreAggInfoContext top-down from aggregate instead of bottom-up from scan in SetPreAggStatus#65237
starocean999 wants to merge 2 commits into
apache:masterfrom
starocean999:master_0706

Conversation

@starocean999

Copy link
Copy Markdown
Contributor

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.

Check List (For Author)

  • Test

    • 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
  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@starocean999

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for one planner correctness regression in SetPreAggStatus.

Critical checkpoints:

  • Goal/test: the PR tries to refactor preagg context creation from scan-side to aggregate-side. The changed tests cover the intended right-join behavior, but the new traversal does not preserve the aggregate-owned stack frame through all child paths.
  • Scope: the code change is small and focused, but it misses parallel generic visitor paths.
  • Concurrency/lifecycle/config/persistence/FE-BE/storage: not materially involved by this patch.
  • Parallel paths: limit/topN/sort barriers and non-Olap relation leaves still use the generic visitor, which clears the stack and can make the aggregate pop fail.
  • Tests: static review only. This checkout is missing .worktree_initialized, thirdparty/installed, and thirdparty/installed/bin/protoc, so I could not run FE build or regression tests. The current regression updates do not cover the failing generic-child cases.
  • User focus: no additional review focus was provided.

Subagent conclusions: optimizer-rewrite reported OR-1 and OR-2; both were independently verified and merged into the single inline comment M-1. tests-session-config found no additional test/session/config candidate. Convergence round 2 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the same final ledger/comment set.

No existing inline review comments were present, so no duplicate GitHub thread was suppressed.

@starocean999

Copy link
Copy Markdown
Contributor Author

/review

@starocean999

Copy link
Copy Markdown
Contributor Author

run buildall

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review completed for head 384c5c65fbecc8a221d2ad1c27299207f83e2f4d.

I did not find a new blocking issue that needs an additional inline comment. The previously raised generic-visitor stack-clearing concern is already covered by the existing review thread, and the current head appears to address that same issue by making the generic visitor use a scoped null sentinel while preserving the aggregate-owned frame, with added regression coverage for aggregate-over-limit and aggregate-over-TVF shapes.

Critical checkpoint conclusions:

  • Goal and tests: the PR refactors SetPreAggStatus context ownership from scan-created bottom-up state to aggregate-owned top-down state, and the added regressions cover the prior generic visitor failure shapes.
  • Scope: the patch is focused on SetPreAggStatus.java and the matching set_preagg regression suite.
  • Concurrency/lifecycle/persistence/config: no new concurrency, lifecycle, persistence, or configuration surface was introduced.
  • Parallel paths: nested aggregate, join/filter/project/repeat collection, generic barrier nodes, and non-Olap relation paths were checked against the visitor dispatch and rule placement.
  • Compatibility/protocol: no FE-BE protocol or persisted format change was introduced.
  • Validation: GitHub shows CheckStyle, formatting, and lightweight checks passing at review time; local FE/regression execution was not possible because this checkout lacks .worktree_initialized, thirdparty/installed, and thirdparty/installed/bin/protoc.

Subagent conclusions:

  • optimizer-rewrite: no new valuable finding beyond the duplicate-covered generic visitor issue.
  • tests-session-config: no new valuable test/session/config/style finding; recommended relying on available CI because local protoc prerequisites are missing.
  • Final convergence round 1 ended with all live subagents reporting NO_NEW_VALUABLE_FINDINGS.

User focus: no additional user-provided review focus was supplied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants