You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "mohit7705 (via GitHub)" <gi...@apache.org> on 2026/01/06 15:31:24 UTC
[PR] refactor(repartition): split BatchPartitioner::try_new into hash and round-robin constructors [datafusion]
mohit7705 opened a new pull request, #19668:
URL: https://github.com/apache/datafusion/pull/19668
### Which issue does this PR close?
Closes #19664
---
### Rationale for this change
After #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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor(repartition): split BatchPartitioner::try_new into hash and round-robin constructors [datafusion]
Posted by "mohit7705 (via GitHub)" <gi...@apache.org>.
mohit7705 commented on code in PR #19668:
URL: https://github.com/apache/datafusion/pull/19668#discussion_r2665970890
##########
datafusion/physical-plan/src/repartition/mod.rs:
##########
@@ -437,30 +437,58 @@ impl BatchPartitioner {
/// Create a new [`BatchPartitioner`] with the provided [`Partitioning`]
///
/// The time spent repartitioning will be recorded to `timer`
+ /// Create a new [`BatchPartitioner`] for hash partitioning
+ pub fn try_new_hash(
Review Comment:
Thanks for the review! 🙏
I’ve addressed the comments by:
- Renaming the constructors to remove `try_` and `Result` where they cannot fail
- Updating all call sites accordingly
- Adding detailed documentation explaining each parameter and behavior
Please let me know if anything else needs adjustment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: split BatchPartitioner::try_new into hash and round-robin constructors [datafusion]
Posted by "mohit7705 (via GitHub)" <gi...@apache.org>.
mohit7705 commented on PR #19668:
URL: https://github.com/apache/datafusion/pull/19668#issuecomment-3716227520
Done 👍
Added documentation for `try_new` explaining its purpose, parameters, and error conditions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor(repartition): split BatchPartitioner::try_new into hash and round-robin constructors [datafusion]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on code in PR #19668:
URL: https://github.com/apache/datafusion/pull/19668#discussion_r2665830760
##########
datafusion/physical-plan/src/repartition/mod.rs:
##########
@@ -437,30 +437,58 @@ impl BatchPartitioner {
/// Create a new [`BatchPartitioner`] with the provided [`Partitioning`]
///
/// The time spent repartitioning will be recorded to `timer`
+ /// Create a new [`BatchPartitioner`] for hash partitioning
+ pub fn try_new_hash(
+ exprs: Vec<Arc<dyn PhysicalExpr>>,
+ num_partitions: usize,
+ timer: metrics::Time,
+ ) -> Result<Self> {
+ Ok(Self {
+ state: BatchPartitionerState::Hash {
+ exprs,
+ num_partitions,
+ hash_buffer: vec![],
+ },
+ timer,
+ })
+ }
+
+ /// Create a new [`BatchPartitioner`] for round-robin partitioning
Review Comment:
documentation needs improvement, clarification needed what each parameter does
##########
datafusion/physical-plan/src/repartition/mod.rs:
##########
@@ -437,30 +437,58 @@ impl BatchPartitioner {
/// Create a new [`BatchPartitioner`] with the provided [`Partitioning`]
///
/// The time spent repartitioning will be recorded to `timer`
+ /// Create a new [`BatchPartitioner`] for hash partitioning
+ pub fn try_new_hash(
Review Comment:
method can't fail if im not mistaken, there is no need for `try` and `Result`
perhaps `new_hash_partitioner` as the name?
##########
datafusion/physical-plan/src/repartition/mod.rs:
##########
@@ -437,30 +437,58 @@ impl BatchPartitioner {
/// Create a new [`BatchPartitioner`] with the provided [`Partitioning`]
///
/// The time spent repartitioning will be recorded to `timer`
+ /// Create a new [`BatchPartitioner`] for hash partitioning
+ pub fn try_new_hash(
+ exprs: Vec<Arc<dyn PhysicalExpr>>,
+ num_partitions: usize,
+ timer: metrics::Time,
+ ) -> Result<Self> {
+ Ok(Self {
+ state: BatchPartitionerState::Hash {
+ exprs,
+ num_partitions,
+ hash_buffer: vec![],
+ },
+ timer,
+ })
+ }
+
+ /// Create a new [`BatchPartitioner`] for round-robin partitioning
Review Comment:
method can't fail if im not mistaken, there is no need for `try` and `Result`
perhaps `new_round_robing_partitioner` as the name?
##########
datafusion/physical-plan/src/repartition/mod.rs:
##########
@@ -437,30 +437,58 @@ impl BatchPartitioner {
/// Create a new [`BatchPartitioner`] with the provided [`Partitioning`]
///
/// The time spent repartitioning will be recorded to `timer`
+ /// Create a new [`BatchPartitioner`] for hash partitioning
+ pub fn try_new_hash(
+ exprs: Vec<Arc<dyn PhysicalExpr>>,
+ num_partitions: usize,
+ timer: metrics::Time,
+ ) -> Result<Self> {
+ Ok(Self {
+ state: BatchPartitionerState::Hash {
+ exprs,
+ num_partitions,
+ hash_buffer: vec![],
+ },
+ timer,
+ })
+ }
+
+ /// Create a new [`BatchPartitioner`] for round-robin partitioning
+ pub fn try_new_round_robin(
+ num_partitions: usize,
+ timer: metrics::Time,
+ input_partition: usize,
+ num_input_partitions: usize,
+ ) -> Result<Self> {
+ Ok(Self {
+ state: BatchPartitionerState::RoundRobin {
+ num_partitions,
+ next_idx: (input_partition * num_partitions) / num_input_partitions,
+ },
+ timer,
+ })
+ }
+
Review Comment:
documentation needs improvement, clarification needed what each parameter does
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: split BatchPartitioner::try_new into hash and round-robin constructors [datafusion]
Posted by "mohit7705 (via GitHub)" <gi...@apache.org>.
mohit7705 commented on PR #19668:
URL: https://github.com/apache/datafusion/pull/19668#issuecomment-3717530237
Thanks again for the reviews! 🙏
@milenkovicm
All checks are green now — feel free to merge whenever convenient.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: split BatchPartitioner::try_new into hash and round-robin constructors [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #19668:
URL: https://github.com/apache/datafusion/pull/19668#discussion_r2668107076
##########
datafusion/physical-plan/src/repartition/mod.rs:
##########
@@ -434,33 +434,90 @@ pub const REPARTITION_RANDOM_STATE: SeededRandomState =
SeededRandomState::with_seeds(0, 0, 0, 0);
impl BatchPartitioner {
- /// Create a new [`BatchPartitioner`] with the provided [`Partitioning`]
+ /// Create a new [`BatchPartitioner`] for hash-based repartitioning.
///
- /// The time spent repartitioning will be recorded to `timer`
+ /// # Parameters
+ /// - `exprs`: Expressions used to compute the hash for each input row.
+ /// - `num_partitions`: Total number of output partitions.
+ /// - `timer`: Metric used to record time spent during repartitioning.
+ ///
+ /// # Notes
+ /// This constructor cannot fail and performs no validation.
+ pub fn new_hash_partitioner(
+ exprs: Vec<Arc<dyn PhysicalExpr>>,
+ num_partitions: usize,
+ timer: metrics::Time,
+ ) -> Self {
+ Self {
+ state: BatchPartitionerState::Hash {
+ exprs,
+ num_partitions,
+ hash_buffer: vec![],
+ },
+ timer,
+ }
+ }
+
+ /// Create a new [`BatchPartitioner`] for round-robin repartitioning.
+ ///
+ /// # Parameters
+ /// - `num_partitions`: Total number of output partitions.
+ /// - `timer`: Metric used to record time spent during repartitioning.
+ /// - `input_partition`: Index of the current input partition.
+ /// - `num_input_partitions`: Total number of input partitions.
+ ///
+ /// # Notes
+ /// The starting output partition is derived from the input partition
+ /// to avoid skew when multiple input partitions are used.
+ pub fn new_round_robin_partitioner(
+ num_partitions: usize,
+ timer: metrics::Time,
+ input_partition: usize,
+ num_input_partitions: usize,
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: split BatchPartitioner::try_new into hash and round-robin constructors [datafusion]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #19668:
URL: https://github.com/apache/datafusion/pull/19668#issuecomment-3716203455
thanks @alamb & @mohit7705
i have just one small comment, and then we can merge this.
If @mohit7705 wants to create patch for 52 would be great otherwise i'll create it tomorrow morning
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: split BatchPartitioner::try_new into hash and round-robin constructors [datafusion]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm merged PR #19668:
URL: https://github.com/apache/datafusion/pull/19668
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: split BatchPartitioner::try_new into hash and round-robin constructors [datafusion]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on code in PR #19668:
URL: https://github.com/apache/datafusion/pull/19668#discussion_r2666191787
##########
datafusion/physical-plan/src/repartition/mod.rs:
##########
@@ -437,30 +437,58 @@ impl BatchPartitioner {
/// Create a new [`BatchPartitioner`] with the provided [`Partitioning`]
///
/// The time spent repartitioning will be recorded to `timer`
+ /// Create a new [`BatchPartitioner`] for hash partitioning
+ pub fn try_new_hash(
+ exprs: Vec<Arc<dyn PhysicalExpr>>,
+ num_partitions: usize,
+ timer: metrics::Time,
+ ) -> Result<Self> {
+ Ok(Self {
+ state: BatchPartitionerState::Hash {
+ exprs,
+ num_partitions,
+ hash_buffer: vec![],
+ },
+ timer,
+ })
+ }
+
+ /// Create a new [`BatchPartitioner`] for round-robin partitioning
+ pub fn try_new_round_robin(
+ num_partitions: usize,
+ timer: metrics::Time,
+ input_partition: usize,
+ num_input_partitions: usize,
+ ) -> Result<Self> {
+ Ok(Self {
+ state: BatchPartitionerState::RoundRobin {
+ num_partitions,
+ next_idx: (input_partition * num_partitions) / num_input_partitions,
+ },
+ timer,
+ })
+ }
+
Review Comment:
can we please add documentation for try new as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: split BatchPartitioner::try_new into hash and round-robin constructors [datafusion]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #19668:
URL: https://github.com/apache/datafusion/pull/19668#issuecomment-3718056659
thanks @mohit7705
It would make sense to have this released with datafusion 52 (#18566). would you be able to provide patch to branch 52 or you want us to do it ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: split BatchPartitioner::try_new into hash and round-robin constructors [datafusion]
Posted by "mohit7705 (via GitHub)" <gi...@apache.org>.
mohit7705 commented on PR #19668:
URL: https://github.com/apache/datafusion/pull/19668#issuecomment-3718724097
Thanks @milenkovicm!
I’ve opened the backport PR to branch-52 as discussed.
Please have a look when you get a chance, and feel free to merge if everything looks good
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org