Split `BatchPartitioner::try_new(..)` into `BatchPartitioner::new_hash` and `BatchPartitioner::new_round_robin`

[milenkovicm]

as part of #18880 BatchPartitioner try_new method got two additional parameters, which are only relevant for round_robin partitioning

-       let mut partitioner =
-            BatchPartitioner::try_new(partitioning, metrics.repartition_time.clone())?;
+        let mut partitioner = BatchPartitioner::try_new(
+           partitioning,
+           metrics.repartition_time.clone(),
+           input_partition,
+            num_input_partitions,
+        )?;

As those two new parameters are only relevant for round robin, it would make sense splitting the method.

Sorry I'm a bit late with my comment. I've stumbled upon this change updating ballista. Its a bit confusing change from usage perspective, there are two parameters which are used only in round robin case, would it make sense splitting try_new method in try_new_hash and try_new_round_robin?

Originally posted by @milenkovicm in #18880 (comment)

cc: @alamb, @Dandandan

[mohit7705]

Hi @milenkovicm 👋
I’d like to work on this issue and submit a PR if that’s okay.
I’ll start by splitting BatchPartitioner::try_new into hash and round-robin variants as suggested.
Please let me know if there’s anything specific you’d like me to watch out for.

[mohit7705]

Hi @milenkovicm 👋
I’ve opened a PR addressing this by splitting
BatchPartitioner::try_new into hash and round-robin constructors.

Thanks!

[mohit7705]

Hi @milenkovicm 👋
I’ve addressed the rustfmt failure and pushed an update.

[adriangb]

I'll say I've seen the pattern of several places in DataFusion organically growing multiple constructors that then get called with things like ..., 0, 0) because like in this case they're only sometimes needed. I think that is especially important in cases like this where multiple parameters are of the same type, which makes it easy to get them mixed up and hard to tell from call sites what is going on without inspecting the method. The same applies to return types (i.e. func() -> (usize, usize) which I've seen a couple of).

Using a builder has the added benefit of having a better backwards compatibility story next time a parameter is added.

In #19619 for example we went down the road of paying the upfront effort to create a builder and it seems to have come out nicely.

In the case of BatchPartitioner maybe even multiple builders are needed to maintain type safety (not being able to pass in a parameter that is irrelevant, e.g. RoundRobinBatchPartitionerBuilder and HashBatchPartitionerBuilder), or we could make the setters fallible and return an error (fn with_partition(self, partition: usize) -> Result<Self> { assert!(self.num_partitions.is_some()); assert!(matches!(self.state, BatchPartitionerState::RoundRobin { .. })); }).

Take this with a grain of salt, I just wanted to remind of the option of using a building instead of constructors with multiple parameters.

[milenkovicm]

Thanks @adriangb,
I do not see problem with constructors with multiple "required" parameters, they force users to provide all required input.
In this specific case, two parameters were required always and two just in case of round-robin, so user has to read the docs or source code to understand when/what to provide (which proposal with two constructors make obvious). In current proposal once user select which partitioner is needed constructor will force input of all required parameters. We could go with the builders in this case, and builders would "start" with same methods as constructors (to enforce required parameters), and there would be no with_ methods at the moment. I would argue that builders are overkill at the moment for few reasons, its unlikely that this signatures will ever change, with current proposal we saved one .build() call, and we did not introduce two builder structs ...

Personally I'm not fun of last part of your proposal, you have traded type safety (compiler) check for runtime check, assert in your example would need to be documented. I personally believe apis should enforce constraints on type(s) rather than documentation if/when possible .

I do agree that builders are way to go with "optional" parameters, and use of builder in #19619 does make sense (to me). FilterExecBuilder::new( which forces you to provide required inputs and with_ methods which are providing optionals. build method does the input validation, as type enforcing check can't be done in that case, so good example of builder.