andygrove commented on PR #2294:
URL:
https://github.com/apache/datafusion-ballista/pull/2294#issuecomment-5295563242
> **Disclaimer:** This review was produced by an LLM (Claude Code) at my
request. I've skimmed it, but treat the reasoning as unverified assistance
rather than a considered human review — check the claims before acting on them.
The test/clippy/fmt results quoted below were actually run against the PR head
(`52b5fac`) locally.
## Overview
Purely additive, and the claim holds up — nothing in the tree constructs
`SortKeyCodec` or `SortKeySketch`, and `RuntimeStatsExec` and T-Digest are
untouched. Three parts:
- **`ballista/core/src/sort_key.rs`** (new): `SortKeyCodec` maps one
fixed-width `ORDER BY` key to an order-preserving `u64` and back (24 arrow
types: ints, `Float32/64`, and the `i32`/`i64`-backed temporals), folding
`descending` into the key as a bitwise NOT. `SortKeySketch` pairs it with
`KllSketch<u64>` plus an out-of-band NULL count.
- **`kll.rs`**: adds `min()`, `max()`, `count()`, `at_rank()`, hand-written
`Clone` and `Debug`. `quantile` is refactored onto `at_rank`.
- **`quantile_sketch.rs`**: new arms measuring the shipped path plus the
priced-and-dropped alternatives.
## Verification
| Check | Result |
|---|---|
| `cargo test -p ballista-core --lib sort_key` | 28 passed |
| `cargo test -p ballista-core --lib` | 303 passed, 2 failed |
| `cargo clippy -p ballista-core --all-targets` | clean |
| `cargo fmt --all --check` | clean |
| `cargo check --benches` | clean |
The two failures are
`utils::tests::test_create_grpc_server_incoming_binds_eagerly` and
`..._port_in_use` — socket-binding tests failing under a sandbox, in a module
this PR doesn't touch. Not attributable to this change.
The NULL rank remap was checked independently against nulls-in-key semantics
for both placements and all four boundary cases (`rank == 0`, inside the run,
exactly on the run's end, `rank >= total`); it agrees. The
`at_rank`-in-integers argument is correct, and the differential oracle is the
right way to hold it. f32 encoding traced by hand in both directions — correct.
## Findings
**1. All-NULL sketch reports `min()`/`max()` as `None` (`sort_key.rs:588`)**
`extreme` only produces a typed NULL for the end the NULL run occupies. For
an all-NULL column the *other* end falls through to the empty value sketch and
returns `None` — which both methods document as "nothing was observed".
Measured on 8 NULL `Int64` rows with `nulls_first: true`:
```
count()==8 min()==Some(Int64(NULL)) max()==None
```
and it inverts under `nulls_first: false`. `quantile()` gets this input
right via its `values == 0` branch
(`an_all_null_column_answers_null_everywhere` covers it), so the two APIs
disagree. Since the module docs note that `cut_partitions` routes shuffle files
on `[sketch.min(), sketch.max()]`, the announced follow-up consumer would see a
half-absent range for an all-NULL partition.
Suggested fix in `extreme`: when `value_extreme` is `None` and `null_count >
0`, return the typed NULL regardless of which end was asked for.
**2. 20 of 24 dispatched types have no round-trip test (`sort_key.rs:293`)**
`dispatch_sortable!` admits 24 types; the tests exercise `Int64`, `UInt64`,
`Float64` and `TimestampNanosecond`. That leaves the entire
`impl_sortable_float!(f32, u32, 32)` instantiation unpinned — a distinct path
from the f64 one, since `to_key` zero-extends a 32-bit key and `from_key`
truncates, while under DESC the `orient` NOT applies over all 64 bits. Same for
the narrow ints and every temporal but one. A loop asserting
encode-monotonicity plus decode round-trip over one representative array per
dispatched type would also make `try_new`'s allowlist and the decode arms
self-checking.
## Smaller notes
- **`cuts()` (`sort_key.rs:673`) uses `.flatten()`**, silently dropping
`None` quantiles and returning fewer than `partitions - 1` entries. Safe today
(`quantile` returns `None` only when `total == 0`, all-or-nothing), but a
consumer indexing cuts by partition has no signal if that invariant ever
changes. `collect::<Option<Vec<_>>>()` or an explicit early return would make
it structural.
- **`KLL_K = 800` is duplicated** — private in `sort_key.rs`, redeclared in
the bench. The bench's claim that `kll_norm_u64` and `sort_key_sketch_f64` are
comparable depends on them staying equal, with nothing enforcing it.
- **`encode` requires exact `DataType` equality**, so a codec built for
`Timestamp(ns, Some("UTC"))` errors on an array tagged `Some("+00:00")` even
though the timezone is irrelevant to the key. Strictness is defensible; worth
knowing when the consumer lands.
- **`Clone` for `KllSketch` reseeds from `rand::random()`**, so cloning is
non-deterministic. The decorrelation argument is sound and `new()` already
seeds from OS entropy, so no regression — just a caveat for any future test
that clones a sketch expecting reproducibility.
- **`quantile` computes `count()` and then `at_rank` recomputes it**, and
`at_rank` allocates and sorts the ~3k retained pairs per call, so `cuts(P)` is
O(P · m log m). Negligible against ingest, and pre-existing in shape.
## Quality
Encoding, direction handling, and NULL placement are each isolated where
they belong, and the arrow-row agreement test is the right invariant to assert
rather than argue. The float bit-table pinned by
`float_key_table_in_docs_is_accurate` is a nice touch — that comment can't rot.
Comment density is high relative to the rest of `ballista/core`, but it's
load-bearing (the rank-remap off-by-one, the NaN/`total_cmp` reasoning, the
priced-and-dropped alternatives) rather than restating code.
Both findings are contained to the two spots above; neither undermines the
additive-only claim.
--
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]
