Skip to content

[Bug](rec-cte) reset _require_bucket_distribution in PipelineFragmentContext::rebuild #60618

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
BiteTheDDDDt merged 1 commit into from
Feb 10, 2026
Merged

[Bug](rec-cte) reset _require_bucket_distribution in PipelineFragmentContext::rebuild #60618

BiteTheDDDDt merged 1 commit into from
Feb 10, 2026

Conversation

@BiteTheDDDDt
Copy link
Contributor

@BiteTheDDDDt BiteTheDDDDt commented Feb 9, 2026 •
edited
Loading

What problem does this PR solve?

reset _require_bucket_distribution in PipelineFragmentContext::rebuild
QQ_1770630327880

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

Copilot AI review requested due to automatic review settings February 9, 2026 09:45
@BiteTheDDDDt
Copy link
Contributor Author

BiteTheDDDDt commented Feb 9, 2026

run buildall

Copilot AI reviewed Feb 9, 2026
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Resets PipelineFragmentContext state during rebuild() to prevent recursive-CTE fragment reruns from inheriting stale bucket distribution requirements that can change subsequent pipeline construction (notably around local shuffle).

Changes:

  • Reset _require_bucket_distribution to false in PipelineFragmentContext::rebuild() before rebuilding the full pipeline.
  • Add an explanatory comment describing why this reset is necessary for correct pipeline building across reruns.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

be/src/pipeline/pipeline_fragment_context.cpp
Comment on lines 2090 to 2094
_submitted = false;
_is_fragment_instance_closed = false;
// _require_bucket_distribution may be set to true to affect the building of pipeline with local shuffle
_require_bucket_distribution = false;
return _build_and_prepare_full_pipeline(thread_pool);
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

This change fixes state carried across rebuild(), but there doesn’t appear to be any unit test coverage for the rerun/rebuild path (e.g., a rec-CTE round triggering wait/release/rebuild/submit). Consider adding a BE pipeline unit test that exercises at least two rebuild cycles and asserts the pipeline build outcome isn’t affected by stale _require_bucket_distribution from the prior build (e.g., local shuffle/bucket distribution decisions differ between rounds).

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@BiteTheDDDDt BiteTheDDDDt Feb 9, 2026

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

@hello-stephen
Copy link
Contributor

hello-stephen commented Feb 9, 2026

BE UT Coverage Report

Increment line coverage 0.00% (0/1) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 52.72% (19445/36882)
Line Coverage 36.21% (181010/499901)
Region Coverage 32.60% (140549/431169)
Branch Coverage 33.62% (60876/181049)

@hello-stephen
Copy link
Contributor

hello-stephen commented Feb 9, 2026

BE Regression && UT Coverage Report

Increment line coverage 100.00% (1/1) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 73.37% (26515/36138)
Line Coverage 56.46% (281527/498663)
Region Coverage 54.17% (235938/435551)
Branch Coverage 55.74% (101318/181753)

@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Feb 10, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Feb 10, 2026

PR approved by at least one committer and no changes requested.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 10, 2026

PR approved by anyone and no changes requested.

@BiteTheDDDDt BiteTheDDDDt merged commit 853f5b1 into apache:master Feb 10, 2026
39 of 43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@Gabriel39 Gabriel39 Gabriel39 approved these changes

@starocean999 starocean999 starocean999 approved these changes

Assignees

No one assigned

Labels

approved Indicates a PR has been approved by one committer. rec_cte reviewed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@BiteTheDDDDt @hello-stephen @Gabriel39 @starocean999