Allow Range partitioned inputs to PartitionedTopK#23355
Merged
Conversation
1b1c490 to
e236f7b
Compare
eb2c841 to
3ab476d
Compare
Contributor
|
This is in my queue, just waiting for #23184 to land 🙇 |
3ab476d to
7d0f380
Compare
gene-bordegaray
left a comment
Contributor
There was a problem hiding this comment.
Looks goodl, siilar to #23416 lets keep the testing set in slt leaner and not stress the full preserve_file_partitions and subset_repartition_threshold matrix. I will make a PR to have this matrix tested in the unit 👍
I think for slt tests keeping it to a simple positive / negative, subset positive / negative is good, lmk what you think 🙇
Thank you
7d0f380 to
ab90d5c
Compare
Contributor
Author
|
Thanks @gene-bordegaray : have rebased atop #23416 (only the top 3 commits are relevant) and applied your feedback. I'll put this back in draft until #23416 has landed. |
ab90d5c to
c92419e
Compare
stuhood
pushed a commit
to paradedb/datafusion
that referenced
this pull request
Jul 14, 2026
…rtitioned requirements (apache#23416) ## Which issue does this PR close? - Closes apache#23289. Rebased on main now that apache#23184 has merged. Overlaps with apache#23355, which also opts the window execs into range satisfaction as part of its PartitionedTopK work — happy to rebase whichever lands second. ## What changes are included in this PR? In one commit: * Opt `WindowAggExec` and `BoundedWindowAggExec` with partition keys into range satisfaction via `InputDistributionRequirements::allow_range_satisfaction_for_key_partitioning`, mirroring `AggregateExec`. Compatible range-partitioned inputs then satisfy the window key requirement without a hash repartition; subset satisfaction and the hash fallback for incompatible keys come from the existing satisfaction machinery. Windows without `PARTITION BY` keep requiring a single partition. ## Are these changes tested? Yes: new `slt` tests in `range_partitioning.slt` (exact and subset reuse, rehash on incompatible keys, `subset_repartition_threshold` / `preserve_file_partitions` / `target_partitions` behavior, `WindowAggExec` via an unbounded frame, and no-`PARTITION BY`), plus plan-shape tests in `enforce_distribution.rs` and acceptance/rejection tests in `sanity_checker.rs`. ## Are there any user-facing changes? No: only plan changes.
Contributor
Author
|
@gene-bordegaray: Ready for another look! Thanks. |
gene-bordegaray
approved these changes
Jul 14, 2026
gene-bordegaray
left a comment
Contributor
There was a problem hiding this comment.
Great, thanks. Just some small nits 👍
cc: @gabotechs
gabotechs
approved these changes
Jul 15, 2026
gabotechs
left a comment
Contributor
There was a problem hiding this comment.
Thanks @stuhood and @gene-bordegaray!
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
3 participants
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?
Partitioning::Rangeto satisfy PartitionedTopKDistribution::KeyPartitionedrequirements #23290.What changes are included in this PR?
In two commits:
WindowTopNto intermediateRepartitionExecnodes, to ensure thatPartitionedTopKcan still be used whentarget_partitionsmismatches physical partitions.required_input_distributionsto declare required partitioning forPartitionedTopKExec,BoundedWindowAggExec, andWindowAggExec.Are these changes tested?
Yes, new
slttests are added for both theWindowTopNrobustness fix, and for the use ofrequired_input_distributionsinPartitionedTopK.Are there any user-facing changes?
No: only plan changes.