feat: split BatchPartitioner::try_new into hash and round-robin constructors #19668
Conversation
…round-robin constructors
milenkovicm
left a comment
milenkovicm
left a comment
There was a problem hiding this comment.
thanks @mohit7705
looks good, just few comments
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
BTW we will need to backport this into branch-52 if we want to include it in the release
milenkovicm
changed the title
milenkovicm
left a comment
milenkovicm
left a comment
There was a problem hiding this comment.
can we please add documentation for try_new as well please ?
|
thanks @alamb & @mohit7705 |
|
Done 👍 Added documentation for |
|
Thanks again for the reviews! 🙏 |
|
thanks @mohit7705 |
| num_partitions: usize, | ||
| timer: metrics::Time, | ||
| input_partition: usize, | ||
| num_input_partitions: usize, |
There was a problem hiding this comment.
num_input_partitions == 0 would lead to a panic below (division by 0). It would be good to prevent this or at least document it as a non-supported value.
…ructors (apache#19668) ### Which issue does this PR close? Closes apache#19664 --- ### Rationale for this change After apache#18880, `BatchPartitioner::try_new` gained additional parameters that are only relevant for round-robin repartitioning. This made the constructor API confusing, as hash repartitioning received parameters it does not use. Splitting the constructor improves clarity and avoids passing round-robin–specific parameters to hash partitioning. --- ### What changes are included in this PR? - Introduce `BatchPartitioner::try_new_hash` - Introduce `BatchPartitioner::try_new_round_robin` - Refactor callers to use the specialized constructors - Retain `BatchPartitioner::try_new` as a delegator for backward compatibility This is a pure refactor; behavior is unchanged. --- ### Are these changes tested? Yes. Existing tests cover this code path. All builds pass locally. --- ### Are there any user-facing changes? No. This change is internal only and does not affect user-facing behavior or APIs. --------- Co-authored-by: Your Name <[email protected]>
|
Thanks @milenkovicm! I’ve opened the backport PR to branch-52 as discussed. |
… constructors (#19681) Backport of #19668 to branch-52. This PR cherry-picks commit 680ddcc from main. Includes: - Split of BatchPartitioner::try_new into hash and round-robin constructors - Documentation improvements - No behavior changes part of #18566 Co-authored-by: Your Name <[email protected]>
4 participants
Which issue does this PR close?
Closes #19664
Rationale for this change
After #18880,
BatchPartitioner::try_newgained additional parameters that areonly relevant for round-robin repartitioning. This made the constructor API
confusing, as hash repartitioning received parameters it does not use.
Splitting the constructor improves clarity and avoids passing
round-robin–specific parameters to hash partitioning.
What changes are included in this PR?
BatchPartitioner::try_new_hashBatchPartitioner::try_new_round_robinBatchPartitioner::try_newas a delegator for backward compatibilityThis is a pure refactor; behavior is unchanged.
Are these changes tested?
Yes. Existing tests cover this code path.
All builds pass locally.
Are there any user-facing changes?
No. This change is internal only and does not affect user-facing behavior or APIs.