gene-bordegaray commented on code in PR #24766:
URL: https://github.com/apache/datafusion/pull/24766#discussion_r4084094720
##########
datafusion/physical-expr/src/partitioning.rs:
##########
@@ -198,48 +206,97 @@ impl Display for Partitioning {
/// partition 2: keys at/after (2023, Allston)
/// ```
///
-/// NOTE: Optimizer and execution behavior for this partitioning is
intentionally
-/// not implemented and will be introduced incrementally. See
-/// <https://github.com/apache/datafusion/issues/22395>.
+/// Equality includes retained samples, since they determine which future
scales
+/// are possible. Use [`Self::has_same_layout`] to compare only the current
layout.
#[derive(Debug, Clone, PartialEq)]
pub struct RangePartitioning {
/// Ordered partitioning key.
ordering: LexOrdering,
- /// Boundaries between adjacent partitions.
- split_points: Vec<SplitPoint>,
+ /// Caller-supplied maximum-resolution split points used to derive the
+ /// effective boundaries.
+ samples: Arc<[SplitPoint]>,
+ /// Effective boundaries for the current partition count.
+ split_points: Arc<[SplitPoint]>,
}
impl RangePartitioning {
/// Creates range partitioning metadata without validating split points.
///
- /// Use [`Self::try_new`] to validate the contract documented on
- /// [`RangePartitioning`].
+ /// Prefer [`Self::try_new_with_samples`] to validate the boundaries and
retain
+ /// additional samples for scaling up. [`Self::try_new`] remains available
for
+ /// validated exact boundaries.
+ #[deprecated(
+ since = "56.0.0",
+ note = "Use RangePartitioning::try_new_with_samples instead"
+ )]
pub fn new(ordering: LexOrdering, split_points: Vec<SplitPoint>) -> Self {
+ let split_points: Arc<[SplitPoint]> = Arc::from(split_points);
Self {
ordering,
+ samples: Arc::clone(&split_points),
split_points,
}
}
/// Creates range partitioning metadata and validates split point shape and
/// ordering.
+ ///
+ /// The exact boundaries are also the retained samples. This allows scaling
+ /// down and back up to the original count, but not beyond it. Prefer
+ /// [`Self::try_new_with_samples`] when additional sample points are
available.
pub fn try_new(ordering: LexOrdering, split_points: Vec<SplitPoint>) ->
Result<Self> {
+ let partition_count = split_points.len() + 1;
+ Self::try_new_with_samples(ordering, split_points, partition_count)
+ }
+
+ /// Creates sample-backed range partitioning and validates the sample
shape,
Review Comment:
can we point to try_new when appropriate in docs
##########
datafusion/physical-expr/src/partitioning.rs:
##########
@@ -198,48 +206,97 @@ impl Display for Partitioning {
/// partition 2: keys at/after (2023, Allston)
/// ```
///
-/// NOTE: Optimizer and execution behavior for this partitioning is
intentionally
-/// not implemented and will be introduced incrementally. See
-/// <https://github.com/apache/datafusion/issues/22395>.
+/// Equality includes retained samples, since they determine which future
scales
+/// are possible. Use [`Self::has_same_layout`] to compare only the current
layout.
#[derive(Debug, Clone, PartialEq)]
pub struct RangePartitioning {
/// Ordered partitioning key.
ordering: LexOrdering,
- /// Boundaries between adjacent partitions.
- split_points: Vec<SplitPoint>,
+ /// Caller-supplied maximum-resolution split points used to derive the
+ /// effective boundaries.
+ samples: Arc<[SplitPoint]>,
+ /// Effective boundaries for the current partition count.
+ split_points: Arc<[SplitPoint]>,
}
impl RangePartitioning {
/// Creates range partitioning metadata without validating split points.
///
- /// Use [`Self::try_new`] to validate the contract documented on
- /// [`RangePartitioning`].
+ /// Prefer [`Self::try_new_with_samples`] to validate the boundaries and
retain
+ /// additional samples for scaling up. [`Self::try_new`] remains available
for
+ /// validated exact boundaries.
+ #[deprecated(
+ since = "56.0.0",
+ note = "Use RangePartitioning::try_new_with_samples instead"
+ )]
pub fn new(ordering: LexOrdering, split_points: Vec<SplitPoint>) -> Self {
+ let split_points: Arc<[SplitPoint]> = Arc::from(split_points);
Self {
ordering,
+ samples: Arc::clone(&split_points),
split_points,
}
}
/// Creates range partitioning metadata and validates split point shape and
/// ordering.
+ ///
+ /// The exact boundaries are also the retained samples. This allows scaling
+ /// down and back up to the original count, but not beyond it. Prefer
+ /// [`Self::try_new_with_samples`] when additional sample points are
available.
pub fn try_new(ordering: LexOrdering, split_points: Vec<SplitPoint>) ->
Result<Self> {
+ let partition_count = split_points.len() + 1;
+ Self::try_new_with_samples(ordering, split_points, partition_count)
+ }
+
+ /// Creates sample-backed range partitioning and validates the sample
shape,
+ /// ordering, and target partition count.
+ ///
+ /// `partition_count` must be at least one and no larger than
+ /// `samples.len() + 1`. When it is smaller than that maximum, the samples
+ /// are evenly down-sampled to derive the effective split points.
+ ///
+ /// Retain at least `maximum_expected_partitions - 1` samples to support
that
Review Comment:
this is very wordy I was thinking something more like the range partitionig
main docs. Here is the example there as reference:
```text
/// For a single range key:
///
/// ```text
/// ordering = [date ASC NULLS LAST]
/// split_points = [
/// (2022-01-01),
/// (2023-01-01),
/// ]
///
/// partition 0: date before 2022-01-01
/// partition 1: date between 2022-01-01 (inclusive) and 2023-01-01
(exclusive)
/// partition 2: date at/after 2023-01-01
/// ```
```
--
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]
