Repository navigation
Conversation
…c filter The partitioned hash join dynamic filter coordinator marks a partition as canceled when its stream is dropped before it reports. The name suggests that the whole query was canceled. That reading is wrong: a parent operator can finish early and drop one partition while sibling partitions keep running (apache#21625). This commit documents `PartitionStatus::CanceledUnknown`, `report_canceled_partition`, `BuildReportHandle`, and `BuildReportState::Canceled` to say what a canceled partition is and why its `true` filter contribution is correct. No behavior change. Co-Authored-By: Claude Opus 5.5 <[email protected]>
tschwarzinger
approved these changes
Nov 4, 2026
tschwarzinger
left a comment
tschwarzinger
left a comment
Contributor
There was a problem hiding this comment.
I stumbled across the notion of canceled partitions while looking at #26249 and was a bit confused about what this means.
This answers my questions, so I think this is a great change!
Contributor
Author
|
Thanks @tschwarzinger! I'm still doing some more investigation to see if we can make the wording better and fully understand all of the cases under which this happens. Will update for another look once I think this is ready. |
The previous commit explained what a canceled partition means in four doc comments. This keeps one full explanation on `PartitionStatus::CanceledUnknown` and points to it from the other three. The explanation now also says why a canceled partition matters to no consumer: its stream emitted no row, because a stream probes only after its report is delivered. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The `CanceledUnknown` comment said a parent join drops "a probe-side stream" and that the filter keeps rows routed to a canceled partition. The first reads as if a whole side could be canceled. A parent join partition drops only this join's stream for the same partition. The second describes a choice of the filter builder, not of the status, and no consumer ever sees those rows. This states the first precisely, drops the second, and says why a canceled stream emits no row: delivery of its report waits for the dynamic filter to be final. Co-Authored-By: Claude Opus 5.5 <[email protected]>
adriangb
commented
Nov 4, 2026
| /// Owns the stream-side lifecycle for one partition's build-data report. | ||
| /// | ||
| /// In `Partitioned` mode, dropping the handle before delivery reports the | ||
| /// partition as canceled. `PartitionStatus::CanceledUnknown` says what that |
Contributor
Author
There was a problem hiding this comment.
This is referencing a doc in another module. But that Doc is orthogonally related: PartitionStatus is for shared_bounds (which is dynamic filter building). This struct is for the build side partition management in general. IMO we should do something like:
BuildReportState,BuildReportHandlehold information on what it means for a partition to be cancelled in general. We stop the build side immediately, any row routed to that build side will be dropped, but we still run all partitions on the probe side (unless all build sides were cancelled) since we don't know what probe side partitions will route to which build side partitions.PartitionStatusshould clarify what this means for dynamic filters.
…t the status Review pointed out that the stream docs sent readers to `PartitionStatus::CanceledUnknown`, which belongs to dynamic filter building, for what cancellation means to the join in general. `BuildReportHandle` now explains cancellation for the join: which stream a parent drops, that sibling partitions and their shared probe input keep running, that a `RepartitionExec` discards the rows routed to the canceled partition, and why the stream emits no row. `CanceledUnknown` now explains only what it means for the dynamic filter: no row routed to the partition reaches a consumer, so the filter may keep or reject those rows. Co-Authored-By: Claude Opus 5.5 <[email protected]>
alamb
approved these changes
Nov 4, 2026
| /// | ||
| /// The partition counts as complete, so sibling partitions do not wait for | ||
| /// its report. Its build keys are unknown, but no probe row routed to it | ||
| /// reaches a consumer: the stream probes only after delivery, and delivery |
Contributor
There was a problem hiding this comment.
I found "a consumer: the stream probes only after delivery, and delivery
/// waits for this filter to be final" confusing
Also if no probe rows are routed to it, why doesn't the join canclel?
Contributor
There was a problem hiding this comment.
Is the idea that the output limit was hit or osmething so the join just needs to shutdown cleanly?
The cancellation docs used internal terms such as report delivery, so a reader outside this operator could not follow them. They now define a canceled partition by what a reader can observe: a parent operator drops the stream before it reads its probe side. The handle doc lists what keeps running, and the filter doc says only why the filter may keep or reject the rows routed to that partition. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Contributor
Author
|
Thanks folks. I've tweaked the wording a bit. I'm happy enough with it. Since this is a docs only PR I'll merge it, happy to tweak more later. |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Nov 11, 2026
adriangb
deleted the
hash-join-dynamic-filter-canceled-partition-docs
branch
November 11, 2026 21:02
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
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 partitioned hash join dynamic filter coordinator marks a partition as "canceled" when its stream is dropped before it reports its build data. A reader can assume that this means the whole query was canceled. That assumption is wrong. A parent operator can legally finish early and drop one hash join partition stream while sibling partitions keep running. In #21625 (the TPC-H Q18 hang), a
RightSemijoin with an empty build side for some partitions does exactly this. An editor who relies on the wrong reading could reintroduce that hang.What changes are included in this PR?
Comment-only changes. The docs define a canceled partition by what a reader can observe: a parent operator drops one partition's stream before that stream reads its probe side. They avoid internal terms such as report delivery.
BuildReportHandleexplains what cancellation means for the join. The other partitions and their shared probe input keep running, aRepartitionExecdiscards the rows routed to the canceled partition, and the canceled stream produces no output. The other partitions stop waiting for the canceled one.PartitionStatus::CanceledUnknownexplains only what it means for the dynamic filter. The partition's build keys are unknown, and the filter may keep or reject the rows routed to it, because no stream joins those rows.BuildReportState::CanceledandSharedBuildAccumulator::report_canceled_partitionpoint to those two docs. One em-dash in an existing comment is replaced.The last commit rewrites the earlier wording in plain terms, so reviewing the full diff is easier than reviewing commit by commit.
What is the testing strategy for this PR?
The changes are comment-only, so they add no tests.
cargo fmtandcargo clippy -p datafusion-physical-plan --all-targets --all-features -- -D warningspass.Are there any user-facing changes?
No.
🤖 Generated with Claude Code