mbutrovich commented on code in PR #26593:
URL: https://github.com/apache/datafusion/pull/26593#discussion_r4468144886
##########
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:
> In distributed-datafusion, this feature ensure all tasks (horizontally
distributed parts of the plan) have the same number of partitions.
> We could move away from this if anyone feels strongly, but I figured it's
a pretty inoffensive change to make in this repo.
I'd rather DataFusion not take on that invariant without more discussion. As
far as I can find, it doesn't have one today. 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 #25483). Padding with equal split
points pushes the other way: it raises partition counts to match instead of
letting them differ.
Bounds already let a task have fewer partitions without padding. In Example
4 of datafusion-contrib/datafusion-distributed#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. Could we keep `validate_range_split_points`
strict, require each bound to lie strictly outside the retained samples, and
leave padding to DFD? That also removes the empty range where `start_bound ==
end_bound`. Once DataFusion accepts equal split points, tightening the check
later would reject layouts that downstream code already builds. If the check
stays strict, the upgrade-guide entry I asked for on line 69 only needs to
cover the bounds fields.
--
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]
