Copilot commented on code in PR #21900:
URL: https://github.com/apache/datafusion/pull/21900#discussion_r3161606233
##########
datafusion/physical-plan/src/repartition/mod.rs:
##########
@@ -446,22 +511,27 @@ impl BatchPartitioner {
/// - `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.
+ /// # Errors
+ /// Returns an error if `num_partitions` is zero.
pub fn new_hash_partitioner(
exprs: Vec<Arc<dyn PhysicalExpr>>,
num_partitions: usize,
timer: metrics::Time,
- ) -> Self {
- Self {
+ ) -> Result<Self> {
+ if num_partitions == 0 {
+ return internal_err!("Hash repartition requires at least one
partition");
+ }
+
+ Ok(Self {
state: BatchPartitionerState::Hash {
exprs,
num_partitions,
+ partition_reducer: StrengthReducedU64::new(num_partitions as
u64),
hash_buffer: vec![],
indices: vec![vec![]; num_partitions],
},
timer,
- }
+ })
}
Review Comment:
`new_hash_partitioner` is `pub` in the `datafusion-physical-plan` crate, but
this change makes it fallible (`-> Result<Self>`) and introduces a new runtime
error for `num_partitions == 0`. That is a breaking public API change for
downstream crates. Consider preserving the existing signature (e.g., keep
`new_hash_partitioner` infallible and add a new `try_new_hash_partitioner` /
`new_hash_partitioner_checked`), or reduce visibility if this is intended to be
internal-only.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
