From 0d6570b0c1a3614fff14eef9f572298e4763d190 Mon Sep 17 00:00:00 2001 From: Jaideep Pyne Date: Wed, 5 Aug 2026 22:30:35 +0530 Subject: [PATCH 1/4] refactor: centralize seed hash validation --- datasketches/src/countmin/sketch.rs | 8 ++----- datasketches/src/cpc/sketch.rs | 12 ++--------- datasketches/src/hash/mod.rs | 17 +++++++++++++++ datasketches/src/thetafamily/theta/sketch.rs | 22 ++++---------------- datasketches/src/thetafamily/tuple/sketch.rs | 9 ++------ 5 files changed, 27 insertions(+), 41 deletions(-) diff --git a/datasketches/src/countmin/sketch.rs b/datasketches/src/countmin/sketch.rs index a2dcb58..f262f7f 100644 --- a/datasketches/src/countmin/sketch.rs +++ b/datasketches/src/countmin/sketch.rs @@ -33,6 +33,7 @@ use crate::countmin::serialization::SERIAL_VERSION; use crate::error::Error; use crate::hash::DEFAULT_UPDATE_SEED; use crate::hash::MurmurHash3X64128; +use crate::hash::check_seed_hash; use crate::hash::compute_seed_hash; const MAX_TABLE_ENTRIES: usize = 1 << 30; @@ -376,12 +377,7 @@ impl CountMinSketch { .map_err(insufficient_data("seed_hash"))?; cursor.read_u8().map_err(insufficient_data("unused8"))?; - let expected_seed_hash = compute_seed_hash(seed); - if seed_hash != expected_seed_hash { - return Err(Error::deserial(format!( - "incompatible seed hash: expected {expected_seed_hash}, got {seed_hash}", - ))); - } + check_seed_hash(seed_hash, seed)?; let entries = entries_for_config_checked(num_hashes, num_buckets)?; let mut sketch = Self::make(num_hashes, num_buckets, seed, entries); diff --git a/datasketches/src/cpc/sketch.rs b/datasketches/src/cpc/sketch.rs index 61f6c9b..422b026 100644 --- a/datasketches/src/cpc/sketch.rs +++ b/datasketches/src/cpc/sketch.rs @@ -48,6 +48,7 @@ use crate::error::Error; use crate::error::ErrorKind; use crate::hash::DEFAULT_UPDATE_SEED; use crate::hash::MurmurHash3X64128; +use crate::hash::check_seed_hash; use crate::hash::compute_seed_hash; /// A Compressed Probabilistic Counting sketch. @@ -625,16 +626,7 @@ impl CpcSketch { let expected_preamble_ints = make_preamble_ints(num_coupons, has_hip, has_table, has_window); ensure_preamble_longs_in(&[expected_preamble_ints], preamble_ints)?; - if seed_hash != compute_seed_hash(seed) { - return Err(Error::new( - ErrorKind::InvalidData, - format!( - "incompatible seed hash: expected {}, got {}", - compute_seed_hash(seed), - seed_hash - ), - )); - } + check_seed_hash(seed_hash, seed)?; if !(MIN_LG_K..=MAX_LG_K).contains(&lg_k) { return Err(Error::invalid_argument(format!( "lg_k out of range; got {}", diff --git a/datasketches/src/hash/mod.rs b/datasketches/src/hash/mod.rs index 41d879c..5be9ba2 100644 --- a/datasketches/src/hash/mod.rs +++ b/datasketches/src/hash/mod.rs @@ -92,6 +92,23 @@ pub(crate) fn compute_seed_hash(seed: u64) -> u16 { seed_hash } +/// Checks that a serialized seed hash matches the hash computed from `seed`. +#[cfg(any( + feature = "countmin", + feature = "cpc", + feature = "theta", + feature = "tuple", +))] +pub(crate) fn check_seed_hash(seed_hash: u16, seed: u64) -> Result<(), crate::error::Error> { + let expected_seed_hash = compute_seed_hash(seed); + if seed_hash != expected_seed_hash { + return Err(crate::error::Error::deserial(format!( + "incompatible seed hash: expected {expected_seed_hash}, got {seed_hash}", + ))); + } + Ok(()) +} + /// Reads an u64 from a byte slice in little-endian order. /// /// # Panics diff --git a/datasketches/src/thetafamily/theta/sketch.rs b/datasketches/src/thetafamily/theta/sketch.rs index 5c1f761..12e14da 100644 --- a/datasketches/src/thetafamily/theta/sketch.rs +++ b/datasketches/src/thetafamily/theta/sketch.rs @@ -31,6 +31,7 @@ use crate::common::NumStdDev; use crate::common::ResizeFactor; use crate::error::Error; use crate::hash::DEFAULT_UPDATE_SEED; +use crate::hash::check_seed_hash; use crate::hash::compute_seed_hash; use crate::theta::bit_pack::BLOCK_WIDTH; use crate::theta::bit_pack::BitPacker; @@ -674,12 +675,7 @@ impl CompactThetaSketch { let seed_hash = cursor .read_u16_le() .map_err(insufficient_data("seed_hash"))?; - let expected_seed_hash = compute_seed_hash(expected_seed); - if seed_hash != expected_seed_hash { - return Err(Error::deserial(format!( - "incompatible seed hash: expected {expected_seed_hash}, got {seed_hash}", - ))); - } + check_seed_hash(seed_hash, expected_seed)?; match pre_longs { V2_PREAMBLE_EMPTY => Ok(Self { @@ -749,12 +745,7 @@ impl CompactThetaSketch { let num_entries; let mut entries = vec![]; if !empty { - let expected_seed_hash = compute_seed_hash(expected_seed); - if seed_hash != expected_seed_hash { - return Err(Error::deserial(format!( - "incompatible seed hash: expected {expected_seed_hash}, got {seed_hash}", - ))); - } + check_seed_hash(seed_hash, expected_seed)?; if pre_longs == 1 { num_entries = 1; } else { @@ -795,12 +786,7 @@ impl CompactThetaSketch { .map_err(insufficient_data("seed_hash"))?; let empty = (flags & FLAGS_IS_EMPTY) != 0; if !empty { - let expected_seed_hash = compute_seed_hash(expected_seed); - if seed_hash != expected_seed_hash { - return Err(Error::deserial(format!( - "incompatible seed hash: expected {expected_seed_hash}, got {seed_hash}", - ))); - } + check_seed_hash(seed_hash, expected_seed)?; } let theta = if pre_longs > 1 { cursor diff --git a/datasketches/src/thetafamily/tuple/sketch.rs b/datasketches/src/thetafamily/tuple/sketch.rs index bd48322..05e47f4 100644 --- a/datasketches/src/thetafamily/tuple/sketch.rs +++ b/datasketches/src/thetafamily/tuple/sketch.rs @@ -33,7 +33,7 @@ use crate::common::NumStdDev; use crate::common::ResizeFactor; use crate::error::Error; use crate::hash::DEFAULT_UPDATE_SEED; -use crate::hash::compute_seed_hash; +use crate::hash::check_seed_hash; use crate::thetacommon::ThetaFamilySketchView; use crate::thetacommon::ThetaKeySketchView; use crate::thetacommon::binomial_bounds; @@ -561,12 +561,7 @@ impl CompactTupleSketch { )); } - let expected_seed_hash = compute_seed_hash(seed); - if seed_hash != expected_seed_hash { - return Err(Error::deserial(format!( - "incompatible seed hash: expected {expected_seed_hash}, got {seed_hash}", - ))); - } + check_seed_hash(seed_hash, seed)?; let mut theta = MAX_THETA; let num_entries = if pre_longs == 1 { From 6b193adab297af3f93acef7f3ff2dc08f76a4b83 Mon Sep 17 00:00:00 2001 From: Jaideep Pyne Date: Thu, 6 Aug 2026 04:26:54 +0530 Subject: [PATCH 2/4] refactor: preserve seed hash error contracts --- datasketches/src/countmin/sketch.rs | 6 +++- datasketches/src/cpc/sketch.rs | 7 ++++- datasketches/src/hash/mod.rs | 15 +++++----- datasketches/src/thetafamily/theta/sketch.rs | 30 ++++++++++++++++++-- datasketches/src/thetafamily/tuple/sketch.rs | 7 ++++- 5 files changed, 52 insertions(+), 13 deletions(-) diff --git a/datasketches/src/countmin/sketch.rs b/datasketches/src/countmin/sketch.rs index f262f7f..fb568d3 100644 --- a/datasketches/src/countmin/sketch.rs +++ b/datasketches/src/countmin/sketch.rs @@ -377,7 +377,11 @@ impl CountMinSketch { .map_err(insufficient_data("seed_hash"))?; cursor.read_u8().map_err(insufficient_data("unused8"))?; - check_seed_hash(seed_hash, seed)?; + check_seed_hash(compute_seed_hash(seed), seed_hash, |expected, actual| { + Error::deserial(format!( + "incompatible seed hash: expected {expected}, got {actual}", + )) + })?; let entries = entries_for_config_checked(num_hashes, num_buckets)?; let mut sketch = Self::make(num_hashes, num_buckets, seed, entries); diff --git a/datasketches/src/cpc/sketch.rs b/datasketches/src/cpc/sketch.rs index 422b026..e0c4112 100644 --- a/datasketches/src/cpc/sketch.rs +++ b/datasketches/src/cpc/sketch.rs @@ -626,7 +626,12 @@ impl CpcSketch { let expected_preamble_ints = make_preamble_ints(num_coupons, has_hip, has_table, has_window); ensure_preamble_longs_in(&[expected_preamble_ints], preamble_ints)?; - check_seed_hash(seed_hash, seed)?; + check_seed_hash(compute_seed_hash(seed), seed_hash, |expected, actual| { + Error::new( + ErrorKind::InvalidData, + format!("incompatible seed hash: expected {expected}, got {actual}"), + ) + })?; if !(MIN_LG_K..=MAX_LG_K).contains(&lg_k) { return Err(Error::invalid_argument(format!( "lg_k out of range; got {}", diff --git a/datasketches/src/hash/mod.rs b/datasketches/src/hash/mod.rs index 5be9ba2..d60c2f3 100644 --- a/datasketches/src/hash/mod.rs +++ b/datasketches/src/hash/mod.rs @@ -92,19 +92,20 @@ pub(crate) fn compute_seed_hash(seed: u64) -> u16 { seed_hash } -/// Checks that a serialized seed hash matches the hash computed from `seed`. +/// Checks that an actual seed hash matches the expected seed hash. #[cfg(any( feature = "countmin", feature = "cpc", feature = "theta", feature = "tuple", ))] -pub(crate) fn check_seed_hash(seed_hash: u16, seed: u64) -> Result<(), crate::error::Error> { - let expected_seed_hash = compute_seed_hash(seed); - if seed_hash != expected_seed_hash { - return Err(crate::error::Error::deserial(format!( - "incompatible seed hash: expected {expected_seed_hash}, got {seed_hash}", - ))); +pub(crate) fn check_seed_hash( + expected: u16, + actual: u16, + mismatch: impl FnOnce(u16, u16) -> E, +) -> Result<(), E> { + if actual != expected { + return Err(mismatch(expected, actual)); } Ok(()) } diff --git a/datasketches/src/thetafamily/theta/sketch.rs b/datasketches/src/thetafamily/theta/sketch.rs index 12e14da..09f1150 100644 --- a/datasketches/src/thetafamily/theta/sketch.rs +++ b/datasketches/src/thetafamily/theta/sketch.rs @@ -675,7 +675,15 @@ impl CompactThetaSketch { let seed_hash = cursor .read_u16_le() .map_err(insufficient_data("seed_hash"))?; - check_seed_hash(seed_hash, expected_seed)?; + check_seed_hash( + compute_seed_hash(expected_seed), + seed_hash, + |expected, actual| { + Error::deserial(format!( + "incompatible seed hash: expected {expected}, got {actual}", + )) + }, + )?; match pre_longs { V2_PREAMBLE_EMPTY => Ok(Self { @@ -745,7 +753,15 @@ impl CompactThetaSketch { let num_entries; let mut entries = vec![]; if !empty { - check_seed_hash(seed_hash, expected_seed)?; + check_seed_hash( + compute_seed_hash(expected_seed), + seed_hash, + |expected, actual| { + Error::deserial(format!( + "incompatible seed hash: expected {expected}, got {actual}", + )) + }, + )?; if pre_longs == 1 { num_entries = 1; } else { @@ -786,7 +802,15 @@ impl CompactThetaSketch { .map_err(insufficient_data("seed_hash"))?; let empty = (flags & FLAGS_IS_EMPTY) != 0; if !empty { - check_seed_hash(seed_hash, expected_seed)?; + check_seed_hash( + compute_seed_hash(expected_seed), + seed_hash, + |expected, actual| { + Error::deserial(format!( + "incompatible seed hash: expected {expected}, got {actual}", + )) + }, + )?; } let theta = if pre_longs > 1 { cursor diff --git a/datasketches/src/thetafamily/tuple/sketch.rs b/datasketches/src/thetafamily/tuple/sketch.rs index 05e47f4..5307466 100644 --- a/datasketches/src/thetafamily/tuple/sketch.rs +++ b/datasketches/src/thetafamily/tuple/sketch.rs @@ -34,6 +34,7 @@ use crate::common::ResizeFactor; use crate::error::Error; use crate::hash::DEFAULT_UPDATE_SEED; use crate::hash::check_seed_hash; +use crate::hash::compute_seed_hash; use crate::thetacommon::ThetaFamilySketchView; use crate::thetacommon::ThetaKeySketchView; use crate::thetacommon::binomial_bounds; @@ -561,7 +562,11 @@ impl CompactTupleSketch { )); } - check_seed_hash(seed_hash, seed)?; + check_seed_hash(compute_seed_hash(seed), seed_hash, |expected, actual| { + Error::deserial(format!( + "incompatible seed hash: expected {expected}, got {actual}", + )) + })?; let mut theta = MAX_THETA; let num_entries = if pre_longs == 1 { From 791af6b1fff7f3a045f9d11d7ff595eaaa598df1 Mon Sep 17 00:00:00 2001 From: tison Date: Thu, 6 Aug 2026 10:56:56 +0800 Subject: [PATCH 3/4] fixup Signed-off-by: tison --- datasketches/src/countmin/mod.rs | 4 +-- datasketches/src/countmin/sketch.rs | 20 +++++++------ datasketches/src/cpc/sketch.rs | 12 ++++---- datasketches/src/error.rs | 2 +- datasketches/src/hash/mod.rs | 15 +++++++--- .../src/thetafamily/common/a_not_b.rs | 28 +++++++++---------- .../src/thetafamily/common/intersection.rs | 20 +++++++------ .../thetafamily/common/jaccard_similarity.rs | 19 ++++--------- datasketches/src/thetafamily/common/union.rs | 18 ++++++------ datasketches/src/thetafamily/theta/sketch.rs | 22 +++++---------- datasketches/src/thetafamily/tuple/sketch.rs | 12 ++++---- datasketches/tests/serde_tests/countmin.rs | 2 +- 12 files changed, 86 insertions(+), 88 deletions(-) diff --git a/datasketches/src/countmin/mod.rs b/datasketches/src/countmin/mod.rs index 9572f11..0a0f0f4 100644 --- a/datasketches/src/countmin/mod.rs +++ b/datasketches/src/countmin/mod.rs @@ -15,9 +15,9 @@ // specific language governing permissions and limitations // under the License. -//! Count-Min sketch implementation for frequency estimation. +//! CountMin sketch implementation for frequency estimation. //! -//! The Count-Min sketch provides approximate frequency counts for streaming data +//! The CountMin sketch provides approximate frequency counts for streaming data //! with configurable relative error and confidence bounds. //! //! # Usage diff --git a/datasketches/src/countmin/sketch.rs b/datasketches/src/countmin/sketch.rs index fb568d3..8c38284 100644 --- a/datasketches/src/countmin/sketch.rs +++ b/datasketches/src/countmin/sketch.rs @@ -31,6 +31,7 @@ use crate::countmin::serialization::LONG_SIZE_BYTES; use crate::countmin::serialization::PREAMBLE_LONGS_SHORT; use crate::countmin::serialization::SERIAL_VERSION; use crate::error::Error; +use crate::error::ErrorKind; use crate::hash::DEFAULT_UPDATE_SEED; use crate::hash::MurmurHash3X64128; use crate::hash::check_seed_hash; @@ -38,7 +39,7 @@ use crate::hash::compute_seed_hash; const MAX_TABLE_ENTRIES: usize = 1 << 30; -/// Count-Min sketch for estimating item frequencies. +/// CountMin sketch for estimating item frequencies. /// /// The sketch provides upper and lower bounds on estimated item frequencies /// with configurable relative error and confidence. @@ -54,7 +55,7 @@ pub struct CountMinSketch { } impl CountMinSketch { - /// Creates a new Count-Min sketch with the default seed. + /// Creates a new CountMin sketch with the default seed. /// /// # Panics /// @@ -73,7 +74,7 @@ impl CountMinSketch { Self::with_seed(num_hashes, num_buckets, DEFAULT_UPDATE_SEED) } - /// Creates a new Count-Min sketch with the provided seed. + /// Creates a new CountMin sketch with the provided seed. /// /// # Panics /// @@ -265,7 +266,7 @@ impl CountMinSketch { self.total_weight = self.total_weight + other.total_weight; } - /// Serializes this sketch into the DataSketches Count-Min format. + /// Serializes this sketch into the DataSketches CountMin format. /// /// # Examples /// @@ -377,11 +378,12 @@ impl CountMinSketch { .map_err(insufficient_data("seed_hash"))?; cursor.read_u8().map_err(insufficient_data("unused8"))?; - check_seed_hash(compute_seed_hash(seed), seed_hash, |expected, actual| { - Error::deserial(format!( - "incompatible seed hash: expected {expected}, got {actual}", - )) - })?; + check_seed_hash( + compute_seed_hash(seed), + seed_hash, + "deserialized CountMinSketch", + ErrorKind::InvalidData, + )?; let entries = entries_for_config_checked(num_hashes, num_buckets)?; let mut sketch = Self::make(num_hashes, num_buckets, seed, entries); diff --git a/datasketches/src/cpc/sketch.rs b/datasketches/src/cpc/sketch.rs index e0c4112..d90a08a 100644 --- a/datasketches/src/cpc/sketch.rs +++ b/datasketches/src/cpc/sketch.rs @@ -626,12 +626,12 @@ impl CpcSketch { let expected_preamble_ints = make_preamble_ints(num_coupons, has_hip, has_table, has_window); ensure_preamble_longs_in(&[expected_preamble_ints], preamble_ints)?; - check_seed_hash(compute_seed_hash(seed), seed_hash, |expected, actual| { - Error::new( - ErrorKind::InvalidData, - format!("incompatible seed hash: expected {expected}, got {actual}"), - ) - })?; + check_seed_hash( + compute_seed_hash(seed), + seed_hash, + "deserialized CpcSketch", + ErrorKind::InvalidData, + )?; if !(MIN_LG_K..=MAX_LG_K).contains(&lg_k) { return Err(Error::invalid_argument(format!( "lg_k out of range; got {}", diff --git a/datasketches/src/error.rs b/datasketches/src/error.rs index a4fe5b9..094ab2d 100644 --- a/datasketches/src/error.rs +++ b/datasketches/src/error.rs @@ -116,7 +116,7 @@ impl Error { pub(crate) fn invalid_preamble_longs(expected: &[u8], actual: u8) -> Self { Error::deserial(format!( - "invalid preamble longs: expected {expected:?}, got {actual}" + "invalid preamble longs: expected one of {expected:?}, got {actual}" )) } } diff --git a/datasketches/src/hash/mod.rs b/datasketches/src/hash/mod.rs index d60c2f3..22ebdb1 100644 --- a/datasketches/src/hash/mod.rs +++ b/datasketches/src/hash/mod.rs @@ -17,6 +17,9 @@ //! Hashing support for sketches. +use crate::error::Error; +use crate::error::ErrorKind; + pub mod value; #[cfg(any( @@ -99,13 +102,17 @@ pub(crate) fn compute_seed_hash(seed: u64) -> u16 { feature = "theta", feature = "tuple", ))] -pub(crate) fn check_seed_hash( +pub(crate) fn check_seed_hash( expected: u16, actual: u16, - mismatch: impl FnOnce(u16, u16) -> E, -) -> Result<(), E> { + name: &'static str, + kind: ErrorKind, +) -> Result<(), Error> { if actual != expected { - return Err(mismatch(expected, actual)); + return Err(Error::new( + kind, + format!("incompatible seed hash of {name}: expected {expected}, got {actual}"), + )); } Ok(()) } diff --git a/datasketches/src/thetafamily/common/a_not_b.rs b/datasketches/src/thetafamily/common/a_not_b.rs index 782b960..3e12151 100644 --- a/datasketches/src/thetafamily/common/a_not_b.rs +++ b/datasketches/src/thetafamily/common/a_not_b.rs @@ -18,6 +18,8 @@ use std::collections::HashSet; use crate::error::Error; +use crate::error::ErrorKind; +use crate::hash::check_seed_hash; use crate::hash::compute_seed_hash; use crate::thetacommon::RetainedEntry; use crate::thetacommon::ThetaFamilySketchView; @@ -70,13 +72,12 @@ impl ANotBOperator { } // A is non-empty, so its seed must be compatible. - if a.seed_hash() != self.seed_hash { - return Err(Error::invalid_argument(format!( - "incompatible seed hash for A: expected {}, got {}", - self.seed_hash, - a.seed_hash() - ))); - } + check_seed_hash( + self.seed_hash, + a.seed_hash(), + "A", + ErrorKind::InvalidArgument, + )?; // An empty B subtracts nothing, so the result is simply a copy of A. This also covers the // "A is non-empty but has no retained keys" state: B's seed and theta must not influence @@ -86,13 +87,12 @@ impl ANotBOperator { } // B is non-empty, so its seed must be compatible. - if b.seed_hash() != self.seed_hash { - return Err(Error::invalid_argument(format!( - "incompatible seed hash for B: expected {}, got {}", - self.seed_hash, - b.seed_hash() - ))); - } + check_seed_hash( + self.seed_hash, + b.seed_hash(), + "B", + ErrorKind::InvalidArgument, + )?; let theta = a.theta64().min(b.theta64()); // A is non-empty here; the result only becomes empty if everything is subtracted in exact diff --git a/datasketches/src/thetafamily/common/intersection.rs b/datasketches/src/thetafamily/common/intersection.rs index 61cd74a..00e5ed4 100644 --- a/datasketches/src/thetafamily/common/intersection.rs +++ b/datasketches/src/thetafamily/common/intersection.rs @@ -17,6 +17,8 @@ use crate::common::ResizeFactor; use crate::error::Error; +use crate::error::ErrorKind; +use crate::hash::check_seed_hash; use crate::thetacommon::RetainedEntry; use crate::thetacommon::ThetaFamilySketchView; use crate::thetacommon::constants::HASH_TABLE_REBUILD_THRESHOLD; @@ -90,16 +92,15 @@ where return Ok(()); } - if !sketch.is_empty() && sketch.seed_hash() != self.table.seed_hash() { - return Err(Error::invalid_argument(format!( - "incompatible seed hash: expected {}, got {}", - self.table.seed_hash(), - sketch.seed_hash() - ))); - } - if sketch.is_empty() { self.table.set_empty(true); + } else { + check_seed_hash( + self.table.seed_hash(), + sketch.seed_hash(), + "intersection update", + ErrorKind::InvalidArgument, + )?; } self.table.set_theta(if self.table.is_empty() { @@ -257,6 +258,7 @@ where mod tests { use super::*; use crate::hash::DEFAULT_UPDATE_SEED; + use crate::hash::compute_seed_hash; use crate::thetacommon::ThetaKeySketchView; #[derive(Clone, Debug, Eq, PartialEq)] @@ -288,7 +290,7 @@ mod tests { impl ThetaKeySketchView for TestSketch { fn seed_hash(&self) -> u16 { - crate::hash::compute_seed_hash(DEFAULT_UPDATE_SEED) + compute_seed_hash(DEFAULT_UPDATE_SEED) } fn theta64(&self) -> u64 { diff --git a/datasketches/src/thetafamily/common/jaccard_similarity.rs b/datasketches/src/thetafamily/common/jaccard_similarity.rs index 8ad7bc0..d1164dc 100644 --- a/datasketches/src/thetafamily/common/jaccard_similarity.rs +++ b/datasketches/src/thetafamily/common/jaccard_similarity.rs @@ -17,6 +17,8 @@ use crate::common::ResizeFactor; use crate::error::Error; +use crate::error::ErrorKind; +use crate::hash::check_seed_hash; use crate::hash::compute_seed_hash; use crate::thetacommon::RetainedEntry; use crate::thetacommon::ThetaFamilySketchView; @@ -295,8 +297,9 @@ impl JaccardSimilarityOperator { A: ThetaKeySketchView, B: ThetaKeySketchView, { - self.validate_seed_hash(sketch_a)?; - self.validate_seed_hash(sketch_b)?; + let seed_hash = compute_seed_hash(self.seed); + check_seed_hash(seed_hash, sketch_a.seed_hash(), "A", ErrorKind::InvalidData)?; + check_seed_hash(seed_hash, sketch_b.seed_hash(), "B", ErrorKind::InvalidData)?; let sketch_a = KeySketchView::new(sketch_a); let sketch_b = KeySketchView::new(sketch_b); @@ -311,18 +314,6 @@ impl JaccardSimilarityOperator { union.update(&sketch_b)?; Ok(union.to_compact_parts(false)) } - - fn validate_seed_hash(&self, sketch: &S) -> Result<(), Error> { - let expected = compute_seed_hash(self.seed); - if expected != sketch.seed_hash() { - return Err(Error::invalid_argument(format!( - "incompatible seed hash: expected {}, got {}", - expected, - sketch.seed_hash(), - ))); - } - Ok(()) - } } /// Returns whether both sketches have the same retained keys and theta. diff --git a/datasketches/src/thetafamily/common/union.rs b/datasketches/src/thetafamily/common/union.rs index c22771f..5b4aebd 100644 --- a/datasketches/src/thetafamily/common/union.rs +++ b/datasketches/src/thetafamily/common/union.rs @@ -17,6 +17,8 @@ use crate::common::ResizeFactor; use crate::error::Error; +use crate::error::ErrorKind; +use crate::hash::check_seed_hash; use crate::thetacommon::RetainedEntry; use crate::thetacommon::ThetaFamilySketchView; use crate::thetacommon::constants::MAX_THETA; @@ -68,13 +70,12 @@ where return Ok(()); } - if self.table.seed_hash() != sketch.seed_hash() { - return Err(Error::invalid_argument(format!( - "incompatible seed hash: expected {}, got {}", - self.table.seed_hash(), - sketch.seed_hash(), - ))); - } + check_seed_hash( + self.table.seed_hash(), + sketch.seed_hash(), + "union update", + ErrorKind::InvalidArgument, + )?; self.table.set_empty(false); self.union_theta = self.union_theta.min(sketch.theta64()); @@ -163,6 +164,7 @@ where mod tests { use super::*; use crate::hash::DEFAULT_UPDATE_SEED; + use crate::hash::compute_seed_hash; use crate::thetacommon::ThetaKeySketchView; #[derive(Clone, Debug, Eq, PartialEq)] @@ -183,7 +185,7 @@ mod tests { impl ThetaKeySketchView for TestSketch { fn seed_hash(&self) -> u16 { - crate::hash::compute_seed_hash(DEFAULT_UPDATE_SEED) + compute_seed_hash(DEFAULT_UPDATE_SEED) } fn theta64(&self) -> u64 { diff --git a/datasketches/src/thetafamily/theta/sketch.rs b/datasketches/src/thetafamily/theta/sketch.rs index 09f1150..6a9662a 100644 --- a/datasketches/src/thetafamily/theta/sketch.rs +++ b/datasketches/src/thetafamily/theta/sketch.rs @@ -30,6 +30,7 @@ use crate::codec::family::Family; use crate::common::NumStdDev; use crate::common::ResizeFactor; use crate::error::Error; +use crate::error::ErrorKind; use crate::hash::DEFAULT_UPDATE_SEED; use crate::hash::check_seed_hash; use crate::hash::compute_seed_hash; @@ -678,11 +679,8 @@ impl CompactThetaSketch { check_seed_hash( compute_seed_hash(expected_seed), seed_hash, - |expected, actual| { - Error::deserial(format!( - "incompatible seed hash: expected {expected}, got {actual}", - )) - }, + "deserialized CompactThetaSketch v2", + ErrorKind::InvalidData, )?; match pre_longs { @@ -756,11 +754,8 @@ impl CompactThetaSketch { check_seed_hash( compute_seed_hash(expected_seed), seed_hash, - |expected, actual| { - Error::deserial(format!( - "incompatible seed hash: expected {expected}, got {actual}", - )) - }, + "deserialized CompactThetaSketch v3", + ErrorKind::InvalidData, )?; if pre_longs == 1 { num_entries = 1; @@ -805,11 +800,8 @@ impl CompactThetaSketch { check_seed_hash( compute_seed_hash(expected_seed), seed_hash, - |expected, actual| { - Error::deserial(format!( - "incompatible seed hash: expected {expected}, got {actual}", - )) - }, + "deserialized CompactThetaSketch v4", + ErrorKind::InvalidData, )?; } let theta = if pre_longs > 1 { diff --git a/datasketches/src/thetafamily/tuple/sketch.rs b/datasketches/src/thetafamily/tuple/sketch.rs index 5307466..21fa290 100644 --- a/datasketches/src/thetafamily/tuple/sketch.rs +++ b/datasketches/src/thetafamily/tuple/sketch.rs @@ -32,6 +32,7 @@ use crate::codec::family::Family; use crate::common::NumStdDev; use crate::common::ResizeFactor; use crate::error::Error; +use crate::error::ErrorKind; use crate::hash::DEFAULT_UPDATE_SEED; use crate::hash::check_seed_hash; use crate::hash::compute_seed_hash; @@ -562,11 +563,12 @@ impl CompactTupleSketch { )); } - check_seed_hash(compute_seed_hash(seed), seed_hash, |expected, actual| { - Error::deserial(format!( - "incompatible seed hash: expected {expected}, got {actual}", - )) - })?; + check_seed_hash( + compute_seed_hash(seed), + seed_hash, + "deserialized CompactTupleSketch", + ErrorKind::InvalidData, + )?; let mut theta = MAX_THETA; let num_entries = if pre_longs == 1 { diff --git a/datasketches/tests/serde_tests/countmin.rs b/datasketches/tests/serde_tests/countmin.rs index 49745f1..cfc6e06 100644 --- a/datasketches/tests/serde_tests/countmin.rs +++ b/datasketches/tests/serde_tests/countmin.rs @@ -24,7 +24,7 @@ use googletest::prelude::contains_substring; use crate::serialization_test_data; // This test validates binary format compatibility (deserialize + byte round-trip) for -// C++ Count-Min snapshots. It intentionally does not assert estimate equivalence against +// C++ CountMin snapshots. It intentionally does not assert estimate equivalence against // original input keys because per-row hash seed derivation differs across implementations. fn assert_cpp_snapshot( filename: &str, From bdce8b8a57319aa9d6d0cedcee0b837ce7de1a5c Mon Sep 17 00:00:00 2001 From: tison Date: Thu, 6 Aug 2026 11:00:28 +0800 Subject: [PATCH 4/4] fixup Signed-off-by: tison --- datasketches/src/hash/mod.rs | 69 +++++++++-------------------------- datasketches/src/hash/seed.rs | 55 ++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 52 deletions(-) create mode 100644 datasketches/src/hash/seed.rs diff --git a/datasketches/src/hash/mod.rs b/datasketches/src/hash/mod.rs index 22ebdb1..c5c26c4 100644 --- a/datasketches/src/hash/mod.rs +++ b/datasketches/src/hash/mod.rs @@ -17,9 +17,6 @@ //! Hashing support for sketches. -use crate::error::Error; -use crate::error::ErrorKind; - pub mod value; #[cfg(any( @@ -39,12 +36,27 @@ mod murmurhash; feature = "theta", feature = "tuple", ))] -pub(crate) use self::murmurhash::MurmurHash3X64128; +pub(crate) use self::murmurhash::*; #[cfg(feature = "bloom")] mod xxhash; #[cfg(feature = "bloom")] -pub(crate) use self::xxhash::XxHash64; +pub(crate) use self::xxhash::*; + +#[cfg(any( + feature = "countmin", + feature = "cpc", + feature = "theta", + feature = "tuple", +))] +mod seed; +#[cfg(any( + feature = "countmin", + feature = "cpc", + feature = "theta", + feature = "tuple", +))] +pub(crate) use self::seed::*; /// The seed 9001 used in the sketch update methods is a prime number that was chosen very early /// on in experimental testing. @@ -70,53 +82,6 @@ pub(crate) use self::xxhash::XxHash64; ))] pub(crate) const DEFAULT_UPDATE_SEED: u64 = 9001; -/// Computes and checks the 16-bit seed hash from the given long seed. -/// -/// The computed seed hash must not be zero in order to maintain compatibility with older -/// serialized versions that did not have this concept. -/// -/// # Panics -/// -/// Panics if the computed seed hash is zero. -#[cfg(any( - feature = "countmin", - feature = "cpc", - feature = "theta", - feature = "tuple", -))] -pub(crate) fn compute_seed_hash(seed: u64) -> u16 { - use std::hash::Hasher; - - let mut hasher = MurmurHash3X64128::with_seed(0); - hasher.write(&seed.to_le_bytes()); - let (h1, _) = hasher.finish128(); - let seed_hash = (h1 & 0xffff) as u16; - assert_ne!(seed_hash, 0); - seed_hash -} - -/// Checks that an actual seed hash matches the expected seed hash. -#[cfg(any( - feature = "countmin", - feature = "cpc", - feature = "theta", - feature = "tuple", -))] -pub(crate) fn check_seed_hash( - expected: u16, - actual: u16, - name: &'static str, - kind: ErrorKind, -) -> Result<(), Error> { - if actual != expected { - return Err(Error::new( - kind, - format!("incompatible seed hash of {name}: expected {expected}, got {actual}"), - )); - } - Ok(()) -} - /// Reads an u64 from a byte slice in little-endian order. /// /// # Panics diff --git a/datasketches/src/hash/seed.rs b/datasketches/src/hash/seed.rs new file mode 100644 index 0000000..4cd7a6f --- /dev/null +++ b/datasketches/src/hash/seed.rs @@ -0,0 +1,55 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use crate::error::Error; +use crate::error::ErrorKind; +use crate::hash::MurmurHash3X64128; + +/// Computes and checks the 16-bit seed hash from the given long seed. +/// +/// The computed seed hash must not be zero in order to maintain compatibility with older +/// serialized versions that did not have this concept. +/// +/// # Panics +/// +/// Panics if the computed seed hash is zero. +pub(crate) fn compute_seed_hash(seed: u64) -> u16 { + use std::hash::Hasher; + + let mut hasher = MurmurHash3X64128::with_seed(0); + hasher.write(&seed.to_le_bytes()); + let (h1, _) = hasher.finish128(); + let seed_hash = (h1 & 0xffff) as u16; + assert_ne!(seed_hash, 0); + seed_hash +} + +/// Checks that an actual seed hash matches the expected seed hash. +pub(crate) fn check_seed_hash( + expected: u16, + actual: u16, + name: &'static str, + kind: ErrorKind, +) -> Result<(), Error> { + if actual != expected { + return Err(Error::new( + kind, + format!("incompatible seed hash of {name}: expected {expected}, got {actual}"), + )); + } + Ok(()) +}