andygrove opened a new pull request, #7402: URL: https://github.com/apache/datafusion-comet/pull/7402
## Which issue does this PR close? Closes #7395. ## Rationale for this change `datasketches` 0.3.0 had two HLL decode defects, and `hll_sketch.rs` carried a workaround for each: - Compact HLL array sketches lost their register block, so a decoded sketch kept its own estimate but every union built from it was wrong. Comet cleared the COMPACT flag before decoding. - An updatable HLL_4 sketch with auxiliary-map entries was read with the compact layout, dropping the exceptions. Comet rejected that shape with an error, so a sketch column written by DataSketches-Java with its default target type (HLL_4) failed in `hll_sketch_estimate`, `hll_union` and `hll_union_agg` once it held enough distinct values. The crate's changelog lists fixes for both. I checked rather than trusted it, see below. 0.6.0 was released on 2026-10-29. ## What changes are included in this PR? - `datasketches` `=0.3.0` to `=0.6.0`. `Cargo.lock` changes only that crate and has no new dependencies. - Two API changes in `hll_sketch.rs`: `hash_value` is now `hash::value`, and `HllSketch::new` / `HllUnion::new` return `Result`. `SparkHllSketch::new` and `SparkHllUnion::new` stay infallible with an `expect`, as the old constructors panicked on the same out-of-range `lgConfigK`, which the serdes already validate. - Removed `normalize_compact_hll_array`, `reject_undecodable_hll4` and the preamble constants only they used. - Removed the HLL_4 auxiliary-map entry from `getIncompatibleReasons` in `CometHllSketchEstimate`, `CometHllUnion` and `CometHllUnionAgg`, since that input is now read correctly. - Replaced the two tests that pinned the rejection with tests against three sketches serialized by DataSketches-Java 6.2.0 (the version Spark 4.1 and 4.2 ship) at lgConfigK 12: HLL_4 updatable with one aux entry, HLL_4 compact with two, and HLL_6. Each test checks the decoded estimate, and the estimate of a union with a second sketch, against Java's value to within 1e-6. - The module pin comment in `native/Cargo.toml` is rewritten for the new reason to pin exactly. Left alone: the `layout` module that sizes accumulators. `estimated_size()` now exists, but the changelog calls it implementation-dependent, so that is part of the HLL audit in #7396. ## How are these changes tested? - Native: `cargo test -p datafusion-comet-spark-expr --lib hll` (38 pass), and `cargo clippy --workspace --all-targets -- -D warnings` is clean. - The new fixture tests do catch the old behavior. I ran the same three fixtures through the raw 0.3.0 crate in a scratch project: both HLL_4 fixtures decode to the right estimate and a wrong union (the compact one gives 1,991,062 where Java gives 4,033,480). The HLL_6 fixture matches on both versions and is a regression guard only. - Before deleting the workarounds I also compared all 18 combinations of HLL_4/6/8, compact/updatable, and 1,000 / 100,000 / 2,000,000 values with Java, and sketches built in Rust and unioned after a byte round trip. All matched to within 1e-6. Those scratch tests are not committed; the three fixtures above are the ones that distinguish old from new. - JVM: `CometExpressionSuite` HLL tests (5) on the default Spark 4.1 profile, with a `libcomet` built from this tree. I did not run the other Spark profiles locally; `run-all-spark-profiles` covers 4.0 and 4.2. Not covered: Spark 4.0 ships DataSketches-Java 6.1.1, and I only generated fixtures with 6.2.0. #7396 includes doing both. -- 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]
