viirya commented on code in PR #7402:
URL: https://github.com/apache/datafusion-comet/pull/7402#discussion_r4447008766
##########
native/spark-expr/src/agg_funcs/hll_sketch.rs:
##########
@@ -412,63 +315,81 @@ mod tests {
);
}
- /// An HLL_4 sketch with auxiliary-map exceptions must be refused rather
than silently
- /// mis-decoded. At lgK=12 the exceptions appear somewhere above ~100k
distinct values.
- #[test]
- fn updatable_hll4_with_aux_entries_is_rejected() {
- use datasketches::hll::{HllSketch, HllType};
- let mut sketch = HllSketch::new(12, HllType::Hll4);
- for i in 0..100_000i64 {
- sketch.update(i);
+ /// Sketches serialized by DataSketches-Java 6.2.0 (the version Spark 4.1
ships) with
+ /// `HllSketch.update(long)` over `0..n` at lgConfigK 12, covering the
layouts a third party
+ /// can hand to `hll_sketch_estimate` / `hll_union` / `hll_union_agg`:
Review Comment:
All three fixtures (and `hll_sketch_spark_lgk12.bin`) are in HLL array mode,
so no LIST/SET-mode Java input is covered. That is where 0.6.0 added new header
checks (`lg_arr == 3` for LIST, `[5, lgK-3]` for SET) that 0.3.0 didn't have. A
Spark-written low-cardinality sketch reaches this path when `hll_sketch_agg`
falls back and `hll_union_agg` runs natively. I checked Java 6.2.0 LIST/SET
sketches (compact and updatable, lgK 4–21) and they all decode and union
correctly, so this is coverage rather than a bug. Could we add one
small-cardinality Java fixture, or narrow the "covering the layouts" wording?
##########
native/spark-expr/src/agg_funcs/hll_sketch.rs:
##########
@@ -412,63 +315,81 @@ mod tests {
);
}
- /// An HLL_4 sketch with auxiliary-map exceptions must be refused rather
than silently
- /// mis-decoded. At lgK=12 the exceptions appear somewhere above ~100k
distinct values.
- #[test]
- fn updatable_hll4_with_aux_entries_is_rejected() {
- use datasketches::hll::{HllSketch, HllType};
- let mut sketch = HllSketch::new(12, HllType::Hll4);
- for i in 0..100_000i64 {
- sketch.update(i);
+ /// Sketches serialized by DataSketches-Java 6.2.0 (the version Spark 4.1
ships) with
+ /// `HllSketch.update(long)` over `0..n` at lgConfigK 12, covering the
layouts a third party
+ /// can hand to `hll_sketch_estimate` / `hll_union` / `hll_union_agg`:
+ ///
+ /// - `hll4_updatable_aux_lgk12_100000.bin`: HLL_4,
`toUpdatableByteArray()`, one auxiliary-map
+ /// exception. The updatable form writes the whole aux table, empty
slots included.
+ /// - `hll4_compact_aux_lgk12_2000000.bin`: HLL_4, `toCompactByteArray()`,
two exceptions.
+ /// - `hll6_lgk12_100000.bin`: HLL_6, which has no compact form distinct
from the updatable one.
+ ///
+ /// `java_union` is the estimate of `Union(12)` over the sketch and an
HLL_8 sketch of `n..2n`,
+ /// also from Java. Registers have to decode exactly for a union to match
it, which is what a
+ /// decoder that drops or misreads them (as `datasketches` 0.3.0 did)
fails.
+ fn assert_matches_java(bytes: &[u8], n: i64, java_estimate: f64,
java_union: f64) {
+ let sketch = SparkHllSketch::from_bytes(bytes).unwrap();
+ assert!(
+ (sketch.estimate() - java_estimate).abs() < 1e-6,
+ "estimate {} != Java {java_estimate}",
+ sketch.estimate()
+ );
+ let mut other = SparkHllSketch::new(12);
+ for i in n..2 * n {
+ other.update_i64(i);
}
- let bytes = sketch.serialize();
- // Confirm the premise rather than assuming it: HLL array mode, HLL_4,
not compact, and
- // carrying at least one exception.
- assert_eq!(
- bytes[preamble::MODE] & preamble::CUR_MODE_MASK,
- preamble::CUR_MODE_HLL
+ let mut union = SparkHllUnion::new(12);
+ union.merge(&sketch);
+ union.merge(&other);
+ let result =
SparkHllSketch::from_bytes(&union.to_sketch_bytes()).unwrap();
+ assert!(
+ (result.estimate() - java_union).abs() < 1e-6,
+ "union estimate {} != Java {java_union}",
+ result.estimate()
);
- assert_eq!(
- preamble::tgt_type(bytes[preamble::MODE]),
- preamble::TGT_HLL4
+ }
+
+ #[test]
+ fn reads_java_updatable_hll4_with_aux_entries() {
+ assert_matches_java(
+ include_bytes!("testdata/hll4_updatable_aux_lgk12_100000.bin"),
+ 100_000,
+ 101148.48322141083,
+ 203363.1420443808,
);
- assert_eq!(bytes[preamble::FLAGS] & preamble::COMPACT_FLAG, 0);
- let aux_count = u32::from_le_bytes([
- bytes[preamble::AUX_COUNT],
- bytes[preamble::AUX_COUNT + 1],
- bytes[preamble::AUX_COUNT + 2],
- bytes[preamble::AUX_COUNT + 3],
- ]);
- assert!(aux_count > 0, "expected the sketch to carry aux entries");
-
- let err = SparkHllSketch::from_bytes(&bytes).unwrap_err().to_string();
- assert!(
- err.contains("auxiliary-map"),
- "expected a clear rejection, got {err}"
+ }
+
+ #[test]
+ fn reads_java_compact_hll4_with_aux_entries() {
+ assert_matches_java(
+ include_bytes!("testdata/hll4_compact_aux_lgk12_2000000.bin"),
+ 2_000_000,
+ 1993970.3281685747,
+ 4033480.112300425,
+ );
+ }
+
+ #[test]
+ fn reads_java_hll6() {
+ assert_matches_java(
+ include_bytes!("testdata/hll6_lgk12_100000.bin"),
+ 100_000,
+ 101148.48322141083,
+ 203363.1420443808,
);
}
- /// The guard is narrowed to the shape that is actually mis-decoded, so
ordinary HLL_4 input
- /// with no exceptions still reads. Rejecting HLL_4 outright would refuse
most third-party
- /// sketches for no reason.
+ /// Ordinary HLL_4 input, Java's default target type, with no exceptions
at all.
#[test]
- fn hll4_without_aux_entries_still_reads() {
+ fn hll4_without_aux_entries_reads() {
Review Comment:
This test was there to show the aux guard was narrow. With the guard gone,
it only round-trips an HLL_4 sketch through the crate itself, which Comet never
writes, and the Java fixtures cover HLL_4 input better. Consider removing it.
--
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]
