jayshrivastava commented on code in PR #26593:
URL: https://github.com/apache/datafusion/pull/26593#discussion_r4471377710
##########
datafusion/common/src/partitioning.rs:
##########
@@ -89,10 +86,10 @@ pub fn validate_range_split_points(
split_points[0].values(),
split_points[1].values(),
sort_options,
- )? != Ordering::Less
+ )? == Ordering::Greater
Review Comment:
> Partition counts already differ from operator to operator, and the only
places that need equal counts are children that must be co-partitioned
([compatible_co_partitioning_layout](https://github.com/apache/datafusion/blob/c120619dec70f723e9e9f9174cb8194c532a5a75/datafusion/physical-plan/src/distribution_requirements.rs#L353-L365))
and interleaved inputs
([can_interleave](https://github.com/apache/datafusion/blob/c120619dec70f723e9e9f9174cb8194c532a5a75/datafusion/physical-plan/src/union.rs#L957-L969)).
I'd like to keep DataFusion open to running operators with different partition
counts, for example an operator that doesn't need as many partitions coalescing
adjacent ranges (the SourceFiner case in
https://github.com/apache/datafusion/issues/25483). Padding with equal split
points pushes the other way: it raises partition counts to match instead of
letting them differ.
I would expect `can_interleave` and `compatible_co_partitioning_layout` to
be improved so that we are able to interleave arbitrary partitioned data. Ex.
`Range(3, [60, 80], start=50)` should be interleaved with `Range(4, [60, 80])`.
You mentioned this below as well. I'll file an issue.
Also, my experience has been that, if the user wants to avoid a
`RepartitionExec` or wants an `InterleaveExec`, they have to make sure the
correct partitioning properties are declared on their data source. I've even
gone as far to declare data sources as `Hash()` partitioned when they are not.
This is to say that I think a user would probably go out of their way to
declare interleavable and/or compatible, not necessarily relying on datafusion.
> Padding with equal split points pushes the other way: it raises partition
counts to match instead of letting them differ
I'd say the uses for padding are user-defined, like our invariant in DFD and
the KLL example Brent mentioned above.
Padding would not really be used to achieve co-partitioning. For example, I
don't see a way to pad `Range(2, [50])` to make it compatible with `Range(3,
[50, 60])`. Padding with `Range(3, [50, 50])` doesn't work. Converting to
`Range(3, [50, 60])` requires a repartition, splitting up the last partition.
> In Example 4 of
https://github.com/datafusion-contrib/datafusion-distributed/pull/782, the last
task could be Range(2, [60], start=50) instead of split points [50, 60] with
start=50 and an empty first partition.
The main distinction is that the former has 2 partitions and latter has 3.
You force the user to have an additional tasks spawned if they want to add
bounds to `Range` partitioning.
Padding empty partitions also introduces additional tasks, but the user can
opt out of this by simply not padding.
--
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]
