adriangb commented on issue #19664:
URL: https://github.com/apache/datafusion/issues/19664#issuecomment-3716432903
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.
--
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]
