phillipleblanc commented on code in PR #2337:
URL:
https://github.com/apache/datafusion-ballista/pull/2337#discussion_r3810613452
##########
ballista/core/src/sort_key.rs:
##########
@@ -672,48 +718,321 @@ impl SortKeySketch {
}
}
- /// The `partitions - 1` boundaries that split everything observed into
- /// `partitions` equally-sized runs, in sort order.
+ /// The `partitions - 1` boundaries splitting everything observed into
+ /// `partitions` runs of equal size, in sort order. If a cut _would be_
NULL, it is adjusted to
+ /// include the nearest value, so this function never returns a NULL cut.
+ ///
+ /// The run is indivisible, so it takes the partition at its end whole and
+ /// only the values beside it balance.
///
- /// Empty when `partitions < 2` or nothing was observed, and otherwise
- /// exactly `partitions - 1` long so a caller can index it by output
- /// partition. Entries may repeat where one value dominates, and may be
- /// typed NULLs where the NULL run spans a boundary; both are faithful
- /// answers about a skewed distribution rather than errors.
- pub fn cuts(&self, partitions: usize) -> Result<Vec<ScalarValue>> {
- if partitions < 2 || self.count() == 0 {
+ /// ```text
+ /// nulls_first nulls_last
+ /// ┌──────┬─────────────────┐ ┌─────────────────┬──────┐
+ /// │NULLs │ values │ │ values │NULLs │
+ /// └──────┴─────────────────┘ └─────────────────┴──────┘
+ /// 0 n N 0 v N
+ ///
+ /// rank = max(pop − n, ...) rank = min(pop, ...)
+ /// pulls UP from min pulls DOWN from max
+ /// ```
+ ///
+ /// | nulls_first | NULLs | values | K | cuts | partition sizes
|
+ ///
|-------------|------:|-------:|--:|----------------|-------------------|
+ /// | true | 10 | 1..=90 | 4 | `[15, 40, 65]` | 24 / 25 / 25 /
26 |
+ /// | true | 60 | 1..=40 | 4 | `[1, 14, 27]` | 60 / 13 / 13 /
14 |
+ /// | false | 60 | 1..=40 | 4 | `[14, 27, 40]` | 13 / 13 / 13 /
61 |
+ /// | false | 90 | 1..=10 | 4 | `[4, 7, 10]` | 3 / 3 / 3 / 91
|
+ ///
+ /// Empty when `partitions < 2` or no value was observed.
+ ///
+ /// Should only error if:
+ /// 1. invalid sketch: min/max is empty but levels are not - guarded
against in proto decode
+ /// 2. codec.decode() failure - guarded against in try_new
+ pub fn cuts(&self, partition_cnt: usize) -> Result<Vec<ScalarValue>> {
Review Comment:
I believe this will conflate "no rows were observed" with "rows were
observed, but every key was NULL." For an all-NULL key, the report has a valid
sketch and nonzero row count, but this method would return empty, leading to
the query failing.
--
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]
