gabotechs commented on code in PR #26593:
URL: https://github.com/apache/datafusion/pull/26593#discussion_r4484194609
##########
datafusion/physical-expr/src/partitioning.rs:
##########
@@ -217,6 +237,10 @@ pub struct RangePartitioning {
samples: Arc<[SplitPoint]>,
/// Effective boundaries for the current partition count.
split_points: Arc<[SplitPoint]>,
+ /// Inclusive start of the key space, in `ordering` order.
+ start_bound: Option<SplitPoint>,
+ /// Exclusive end of the key space, in `ordering` order.
+ end_bound: Option<SplitPoint>,
Review Comment:
Not sure if this has been considered yet, maybe I'm late to the party, but
has anyone thought about just making `split_points` require 2 more positions,
the start and end bound?
Today we have something like this:
```rust
Range([10, 20, 30]) -> p0: [-inf, 10) p1: [10, 20) p2: [20, 30) p3: [30,
+inf)
partition_count == split_points.len() + 1
```
And this PR proposes adding optional start and end bounds, so that we have
something like this:
```rust
Range([10, 20, 30], start=Some(5), end=Some(35)) -> p0: [5, 10) p1: [10, 20)
p2: [20, 30) p3: [30, 35)
Range([10, 20, 30], start=None, end=Some(35)) -> p0: [-inf, 10) p1: [10, 20)
p2: [20, 30) p3: [30, 35)
Range([10, 20, 30], start=Some(5), end=None) -> p0: [5, 10) p1: [10, 20) p2:
[20, 30) p3: [30, +inf)
partition_count == split_points.len() + 1
```
Is there any reason not to make the bounds required? Range partitioning will
always be originated by leaf nodes, like Parquet scans, and isn't there safe to
assume that the ranges are always going to be bounded? If that's the case, we
could even go as far as just baking the start and end bounds in the split
points themselves:
```rust
Range([5, 10, 20, 30, 35]) -> p0: [5, 10) p1: [10, 20) p2: [20, 30) p3: [30,
35)
Range([-inf, 10, 20, 30, 35]) -> p0: [-inf, 10) p1: [10, 20) p2: [20, 30)
p3: [30, 35)
Range([5, 10, 20, 30, +inf]) -> p0: [5, 10) p1: [10, 20) p2: [20, 30) p3:
[30, +inf)
partition_count == split_points.len() - 1 // notice how this changed
```
+-inf can even be something that could be explicitly stated in the first
and/or last split point.
I feel like this should probably have been discussed before, but I don't see
any reference in any of the PRs mentioned linked in this one.
To @jayshrivastava's point in the FAQ:
> Some types don't have a max or a reasonable value ex. what's the max of a
string? "zzzzzz"? For variable length types too
Then, strings split points will just never have a +-inf bound, there will
always be a value required, which at first sight sounds like a fair assumption.
Range will always be originated from leaf nodes, and I'd expect the leaf nodes
to always be able to provide an appropriate value for all split points in the
range.
--
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]
