Refactor hash join build-report lifecycle into BuildReportHandle#22623
Open
kosiew wants to merge 4 commits into
Open
Refactor hash join build-report lifecycle into BuildReportHandle#22623kosiew wants to merge 4 commits into
BuildReportHandle#22623kosiew wants to merge 4 commits into
Conversation
- Added BuildReportHandle to streamline operations - Centralized the schedule, deliver, cancel, and finalize lifecycle processes - Moved OnceFut waiter functionality into the handle for better management - Simplified the drop and wait path for HashJoinStream - Introduced three focused lifecycle tests for improved coverage - Added test-only helpers in shared_bounds for stream lifecycle tests - Reused existing accumulator test setup for efficiency
- Updated `wait_for_delivery()` to directly set `Delivered` after a successful `fut.get_shared(cx)`. - Removed the `mark_delivered()` helper to prevent independent marking of delivered states in tests and internal code. - Eliminated redundant stream-level drop cleanup entry point; cleanup is now handled by `BuildReportHandle`’s Drop implementation. - Updated the delivered-path test for improved verification of the waiter flow: - Changed test to use a one-partition accumulator. - Polls `wait_for_delivery()` with a noop waker/context. - Asserts that the completion count is 1 to confirm expected behavior without extra cancellation side effects.
…tream modules - shared_bounds.rs: - Removed unused top-level test helper - Removed forwarding test wrapper - Kept only cross-module helpers at the top level - stream.rs: - Added partitioned_handle test helper - Removed repeated BuildReportHandle::new(...) boilerplate - Simplified noop context creation - Documented defensive no-accumulator fallback in schedule
- Renamed `wait_for_delivery` to `poll_delivery` - Renamed `cancel_if_pending` to `cancel_pending` - Implemented `Debug`, `PartialEq`, and `Eq` for `BuildReportState` - Added test-only `state()` accessor - Included state assertions in tests - Added no-accumulator finalize test - Updated idempotency test name to align with new method naming
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
The build-report lifecycle for hash-join partitions was previously spread across
HashJoinStream,OnceFutpolling, and drop-time cancellation logic. Although correctness around scheduled versus delivered reports had already been addressed, the lifecycle ownership remained fragmented and difficult to reason about.This change centralizes lifecycle management into a dedicated abstraction, making state transitions explicit and ensuring drop-time behavior is self-contained and easier to maintain.
What changes are included in this PR?
Introduce a new
BuildReportHandletype that owns the lifecycle of a partition's build-data report.Consolidate report lifecycle state management into explicit states:
NotReportedScheduledDeliveredCanceledFinalizedMove report scheduling, delivery tracking, cancellation, and finalization logic out of
HashJoinStreamand intoBuildReportHandle.Implement drop-safe behavior in
BuildReportHandleso pending scheduled reports are canceled when dropped before delivery.Replace the stream's separate accumulator, waiter, and lifecycle state fields with a single
build_reporthandle.Add test-only helpers in
shared_bounds.rsto construct partitioned accumulators and inspect completed partition counts.Update lifecycle documentation to reflect the new centralized ownership model and state transitions.
Are these changes tested?
Yes.
New tests were added covering lifecycle behavior and invariants:
report_canceled_partition_is_noop_after_reportreport_canceled_partition_marks_pending_partition_canceledbuild_report_handle_cancels_scheduled_partition_on_dropbuild_report_handle_does_not_cancel_delivered_partition_on_dropbuild_report_handle_cancel_pending_is_idempotentbuild_report_handle_no_accumulator_finalizesThese tests verify correct cancellation behavior, delivery tracking, terminal-state handling, and idempotency.
Are there any user-facing changes?
No. This is an internal refactoring of hash-join build-report lifecycle management and does not change user-facing behavior.
LLM-generated code disclosure
This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed and tested.