From d729a0616d96763897ead60b58f086e0c8521de8 Mon Sep 17 00:00:00 2001 From: tison Date: Thu, 5 Nov 2026 10:15:11 +0800 Subject: [PATCH 1/6] refactor(xor): simplify raw parts and construction --- CHANGELOG.md | 2 +- datasketches/src/xor/filter.rs | 276 ++++++++------------- datasketches/src/xor/mod.rs | 4 +- tests-integration/tests/serde_tests/xor.rs | 22 +- tests-integration/tests/xor_test/parts.rs | 29 +-- 5 files changed, 130 insertions(+), 203 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5503e35c..2d4dc3cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ All significant changes to this project will be documented in this file. ### New features * Add `HllUnion::serialize` to serialize union results directly. -* Add immutable `XorFilter` and `XorFilterBuilder` behind the `xor` feature, with 8-, 16-, and 32-bit fingerprints, precomputed-hash queries and borrowed distinct-hash construction, bounded seeded retries and compact temporary storage, optional cardinality metadata for imported filters, standard serialization for all three widths (Java-compatible for 8/16-bit filters), and raw parts for application-defined storage. +* Add immutable `XorFilter` and `XorFilterBuilder` behind the `xor` feature, with 8-, 16-, and 32-bit fingerprints, precomputed-hash queries and borrowed distinct-hash construction, bounded seeded retries and compact temporary storage, optional cardinality metadata for imported filters, standard serialization for all three widths (Java-compatible for 8/16-bit filters), and public raw parts with validated imports for application-defined storage. ### Improvements diff --git a/datasketches/src/xor/filter.rs b/datasketches/src/xor/filter.rs index 166e24ff..7c2e6d6a 100644 --- a/datasketches/src/xor/filter.rs +++ b/datasketches/src/xor/filter.rs @@ -96,20 +96,24 @@ impl XorFingerprints { /// Raw state of an [`XorFilter`] for application-defined storage. /// -/// Created by [`XorFilter::into_parts`] or [`XorFilterParts::new`], and consumed by -/// [`XorFilter::from_parts`], which validates the layout. +/// Created directly or by [`XorFilter::into_parts`], and consumed by +/// [`XorFilter::from_parts`], which validates the layout before it can be queried. #[derive(Debug, Clone, PartialEq, Eq)] pub struct XorFilterParts { - seed: u64, - segment_length: usize, - fingerprints: XorFingerprints, - num_items: Option, + /// Construction seed used to mix input hashes. + pub seed: u64, + /// Number of fingerprint slots in each of the three segments. + pub segment_length: usize, + /// Fingerprint slots ordered as three consecutive, equally sized segments. + pub fingerprints: XorFingerprints, + /// Number of distinct input hashes, or `None` if the storage format omits it. + /// + /// Standard serialization requires a known count. `Some(0)` makes every query return `false`. + pub num_items: Option, } impl XorFilterParts { - /// Creates parts from a construction seed, segment length, and fingerprint slots. - /// - /// The item count is unknown unless set with [`with_num_items`](Self::with_num_items). + /// Creates raw parts with an unknown item count for application-defined storage. pub fn new(seed: u64, segment_length: usize, fingerprints: XorFingerprints) -> Self { Self { seed, @@ -118,39 +122,6 @@ impl XorFilterParts { num_items: None, } } - - /// Records the number of distinct hashes in the original input set. - /// - /// Standard serialization and emptiness checks require a known item count. - pub fn with_num_items(mut self, num_items: usize) -> Self { - self.num_items = Some(num_items); - self - } - - /// Returns the construction seed. - pub fn seed(&self) -> u64 { - self.seed - } - - /// Returns the number of fingerprint slots in each of the three segments. - pub fn segment_length(&self) -> usize { - self.segment_length - } - - /// Returns the fingerprint slots. - pub fn fingerprints(&self) -> &XorFingerprints { - &self.fingerprints - } - - /// Returns the number of distinct input hashes, or `None` if unknown. - pub fn num_items(&self) -> Option { - self.num_items - } - - /// Returns the fingerprint slots without copying. - pub fn into_fingerprints(self) -> XorFingerprints { - self.fingerprints - } } /// An immutable xor filter for probabilistic set membership. @@ -190,17 +161,12 @@ impl XorFilter { /// ``` /// use datasketches::xor::XorFilter; /// use datasketches::xor::XorFilterBuilder; - /// use datasketches::xor::XorFilterParts; /// use datasketches::xor::XorFilterType; /// /// let filter = XorFilterBuilder::new(XorFilterType::Xor8).build_from_hashes([10, 20, 30])?; - /// let parts = filter.into_parts(); - /// // A storage format that does not record the item count keeps only these fields. - /// let parts = XorFilterParts::new( - /// parts.seed(), - /// parts.segment_length(), - /// parts.into_fingerprints(), - /// ); + /// let mut parts = filter.into_parts(); + /// // A storage format may omit the item count. + /// parts.num_items = None; /// let imported = XorFilter::from_parts(parts)?; /// assert!(imported.contains_hash(20)); /// assert_eq!(imported.num_items(), None); @@ -349,101 +315,6 @@ impl XorFilter { pub fn estimated_size(&self) -> usize { size_of::() + self.fingerprints.byte_len() } - - fn build( - keys: &[u64], - filter_type: XorFilterType, - base_seed: u64, - max_attempts: usize, - ) -> Result { - debug_assert!(max_attempts > 0); - let num_items = keys.len(); - let capacity = compute_capacity(num_items)?; - let segment_length = capacity / usize::from(NUM_HASHES); - let mut xor_mask = vec![0_u64; capacity]; - let mut count = vec![0_u32; capacity]; - // One spare slot per segment allows a speculative enqueue even when full. - let mut queue = vec![0_u32; capacity + 3]; - let mut stack_hash = vec![0_u64; num_items]; - let mut stack_index = vec![0_u32; num_items]; - - let mut rng_state = base_seed; - let mut construction_seed = 0; - let mut stack_size = 0; - for attempt in 0..max_attempts { - if attempt != 0 { - xor_mask.fill(0); - count.fill(0); - } - rng_state = rng_state.wrapping_add(SPLITMIX_GAMMA); - construction_seed = splitmix64(rng_state); - stack_size = map( - keys, - construction_seed, - segment_length, - &mut xor_mask, - &mut count, - &mut queue, - &mut stack_hash, - &mut stack_index, - ); - if stack_size == num_items { - break; - } - // For a fixed seed, mixing is a bijection, so two remaining hashes that cancel - // out are the same input hash. Reseeding cannot separate duplicates. - if count.iter().zip(&xor_mask).any(|(&c, &m)| c == 2 && m == 0) { - return Err( - Error::invalid_argument("xor filter input contains duplicate hashes") - .with_context("num_items", num_items), - ); - } - } - - if stack_size != num_items { - return Err(Error::invalid_argument(format!( - "xor filter construction failed after {max_attempts} attempts" - )) - .with_context("num_items", num_items)); - } - - // Assignment only needs the peel stack. Release the graph before allocating the - // fingerprint payload so these allocations do not overlap at peak memory usage. - drop(xor_mask); - drop(count); - drop(queue); - - let fingerprints = match filter_type { - XorFilterType::Xor8 => XorFingerprints::Xor8(assign_fingerprints( - capacity, - segment_length, - &stack_hash, - &stack_index, - |hash| fingerprint(hash) as u8, - )), - XorFilterType::Xor16 => XorFingerprints::Xor16(assign_fingerprints( - capacity, - segment_length, - &stack_hash, - &stack_index, - |hash| fingerprint(hash) as u16, - )), - XorFilterType::Xor32 => XorFingerprints::Xor32(assign_fingerprints( - capacity, - segment_length, - &stack_hash, - &stack_index, - |hash| fingerprint(hash) as u32, - )), - }; - - Ok(Self { - segment_length, - num_items: Some(num_items), - seed: construction_seed, - fingerprints, - }) - } } /// Builder for configuring and creating an immutable [`XorFilter`]. @@ -541,13 +412,93 @@ impl XorFilterBuilder { /// prevent peeling; a failed attempt usually exposes them and stops construction early, /// but detection is not guaranteed. Use [`build_from_hashes`](Self::build_from_hashes) to /// remove duplicates. - pub fn build_from_distinct_hashes(self, hashes: &[u64]) -> Result { - XorFilter::build( - hashes, - self.filter_type, - self.seed, - MAX_CONSTRUCTION_ATTEMPTS, - ) + pub fn build_from_distinct_hashes(self, keys: &[u64]) -> Result { + let num_items = keys.len(); + let capacity = compute_capacity(num_items)?; + let segment_length = capacity / usize::from(NUM_HASHES); + let mut xor_mask = vec![0_u64; capacity]; + let mut count = vec![0_u32; capacity]; + // One spare slot per segment allows a speculative enqueue even when full. + let mut queue = vec![0_u32; capacity + 3]; + let mut stack_hash = vec![0_u64; num_items]; + let mut stack_index = vec![0_u32; num_items]; + + let mut rng_state = self.seed; + let mut construction_seed = 0; + let mut stack_size = 0; + for attempt in 0..MAX_CONSTRUCTION_ATTEMPTS { + if attempt != 0 { + xor_mask.fill(0); + count.fill(0); + } + rng_state = rng_state.wrapping_add(SPLITMIX_GAMMA); + construction_seed = splitmix64(rng_state); + stack_size = map( + keys, + construction_seed, + segment_length, + &mut xor_mask, + &mut count, + &mut queue, + &mut stack_hash, + &mut stack_index, + ); + if stack_size == num_items { + break; + } + // For a fixed seed, mixing is a bijection, so two remaining hashes that cancel + // out are the same input hash. Reseeding cannot separate duplicates. + if count.iter().zip(&xor_mask).any(|(&c, &m)| c == 2 && m == 0) { + return Err( + Error::invalid_argument("xor filter input contains duplicate hashes") + .with_context("num_items", num_items), + ); + } + } + + if stack_size != num_items { + return Err(Error::invalid_argument(format!( + "xor filter construction failed after {MAX_CONSTRUCTION_ATTEMPTS} attempts" + )) + .with_context("num_items", num_items)); + } + + // Assignment only needs the peel stack. Release the graph before allocating the + // fingerprint payload so these allocations do not overlap at peak memory usage. + drop(xor_mask); + drop(count); + drop(queue); + + let fingerprints = match self.filter_type { + XorFilterType::Xor8 => XorFingerprints::Xor8(assign_fingerprints( + capacity, + segment_length, + &stack_hash, + &stack_index, + |hash| fingerprint(hash) as u8, + )), + XorFilterType::Xor16 => XorFingerprints::Xor16(assign_fingerprints( + capacity, + segment_length, + &stack_hash, + &stack_index, + |hash| fingerprint(hash) as u16, + )), + XorFilterType::Xor32 => XorFingerprints::Xor32(assign_fingerprints( + capacity, + segment_length, + &stack_hash, + &stack_index, + |hash| fingerprint(hash) as u32, + )), + }; + + Ok(XorFilter { + segment_length, + num_items: Some(num_items), + seed: construction_seed, + fingerprints, + }) } } @@ -784,25 +735,4 @@ mod tests { assert_eq!(hashes, [mix(42, 1)]); assert_eq!(indexes, [0]); } - - #[test] - fn borrowed_input_survives_exhaustion_and_can_retry() { - // This synthetic set fails its first peeling attempt with base seed 1. - // Limiting the private engine to one attempt exercises the public build failure path - // without relying on a rare failure across all 100 production attempts. - let keys: Vec<_> = (917_504..917_504 + 16_384u64) - .map(|i| splitmix64(i.wrapping_add(SPLITMIX_GAMMA))) - .collect(); - let error = XorFilter::build(&keys, XorFilterType::Xor8, 1, 1).unwrap_err(); - assert_eq!(error.kind(), ErrorKind::InvalidArgument); - - let filter = XorFilterBuilder::new(XorFilterType::Xor8) - .seed(1_u64.wrapping_add(SPLITMIX_GAMMA)) - .build_from_distinct_hashes(&keys) - .unwrap(); - assert_eq!(filter.num_items(), Some(keys.len())); - for &key in &keys { - assert!(filter.contains_hash(key)); - } - } } diff --git a/datasketches/src/xor/mod.rs b/datasketches/src/xor/mod.rs index 796db5fc..6250c6ff 100644 --- a/datasketches/src/xor/mod.rs +++ b/datasketches/src/xor/mod.rs @@ -15,9 +15,9 @@ // specific language governing permissions and limitations // under the License. -//! Xor filters for immutable probabilistic set membership. +//! XOR filters for immutable probabilistic set membership. //! -//! An xor filter is built from the complete input set and cannot be updated afterward. Compared +//! An XOR filter is built from the complete input set and cannot be updated afterward. Compared //! with a Bloom filter at the same false-positive probability, it generally needs less space and //! performs exactly three fingerprint reads per query. Construction needs temporary space linear //! in the number of distinct hashes. diff --git a/tests-integration/tests/serde_tests/xor.rs b/tests-integration/tests/serde_tests/xor.rs index e91fdde9..26ea69d3 100644 --- a/tests-integration/tests/serde_tests/xor.rs +++ b/tests-integration/tests/serde_tests/xor.rs @@ -256,14 +256,14 @@ fn xor32_image_preserves_full_width_little_endian_slots() { "0100000001000000", "67452301efcdab89ffffffff", )); - let filter = XorFilter::from_parts( - XorFilterParts::new( - 0x0123_4567_89ab_cdef, - 1, - XorFingerprints::Xor32(vec![0x0123_4567, 0x89ab_cdef, u32::MAX].into_boxed_slice()), - ) - .with_num_items(1), - ) + let filter = XorFilter::from_parts(XorFilterParts { + seed: 0x0123_4567_89ab_cdef, + segment_length: 1, + fingerprints: XorFingerprints::Xor32( + vec![0x0123_4567, 0x89ab_cdef, u32::MAX].into_boxed_slice(), + ), + num_items: Some(1), + }) .unwrap(); assert_eq!(filter.serialized_size().unwrap(), bytes.len()); assert_eq!(filter.serialize().unwrap(), bytes); @@ -320,9 +320,9 @@ fn raw_parts_preserve_an_application_storage_layout() { assert_eq!(filter.num_items(), None); let parts = filter.into_parts(); - assert_eq!(parts.num_items(), None); - let (seed, segment) = (parts.seed(), parts.segment_length()); - let payload = parts.into_fingerprints(); + assert_eq!(parts.num_items, None); + let (seed, segment) = (parts.seed, parts.segment_length); + let payload = parts.fingerprints; let mut encoded = b"TEST".to_vec(); encoded.push(width); encoded.extend((segment as u32).to_le_bytes()); diff --git a/tests-integration/tests/xor_test/parts.rs b/tests-integration/tests/xor_test/parts.rs index 90040263..40c0a6c1 100644 --- a/tests-integration/tests/xor_test/parts.rs +++ b/tests-integration/tests/xor_test/parts.rs @@ -75,9 +75,9 @@ fn raw_parts_interoperate_with_xorf_in_both_directions() { .build_from_distinct_hashes(&keys) .unwrap(); let parts = built.clone().into_parts(); - assert_eq!(parts.num_items(), Some(keys.len())); - let (seed, block_length) = (parts.seed(), parts.segment_length()); - let payload = parts.into_fingerprints(); + assert_eq!(parts.num_items, Some(keys.len())); + let (seed, block_length) = (parts.seed, parts.segment_length); + let payload = parts.fingerprints; // Compare all answers, including false positives, after transferring ownership. let reference: Box> = match payload { XorFingerprints::Xor8(fingerprints) => Box::new(xorf::Xor8 { @@ -111,11 +111,12 @@ fn raw_parts_reject_invalid_layouts_and_counts() { (usize::MAX, 3, None), (1, 3, Some(4)), ] { - let mut parts = - XorFilterParts::new(0, segment, XorFingerprints::Xor8(vec![0; length].into())); - if let Some(count) = count { - parts = parts.with_num_items(count); - } + let parts = XorFilterParts { + seed: 0, + segment_length: segment, + fingerprints: XorFingerprints::Xor8(vec![0; length].into()), + num_items: count, + }; assert_eq!( XorFilter::from_parts(parts).unwrap_err().kind(), ErrorKind::InvalidArgument @@ -202,13 +203,9 @@ fn unknown_count_does_not_infer_emptiness_from_zero_fingerprints() { let zero_hash = empty.seed().wrapping_neg(); assert_eq!(empty.is_empty(), Some(true)); assert!(!empty.contains_hash(zero_hash)); - let parts = empty.into_parts(); - let unknown = XorFilter::from_parts(XorFilterParts::new( - parts.seed(), - parts.segment_length(), - parts.into_fingerprints(), - )) - .unwrap(); + let mut parts = empty.into_parts(); + parts.num_items = None; + let unknown = XorFilter::from_parts(parts).unwrap(); assert_eq!(unknown.is_empty(), None); assert_eq!(unknown.num_items(), None); assert_eq!(unknown.bits_per_item(), None); @@ -223,7 +220,7 @@ fn unknown_count_does_not_infer_emptiness_from_zero_fingerprints() { ErrorKind::InvalidArgument ); let parts = unknown.into_parts(); - assert_eq!(parts.num_items(), None); + assert_eq!(parts.num_items, None); let restored = XorFilter::from_parts(parts).unwrap(); assert_eq!(restored.is_empty(), None); assert!(restored.contains_hash(zero_hash)); From b3ede562c5a37860105dbb73c526eec22ac496cb Mon Sep 17 00:00:00 2001 From: tison Date: Thu, 5 Nov 2026 11:46:05 +0800 Subject: [PATCH 2/6] refactor(xor): replace builders and parts with direct constructors --- CHANGELOG.md | 2 +- benchmarks/xor/build.rs | 38 +- benchmarks/xor/build_batches.rs | 19 +- benchmarks/xor/query.rs | 32 +- benchmarks/xor/query_shards.rs | 3 +- datasketches/src/xor/filter.rs | 423 +++++++++------------ datasketches/src/xor/mod.rs | 18 +- tests-integration/tests/serde_tests/xor.rs | 60 ++- tests-integration/tests/xor_test/filter.rs | 40 +- tests-integration/tests/xor_test/parts.rs | 71 ++-- 10 files changed, 279 insertions(+), 427 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d4dc3cc..c625b29a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ All significant changes to this project will be documented in this file. ### New features * Add `HllUnion::serialize` to serialize union results directly. -* Add immutable `XorFilter` and `XorFilterBuilder` behind the `xor` feature, with 8-, 16-, and 32-bit fingerprints, precomputed-hash queries and borrowed distinct-hash construction, bounded seeded retries and compact temporary storage, optional cardinality metadata for imported filters, standard serialization for all three widths (Java-compatible for 8/16-bit filters), and public raw parts with validated imports for application-defined storage. +* Add immutable `XorFilter` behind the `xor` feature, with 8-, 16-, and 32-bit fingerprints, direct construction from values or precomputed hashes, borrowed distinct-hash construction, bounded seeded retries and compact temporary storage, optional cardinality metadata for imported filters, standard serialization for all three widths (Java-compatible for 8/16-bit filters), and validated imports for application-defined storage. ### Improvements diff --git a/benchmarks/xor/build.rs b/benchmarks/xor/build.rs index 0a9426c6..bc602dbc 100644 --- a/benchmarks/xor/build.rs +++ b/benchmarks/xor/build.rs @@ -19,7 +19,7 @@ use std::hint::black_box; use benchmarks::Bencher; use benchmarks::Runner; -use datasketches::xor::XorFilterBuilder; +use datasketches::xor::XorFilter; use datasketches::xor::XorFilterType; use crate::xor::hashes; @@ -32,11 +32,7 @@ fn repeated_hashes(rows: usize) -> Vec { fn distinct_8(bencher: Bencher, cardinality: usize) { let keys = hashes(cardinality); bencher.bench_local(|| { - black_box( - XorFilterBuilder::new(XorFilterType::Xor8) - .build_from_distinct_hashes(&keys) - .unwrap(), - ) + black_box(XorFilter::from_distinct_hashes(&keys, XorFilterType::Xor8, 0).unwrap()) }); } @@ -48,11 +44,7 @@ fn xorf_distinct_8(bencher: Bencher, cardinality: usize) { fn deduplicate_8(bencher: Bencher, rows: usize) { let keys = repeated_hashes(rows); bencher.bench_local(|| { - black_box( - XorFilterBuilder::new(XorFilterType::Xor8) - .build_from_hashes(keys.iter().copied()) - .unwrap(), - ) + black_box(XorFilter::from_hashes(keys.iter().copied(), XorFilterType::Xor8, 0).unwrap()) }); } @@ -70,11 +62,7 @@ fn xorf_deduplicate_8(bencher: Bencher, rows: usize) { fn distinct_16(bencher: Bencher, cardinality: usize) { let keys = hashes(cardinality); bencher.bench_local(|| { - black_box( - XorFilterBuilder::new(XorFilterType::Xor16) - .build_from_distinct_hashes(&keys) - .unwrap(), - ) + black_box(XorFilter::from_distinct_hashes(&keys, XorFilterType::Xor16, 0).unwrap()) }); } @@ -86,11 +74,7 @@ fn xorf_distinct_16(bencher: Bencher, cardinality: usize) { fn deduplicate_16(bencher: Bencher, rows: usize) { let keys = repeated_hashes(rows); bencher.bench_local(|| { - black_box( - XorFilterBuilder::new(XorFilterType::Xor16) - .build_from_hashes(keys.iter().copied()) - .unwrap(), - ) + black_box(XorFilter::from_hashes(keys.iter().copied(), XorFilterType::Xor16, 0).unwrap()) }); } @@ -108,11 +92,7 @@ fn xorf_deduplicate_16(bencher: Bencher, rows: usize) { fn distinct_32(bencher: Bencher, cardinality: usize) { let keys = hashes(cardinality); bencher.bench_local(|| { - black_box( - XorFilterBuilder::new(XorFilterType::Xor32) - .build_from_distinct_hashes(&keys) - .unwrap(), - ) + black_box(XorFilter::from_distinct_hashes(&keys, XorFilterType::Xor32, 0).unwrap()) }); } @@ -124,11 +104,7 @@ fn xorf_distinct_32(bencher: Bencher, cardinality: usize) { fn deduplicate_32(bencher: Bencher, rows: usize) { let keys = repeated_hashes(rows); bencher.bench_local(|| { - black_box( - XorFilterBuilder::new(XorFilterType::Xor32) - .build_from_hashes(keys.iter().copied()) - .unwrap(), - ) + black_box(XorFilter::from_hashes(keys.iter().copied(), XorFilterType::Xor32, 0).unwrap()) }); } diff --git a/benchmarks/xor/build_batches.rs b/benchmarks/xor/build_batches.rs index dff95af0..a8cc46df 100644 --- a/benchmarks/xor/build_batches.rs +++ b/benchmarks/xor/build_batches.rs @@ -19,14 +19,14 @@ use std::hint::black_box; use benchmarks::Bencher; use benchmarks::Runner; -use datasketches::xor::XorFilterBuilder; +use datasketches::xor::XorFilter; use datasketches::xor::XorFilterType; use crate::xor::hashes_with_offset; fn build(bencher: Bencher, cardinality: usize, build: impl Fn(&[u64]) -> F) { - // Multiple independent sets keep a particular seed's retry count on one set - // from dominating the comparison of default construction policies. + // Multiple independent sets keep one set's retry count from dominating the comparison. + // The deduplicating constructor receives the same already-distinct inputs. let inputs: Vec<_> = (0..32) .map(|i| hashes_with_offset(cardinality, i * cardinality as u64)) .collect(); @@ -38,23 +38,20 @@ fn build(bencher: Bencher, cardinality: usize, build: impl Fn(&[u64]) -> F) { } pub fn register(runner: &mut Runner) { + // Use xorf's initial seed for each constructor. for (cardinality, rounds) in [(128, 10_000), (16_384, 40), (50_000, 12)] { runner.compare( &format!("xor::build_batches::8/{cardinality}"), rounds, &[ - ("default_seed", &|b| { + ("distinct", &|b| { build(b, cardinality, |keys| { - XorFilterBuilder::new(XorFilterType::Xor8) - .build_from_distinct_hashes(keys) - .unwrap() + XorFilter::from_distinct_hashes(keys, XorFilterType::Xor8, 1).unwrap() }) }), - ("matched_seed", &|b| { + ("deduplicate", &|b| { build(b, cardinality, |keys| { - XorFilterBuilder::new(XorFilterType::Xor8) - .seed(1) - .build_from_distinct_hashes(keys) + XorFilter::from_hashes(keys.iter().copied(), XorFilterType::Xor8, 1) .unwrap() }) }), diff --git a/benchmarks/xor/query.rs b/benchmarks/xor/query.rs index 3d3f0d2c..008465f0 100644 --- a/benchmarks/xor/query.rs +++ b/benchmarks/xor/query.rs @@ -19,7 +19,7 @@ use std::hint::black_box; use benchmarks::Bencher; use benchmarks::Runner; -use datasketches::xor::XorFilterBuilder; +use datasketches::xor::XorFilter; use datasketches::xor::XorFilterType; use xorf::Filter as _; @@ -27,9 +27,8 @@ use crate::xor::hashes; fn datasketches_8_present(bencher: Bencher, cardinality: usize) { let keys = hashes(cardinality * 2); - let filter = XorFilterBuilder::new(XorFilterType::Xor8) - .build_from_distinct_hashes(&keys[..cardinality]) - .unwrap(); + let filter = + XorFilter::from_distinct_hashes(&keys[..cardinality], XorFilterType::Xor8, 0).unwrap(); bencher.bench_local(|| { for key in &keys[..cardinality] { black_box(filter.contains_hash(black_box(*key))); @@ -39,9 +38,8 @@ fn datasketches_8_present(bencher: Bencher, cardinality: usize) { fn datasketches_8_absent(bencher: Bencher, cardinality: usize) { let keys = hashes(cardinality * 2); - let filter = XorFilterBuilder::new(XorFilterType::Xor8) - .build_from_distinct_hashes(&keys[..cardinality]) - .unwrap(); + let filter = + XorFilter::from_distinct_hashes(&keys[..cardinality], XorFilterType::Xor8, 0).unwrap(); bencher.bench_local(|| { for key in &keys[cardinality..] { black_box(filter.contains_hash(black_box(*key))); @@ -71,9 +69,8 @@ fn xorf_8_absent(bencher: Bencher, cardinality: usize) { fn datasketches_16_present(bencher: Bencher, cardinality: usize) { let keys = hashes(cardinality * 2); - let filter = XorFilterBuilder::new(XorFilterType::Xor16) - .build_from_distinct_hashes(&keys[..cardinality]) - .unwrap(); + let filter = + XorFilter::from_distinct_hashes(&keys[..cardinality], XorFilterType::Xor16, 0).unwrap(); bencher.bench_local(|| { for key in &keys[..cardinality] { black_box(filter.contains_hash(black_box(*key))); @@ -83,9 +80,8 @@ fn datasketches_16_present(bencher: Bencher, cardinality: usize) { fn datasketches_16_absent(bencher: Bencher, cardinality: usize) { let keys = hashes(cardinality * 2); - let filter = XorFilterBuilder::new(XorFilterType::Xor16) - .build_from_distinct_hashes(&keys[..cardinality]) - .unwrap(); + let filter = + XorFilter::from_distinct_hashes(&keys[..cardinality], XorFilterType::Xor16, 0).unwrap(); bencher.bench_local(|| { for key in &keys[cardinality..] { black_box(filter.contains_hash(black_box(*key))); @@ -115,9 +111,8 @@ fn xorf_16_absent(bencher: Bencher, cardinality: usize) { fn datasketches_32_present(bencher: Bencher, cardinality: usize) { let keys = hashes(cardinality * 2); - let filter = XorFilterBuilder::new(XorFilterType::Xor32) - .build_from_distinct_hashes(&keys[..cardinality]) - .unwrap(); + let filter = + XorFilter::from_distinct_hashes(&keys[..cardinality], XorFilterType::Xor32, 0).unwrap(); bencher.bench_local(|| { for key in &keys[..cardinality] { black_box(filter.contains_hash(black_box(*key))); @@ -127,9 +122,8 @@ fn datasketches_32_present(bencher: Bencher, cardinality: usize) { fn datasketches_32_absent(bencher: Bencher, cardinality: usize) { let keys = hashes(cardinality * 2); - let filter = XorFilterBuilder::new(XorFilterType::Xor32) - .build_from_distinct_hashes(&keys[..cardinality]) - .unwrap(); + let filter = + XorFilter::from_distinct_hashes(&keys[..cardinality], XorFilterType::Xor32, 0).unwrap(); bencher.bench_local(|| { for key in &keys[cardinality..] { black_box(filter.contains_hash(black_box(*key))); diff --git a/benchmarks/xor/query_shards.rs b/benchmarks/xor/query_shards.rs index 9cd5baa1..4a52ede3 100644 --- a/benchmarks/xor/query_shards.rs +++ b/benchmarks/xor/query_shards.rs @@ -20,7 +20,6 @@ use std::hint::black_box; use benchmarks::Bencher; use benchmarks::Runner; use datasketches::xor::XorFilter; -use datasketches::xor::XorFilterParts; use datasketches::xor::XorFilterType; use datasketches::xor::XorFingerprints; use xorf::Filter; @@ -74,7 +73,7 @@ impl Reference { XorFingerprints::Xor32(f.fingerprints), ), }; - XorFilter::from_parts(XorFilterParts::new(seed, segment, fingerprints)).unwrap() + XorFilter::from_parts(seed, segment, fingerprints, None).unwrap() } } diff --git a/datasketches/src/xor/filter.rs b/datasketches/src/xor/filter.rs index 7c2e6d6a..e0dfff89 100644 --- a/datasketches/src/xor/filter.rs +++ b/datasketches/src/xor/filter.rs @@ -24,7 +24,6 @@ use crate::hash::XxHash64; const NUM_HASHES: u8 = 3; const HASH_SEED: u64 = 0; -const DEFAULT_CONSTRUCTION_SEED: u64 = 0; const LOAD_FACTOR: f64 = 1.23; const CAPACITY_OFFSET: u64 = 32; const MAX_CONSTRUCTION_ATTEMPTS: usize = 100; @@ -94,36 +93,6 @@ impl XorFingerprints { } } -/// Raw state of an [`XorFilter`] for application-defined storage. -/// -/// Created directly or by [`XorFilter::into_parts`], and consumed by -/// [`XorFilter::from_parts`], which validates the layout before it can be queried. -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct XorFilterParts { - /// Construction seed used to mix input hashes. - pub seed: u64, - /// Number of fingerprint slots in each of the three segments. - pub segment_length: usize, - /// Fingerprint slots ordered as three consecutive, equally sized segments. - pub fingerprints: XorFingerprints, - /// Number of distinct input hashes, or `None` if the storage format omits it. - /// - /// Standard serialization requires a known count. `Some(0)` makes every query return `false`. - pub num_items: Option, -} - -impl XorFilterParts { - /// Creates raw parts with an unknown item count for application-defined storage. - pub fn new(seed: u64, segment_length: usize, fingerprints: XorFingerprints) -> Self { - Self { - seed, - segment_length, - fingerprints, - num_items: None, - } - } -} - /// An immutable xor filter for probabilistic set membership. /// /// A query that returns `false` proves that the item was not in the input set. A query that returns @@ -133,8 +102,8 @@ impl XorFilterParts { /// /// Ordinary values passed to [`contains`](Self::contains) are reduced to 64-bit hashes with /// xxHash64 and seed `0`. Use [`contains_hash`](Self::contains_hash) only with the same hashes that -/// were supplied to [`XorFilterBuilder::build_from_hashes`] or -/// [`XorFilterBuilder::build_from_distinct_hashes`]. +/// were supplied to [`from_hashes`](Self::from_hashes) or +/// [`from_distinct_hashes`](Self::from_distinct_hashes). #[derive(Debug, Clone, PartialEq, Eq)] pub struct XorFilter { pub(crate) segment_length: usize, @@ -144,11 +113,176 @@ pub struct XorFilter { } impl XorFilter { - /// Imports raw parts without requiring a DataSketches serialization header. + /// Creates a filter from values hashed by xxHash64 with seed `0`. + /// + /// Removes duplicate hashes before construction. The construction `seed` selects a + /// deterministic sequence of attempts; it does not change the hash function used for values. + /// Pass borrowed values to retain them after construction. + /// + /// # Errors + /// + /// Returns an error under the same conditions as [`from_hashes`](Self::from_hashes). + /// + /// # Examples + /// + /// ``` + /// use datasketches::xor::XorFilter; + /// use datasketches::xor::XorFilterType; + /// + /// let filter = XorFilter::from_values(["apple", "banana"], XorFilterType::Xor8, 0)?; + /// assert!(filter.contains("apple")); + /// # Ok::<(), datasketches::error::Error>(()) + /// ``` + pub fn from_values( + items: impl IntoIterator, + filter_type: XorFilterType, + seed: u64, + ) -> Result { + Self::from_hashes( + items.into_iter().map(|item| hash_item(&item)), + filter_type, + seed, + ) + } + + /// Creates a filter from precomputed 64-bit hashes, removing duplicates. + /// + /// This method bypasses xxHash64. Query the resulting filter with + /// [`contains_hash`](Self::contains_hash) using hashes from the same source. + /// The construction `seed` selects a deterministic sequence of attempts. + /// Pass `hashes.iter().copied()` to retain the original input after construction. + /// + /// # Errors + /// + /// Returns [`InvalidArgument`](crate::error::ErrorKind::InvalidArgument) if the fingerprint + /// slot count exceeds `u32::MAX` or a scratch allocation would exceed `isize::MAX` bytes, + /// or if no peelable construction is found within 100 attempts. Valid input can also + /// exhaust the construction attempts. A successful build may still exceed the limits of + /// [`serialize`](Self::serialize). + pub fn from_hashes( + hashes: impl IntoIterator, + filter_type: XorFilterType, + seed: u64, + ) -> Result { + let mut hashes: Vec<_> = hashes.into_iter().collect(); + hashes.sort_unstable(); + hashes.dedup(); + Self::from_distinct_hashes(&hashes, filter_type, seed) + } + + /// Creates a filter from distinct precomputed hashes without copying or sorting them. + /// + /// The caller must supply distinct hashes. Use [`from_hashes`](Self::from_hashes) when + /// duplicates may be present. This method borrows the input without modifying it. + /// The construction `seed` selects a deterministic sequence of attempts; input order + /// does not affect the result. + /// + /// # Errors + /// + /// Returns [`InvalidArgument`](crate::error::ErrorKind::InvalidArgument) under the size limits + /// described by [`from_hashes`](Self::from_hashes), or if no peelable construction is found + /// within 100 attempts, even for valid input. Duplicate hashes prevent construction and + /// return an error; they are not removed by this method. + pub fn from_distinct_hashes( + keys: &[u64], + filter_type: XorFilterType, + seed: u64, + ) -> Result { + let num_items = keys.len(); + let capacity = compute_capacity(num_items)?; + let segment_length = capacity / usize::from(NUM_HASHES); + let mut xor_mask = vec![0_u64; capacity]; + let mut count = vec![0_u32; capacity]; + // One spare slot per segment allows a speculative enqueue even when full. + let mut queue = vec![0_u32; capacity + 3]; + let mut stack_hash = vec![0_u64; num_items]; + let mut stack_index = vec![0_u32; num_items]; + + let mut rng_state = seed; + let mut construction_seed = 0; + let mut stack_size = 0; + for attempt in 0..MAX_CONSTRUCTION_ATTEMPTS { + if attempt != 0 { + xor_mask.fill(0); + count.fill(0); + } + rng_state = rng_state.wrapping_add(SPLITMIX_GAMMA); + construction_seed = splitmix64(rng_state); + stack_size = map( + keys, + construction_seed, + segment_length, + &mut xor_mask, + &mut count, + &mut queue, + &mut stack_hash, + &mut stack_index, + ); + if stack_size == num_items { + break; + } + // For a fixed seed, mixing is a bijection, so two remaining hashes that cancel + // out are the same input hash. Reseeding cannot separate duplicates. + if count.iter().zip(&xor_mask).any(|(&c, &m)| c == 2 && m == 0) { + return Err( + Error::invalid_argument("xor filter input contains duplicate hashes") + .with_context("num_items", num_items), + ); + } + } + + if stack_size != num_items { + return Err(Error::invalid_argument(format!( + "xor filter construction failed after {MAX_CONSTRUCTION_ATTEMPTS} attempts" + )) + .with_context("num_items", num_items)); + } + + // Assignment only needs the peel stack. Release the graph before allocating the + // fingerprint payload so these allocations do not overlap at peak memory usage. + drop(xor_mask); + drop(count); + drop(queue); + + let fingerprints = match filter_type { + XorFilterType::Xor8 => XorFingerprints::Xor8(assign_fingerprints( + capacity, + segment_length, + &stack_hash, + &stack_index, + |hash| fingerprint(hash) as u8, + )), + XorFilterType::Xor16 => XorFingerprints::Xor16(assign_fingerprints( + capacity, + segment_length, + &stack_hash, + &stack_index, + |hash| fingerprint(hash) as u16, + )), + XorFilterType::Xor32 => XorFingerprints::Xor32(assign_fingerprints( + capacity, + segment_length, + &stack_hash, + &stack_index, + |hash| fingerprint(hash) as u32, + )), + }; + + Ok(Self { + segment_length, + num_items: Some(num_items), + seed: construction_seed, + fingerprints, + }) + } + + /// Imports a filter from its raw fields without requiring a DataSketches serialization header. /// /// The caller must preserve the original construction seed and fingerprint order, and query /// with the original input hashes. A supplied item count must describe the original set. /// A valid layout alone cannot establish these invariants. + /// Pass `None` when the storage format omits the item count. Standard serialization requires + /// a known count; a known zero count makes every query return `false`. /// /// # Errors /// @@ -160,25 +294,25 @@ impl XorFilter { /// /// ``` /// use datasketches::xor::XorFilter; - /// use datasketches::xor::XorFilterBuilder; /// use datasketches::xor::XorFilterType; /// - /// let filter = XorFilterBuilder::new(XorFilterType::Xor8).build_from_hashes([10, 20, 30])?; - /// let mut parts = filter.into_parts(); - /// // A storage format may omit the item count. - /// parts.num_items = None; - /// let imported = XorFilter::from_parts(parts)?; + /// let filter = XorFilter::from_hashes([10, 20, 30], XorFilterType::Xor8, 0)?; + /// let imported = XorFilter::from_parts( + /// filter.seed(), + /// filter.segment_length(), + /// filter.fingerprints().clone(), + /// None, + /// )?; /// assert!(imported.contains_hash(20)); /// assert_eq!(imported.num_items(), None); /// # Ok::<(), datasketches::error::Error>(()) /// ``` - pub fn from_parts(parts: XorFilterParts) -> Result { - let XorFilterParts { - seed, - segment_length, - fingerprints, - num_items, - } = parts; + pub fn from_parts( + seed: u64, + segment_length: usize, + fingerprints: XorFingerprints, + num_items: Option, + ) -> Result { if segment_length == 0 || segment_length > u32::MAX as usize || segment_length.checked_mul(3) != Some(fingerprints.len()) @@ -210,16 +344,6 @@ impl XorFilter { &self.fingerprints } - /// Converts the filter into raw parts without copying the fingerprint slots. - pub fn into_parts(self) -> XorFilterParts { - XorFilterParts { - seed: self.seed, - segment_length: self.segment_length, - fingerprints: self.fingerprints, - num_items: self.num_items, - } - } - /// Returns `true` if an item is possibly in the input set. /// /// A `false` result means the item was definitely absent; a `true` result may be a false @@ -317,191 +441,6 @@ impl XorFilter { } } -/// Builder for configuring and creating an immutable [`XorFilter`]. -/// -/// Stores only the fingerprint type and construction seed. Each build consumes the configuration -/// and takes the complete input set. Ordinary values and precomputed hashes are deduplicated in -/// temporary storage; already-distinct hashes can be borrowed without copying or sorting. -#[derive(Debug, Clone)] -pub struct XorFilterBuilder { - filter_type: XorFilterType, - seed: u64, -} - -impl XorFilterBuilder { - /// Creates a builder for the requested fingerprint representation. - /// - /// The default base construction seed is `0`. - pub fn new(filter_type: XorFilterType) -> Self { - Self { - filter_type, - seed: DEFAULT_CONSTRUCTION_SEED, - } - } - - /// Sets the base seed used to derive construction attempts. - /// - /// A fixed seed makes construction deterministic for a given set of hashes. The filter stores - /// the successful derived seed, which is returned by [`XorFilter::seed`]. - pub fn seed(mut self, seed: u64) -> Self { - self.seed = seed; - self - } - - /// Builds a filter from values hashed by xxHash64 with seed `0`. - /// - /// Duplicate hashes are removed before construction. The input is consumed as an iterator; - /// pass borrowed values to retain them for a later build, including after an error. - /// - /// # Errors - /// - /// Returns an error under the same conditions as - /// [`build_from_hashes`](Self::build_from_hashes). - /// - /// # Examples - /// - /// ``` - /// use datasketches::xor::XorFilterBuilder; - /// use datasketches::xor::XorFilterType; - /// - /// let filter = XorFilterBuilder::new(XorFilterType::Xor8) - /// .seed(42) - /// .build(["apple", "banana"])?; - /// assert!(filter.contains("apple")); - /// # Ok::<(), datasketches::error::Error>(()) - /// ``` - pub fn build(self, items: impl IntoIterator) -> Result { - self.build_from_hashes(items.into_iter().map(|item| hash_item(&item))) - } - - /// Builds a filter from precomputed 64-bit hashes, removing duplicates. - /// - /// This method bypasses xxHash64. Query the resulting filter with - /// [`XorFilter::contains_hash`] using hashes from the same source. Temporary input storage is - /// released on success or failure. Pass `hashes.iter().copied()` to retain the original input - /// for a later build, including after an error. - /// - /// # Errors - /// - /// Returns [`InvalidArgument`](crate::error::ErrorKind::InvalidArgument) if the fingerprint - /// slot count exceeds `u32::MAX` or a scratch allocation would exceed `isize::MAX` bytes, - /// or if no peelable construction is found within 100 attempts. Valid input can also - /// exhaust the construction attempts. A successful build may still exceed the limits of - /// [`XorFilter::serialize`]. - pub fn build_from_hashes( - self, - hashes: impl IntoIterator, - ) -> Result { - let mut hashes: Vec<_> = hashes.into_iter().collect(); - hashes.sort_unstable(); - hashes.dedup(); - self.build_from_distinct_hashes(&hashes) - } - - /// Builds a filter from distinct precomputed hashes without copying or sorting them. - /// - /// Borrows the input without modifying it, so the same slice can be retried with another - /// construction seed after an error. Changing the seed does not guarantee success. - /// Input order does not affect the result. - /// - /// # Errors - /// - /// Returns [`InvalidArgument`](crate::error::ErrorKind::InvalidArgument) under the size limits - /// described by [`build_from_hashes`](Self::build_from_hashes), or if no peelable - /// construction is found within 100 attempts, even for valid input. Duplicate hashes - /// prevent peeling; a failed attempt usually exposes them and stops construction early, - /// but detection is not guaranteed. Use [`build_from_hashes`](Self::build_from_hashes) to - /// remove duplicates. - pub fn build_from_distinct_hashes(self, keys: &[u64]) -> Result { - let num_items = keys.len(); - let capacity = compute_capacity(num_items)?; - let segment_length = capacity / usize::from(NUM_HASHES); - let mut xor_mask = vec![0_u64; capacity]; - let mut count = vec![0_u32; capacity]; - // One spare slot per segment allows a speculative enqueue even when full. - let mut queue = vec![0_u32; capacity + 3]; - let mut stack_hash = vec![0_u64; num_items]; - let mut stack_index = vec![0_u32; num_items]; - - let mut rng_state = self.seed; - let mut construction_seed = 0; - let mut stack_size = 0; - for attempt in 0..MAX_CONSTRUCTION_ATTEMPTS { - if attempt != 0 { - xor_mask.fill(0); - count.fill(0); - } - rng_state = rng_state.wrapping_add(SPLITMIX_GAMMA); - construction_seed = splitmix64(rng_state); - stack_size = map( - keys, - construction_seed, - segment_length, - &mut xor_mask, - &mut count, - &mut queue, - &mut stack_hash, - &mut stack_index, - ); - if stack_size == num_items { - break; - } - // For a fixed seed, mixing is a bijection, so two remaining hashes that cancel - // out are the same input hash. Reseeding cannot separate duplicates. - if count.iter().zip(&xor_mask).any(|(&c, &m)| c == 2 && m == 0) { - return Err( - Error::invalid_argument("xor filter input contains duplicate hashes") - .with_context("num_items", num_items), - ); - } - } - - if stack_size != num_items { - return Err(Error::invalid_argument(format!( - "xor filter construction failed after {MAX_CONSTRUCTION_ATTEMPTS} attempts" - )) - .with_context("num_items", num_items)); - } - - // Assignment only needs the peel stack. Release the graph before allocating the - // fingerprint payload so these allocations do not overlap at peak memory usage. - drop(xor_mask); - drop(count); - drop(queue); - - let fingerprints = match self.filter_type { - XorFilterType::Xor8 => XorFingerprints::Xor8(assign_fingerprints( - capacity, - segment_length, - &stack_hash, - &stack_index, - |hash| fingerprint(hash) as u8, - )), - XorFilterType::Xor16 => XorFingerprints::Xor16(assign_fingerprints( - capacity, - segment_length, - &stack_hash, - &stack_index, - |hash| fingerprint(hash) as u16, - )), - XorFilterType::Xor32 => XorFingerprints::Xor32(assign_fingerprints( - capacity, - segment_length, - &stack_hash, - &stack_index, - |hash| fingerprint(hash) as u32, - )), - }; - - Ok(XorFilter { - segment_length, - num_items: Some(num_items), - seed: construction_seed, - fingerprints, - }) - } -} - fn hash_item(item: &T) -> u64 { let mut hasher = XxHash64::with_seed(HASH_SEED); item.hash(&mut hasher); diff --git a/datasketches/src/xor/mod.rs b/datasketches/src/xor/mod.rs index 6250c6ff..b126ae56 100644 --- a/datasketches/src/xor/mod.rs +++ b/datasketches/src/xor/mod.rs @@ -22,8 +22,8 @@ //! performs exactly three fingerprint reads per query. Construction needs temporary space linear //! in the number of distinct hashes. //! -//! The [`XorFilterBuilder`] hashes ordinary Rust values with xxHash64. Call -//! [`XorFilterBuilder::build_from_hashes`] or [`XorFilterBuilder::build_from_distinct_hashes`] +//! [`XorFilter::from_values`] hashes ordinary Rust values with xxHash64. Call +//! [`XorFilter::from_hashes`] or [`XorFilter::from_distinct_hashes`] //! when hashes have already been computed and must not be hashed again. //! //! # Cross-language hashing @@ -46,21 +46,19 @@ //! //! # Application-defined storage //! -//! [`XorFilter::into_parts`] and [`XorFilter::from_parts`] exchange [`XorFilterParts`], which -//! preserve the construction seed, segment length, and typed fingerprint slots without adding -//! metadata to an existing disk format. Imported parts may omit the item count. Filters with a +//! [`XorFilter::from_parts`] imports a construction seed, segment length, and typed fingerprint +//! slots without adding metadata to an existing disk format. Accessors expose these fields for +//! serialization. Imports may omit the item count. Filters with a //! known zero item count always reject queries; imported parts with an unknown count retain the //! ordinary fingerprint comparison, including possible false positives. //! //! # Examples //! //! ``` -//! use datasketches::xor::XorFilterBuilder; +//! use datasketches::xor::XorFilter; //! use datasketches::xor::XorFilterType; //! -//! let filter = XorFilterBuilder::new(XorFilterType::Xor8) -//! .build(["apple", "banana"]) -//! .unwrap(); +//! let filter = XorFilter::from_values(["apple", "banana"], XorFilterType::Xor8, 0).unwrap(); //! //! assert!(filter.contains(&"apple")); //! ``` @@ -74,7 +72,5 @@ mod filter; mod serialization; pub use self::filter::XorFilter; -pub use self::filter::XorFilterBuilder; -pub use self::filter::XorFilterParts; pub use self::filter::XorFilterType; pub use self::filter::XorFingerprints; diff --git a/tests-integration/tests/serde_tests/xor.rs b/tests-integration/tests/serde_tests/xor.rs index 26ea69d3..cf8d48f5 100644 --- a/tests-integration/tests/serde_tests/xor.rs +++ b/tests-integration/tests/serde_tests/xor.rs @@ -17,8 +17,6 @@ use datasketches::error::ErrorKind; use datasketches::xor::XorFilter; -use datasketches::xor::XorFilterBuilder; -use datasketches::xor::XorFilterParts; use datasketches::xor::XorFilterType; use datasketches::xor::XorFingerprints; @@ -52,10 +50,7 @@ fn java_images_round_trip_byte_for_byte() { (XorFilterType::Xor16, JAVA_XOR16_IMAGE), ] { let expected = decode_hex(image); - let built = XorFilterBuilder::new(filter_type) - .seed(42) - .build(values) - .unwrap(); + let built = XorFilter::from_values(values, filter_type, 42).unwrap(); let restored = XorFilter::deserialize(&expected).unwrap(); assert_eq!(restored.serialize().unwrap(), expected); @@ -86,9 +81,7 @@ fn serialization_round_trips_standard_fingerprint_types() { XorFilterType::Xor32, ] { for count in [0, 1, 10_000_u64] { - let original = XorFilterBuilder::new(filter_type) - .build_from_hashes(0..count) - .unwrap(); + let original = XorFilter::from_hashes(0..count, filter_type, 0).unwrap(); let bytes = original.serialize().unwrap(); assert_eq!(bytes.len(), original.serialized_size().unwrap()); @@ -111,8 +104,7 @@ fn every_truncated_image_is_rejected() { XorFilterType::Xor16, XorFilterType::Xor32, ] { - let bytes = XorFilterBuilder::new(filter_type) - .build_from_hashes(0..100_u64) + let bytes = XorFilter::from_hashes(0..100_u64, filter_type, 0) .unwrap() .serialize() .unwrap(); @@ -126,8 +118,7 @@ fn every_truncated_image_is_rejected() { #[test] fn invalid_preamble_fields_are_rejected() { - let valid = XorFilterBuilder::new(XorFilterType::Xor8) - .build_from_hashes(0..100_u64) + let valid = XorFilter::from_hashes(0..100_u64, XorFilterType::Xor8, 0) .unwrap() .serialize() .unwrap(); @@ -151,8 +142,7 @@ fn invalid_preamble_fields_are_rejected() { #[test] fn unsafe_lengths_are_rejected_before_indexing() { - let valid = XorFilterBuilder::new(XorFilterType::Xor8) - .build_from_hashes(0..100_u64) + let valid = XorFilter::from_hashes(0..100_u64, XorFilterType::Xor8, 0) .unwrap() .serialize() .unwrap(); @@ -183,9 +173,7 @@ fn trailing_storage_is_ignored() { XorFilterType::Xor16, XorFilterType::Xor32, ] { - let original = XorFilterBuilder::new(kind) - .build_from_hashes(0..100_u64) - .unwrap(); + let original = XorFilter::from_hashes(0..100_u64, kind, 0).unwrap(); let mut bytes = original.serialize().unwrap(); bytes.extend_from_slice(&[0xaa; 16]); @@ -220,15 +208,17 @@ fn java_utf8_and_raw_byte_inputs_match() { ), ]; for (kind, image) in images { - let built = XorFilterBuilder::new(kind) - .seed(42) - .build([ + let built = XorFilter::from_values( + [ raw_bytes::from_slice(b"hello"), raw_bytes::from_slice("数据🔎".as_bytes()), raw_bytes::from_slice(&[]), raw_bytes::from_slice(&[0, 1, 255]), - ]) - .unwrap(); + ], + kind, + 42, + ) + .unwrap(); let expected = decode_hex(image); let restored = XorFilter::deserialize(&expected).unwrap(); assert_eq!(restored.serialize().unwrap(), expected); @@ -256,14 +246,12 @@ fn xor32_image_preserves_full_width_little_endian_slots() { "0100000001000000", "67452301efcdab89ffffffff", )); - let filter = XorFilter::from_parts(XorFilterParts { - seed: 0x0123_4567_89ab_cdef, - segment_length: 1, - fingerprints: XorFingerprints::Xor32( - vec![0x0123_4567, 0x89ab_cdef, u32::MAX].into_boxed_slice(), - ), - num_items: Some(1), - }) + let filter = XorFilter::from_parts( + 0x0123_4567_89ab_cdef, + 1, + XorFingerprints::Xor32(vec![0x0123_4567, 0x89ab_cdef, u32::MAX].into_boxed_slice()), + Some(1), + ) .unwrap(); assert_eq!(filter.serialized_size().unwrap(), bytes.len()); assert_eq!(filter.serialize().unwrap(), bytes); @@ -312,23 +300,21 @@ fn raw_parts_preserve_an_application_storage_layout() { } _ => unreachable!(), }; - let filter = XorFilter::from_parts(XorFilterParts::new(seed, segment, payload)).unwrap(); + let filter = XorFilter::from_parts(seed, segment, payload, None).unwrap(); for hash in [0, 1, 2, u64::MAX] { assert!(filter.contains_hash(hash)); } assert_eq!(filter.capacity(), slots); assert_eq!(filter.num_items(), None); - let parts = filter.into_parts(); - assert_eq!(parts.num_items, None); - let (seed, segment) = (parts.seed, parts.segment_length); - let payload = parts.fingerprints; + let (seed, segment) = (filter.seed(), filter.segment_length()); + let payload = filter.fingerprints(); let mut encoded = b"TEST".to_vec(); encoded.push(width); encoded.extend((segment as u32).to_le_bytes()); encoded.extend(seed.to_le_bytes()); match payload { - XorFingerprints::Xor8(values) => encoded.extend_from_slice(&values), + XorFingerprints::Xor8(values) => encoded.extend_from_slice(values), XorFingerprints::Xor16(values) => { encoded.extend(values.iter().flat_map(|v| v.to_le_bytes())) } diff --git a/tests-integration/tests/xor_test/filter.rs b/tests-integration/tests/xor_test/filter.rs index 96919e88..251d3d09 100644 --- a/tests-integration/tests/xor_test/filter.rs +++ b/tests-integration/tests/xor_test/filter.rs @@ -15,15 +15,12 @@ // specific language governing permissions and limitations // under the License. -use datasketches::xor::XorFilterBuilder; +use datasketches::xor::XorFilter; use datasketches::xor::XorFilterType; #[test] fn values_have_no_false_negatives() { - let filter = XorFilterBuilder::new(XorFilterType::Xor8) - .seed(8123) - .build(0..10_000_u64) - .unwrap(); + let filter = XorFilter::from_values(0..10_000_u64, XorFilterType::Xor8, 8123).unwrap(); for value in 0..10_000_u64 { assert!(filter.contains(&value), "false negative for {value}"); @@ -41,9 +38,7 @@ fn precomputed_hashes_have_no_false_negatives() { XorFilterType::Xor16, XorFilterType::Xor32, ] { - let filter = XorFilterBuilder::new(filter_type) - .build_from_hashes(hashes.iter().copied()) - .unwrap(); + let filter = XorFilter::from_hashes(hashes.iter().copied(), filter_type, 0).unwrap(); for &hash in &hashes { assert!(filter.contains_hash(hash), "false negative for {hash}"); } @@ -53,9 +48,8 @@ fn precomputed_hashes_have_no_false_negatives() { #[test] fn borrowed_values_can_be_reused() { let values = vec!["datasketches".to_string(), "xor filter".to_string()]; - let builder = XorFilterBuilder::new(XorFilterType::Xor16).seed(42); - let first = builder.clone().build(&values).unwrap(); - let second = builder.build(&values).unwrap(); + let first = XorFilter::from_values(&values, XorFilterType::Xor16, 42).unwrap(); + let second = XorFilter::from_values(&values, XorFilterType::Xor16, 42).unwrap(); assert_eq!(first, second); for value in &values { assert!(first.contains(value)); @@ -64,9 +58,7 @@ fn borrowed_values_can_be_reused() { #[test] fn duplicates_are_removed_before_construction() { - let filter = XorFilterBuilder::new(XorFilterType::Xor8) - .build_from_hashes([7, 7, 11, 11, 11]) - .unwrap(); + let filter = XorFilter::from_hashes([7, 7, 11, 11, 11], XorFilterType::Xor8, 0).unwrap(); assert_eq!(filter.num_items(), Some(2)); assert!(filter.contains_hash(7)); assert!(filter.contains_hash(11)); @@ -74,9 +66,7 @@ fn duplicates_are_removed_before_construction() { #[test] fn metadata_describes_the_compact_payload() { - let filter = XorFilterBuilder::new(XorFilterType::Xor8) - .build_from_hashes(0..10_000_u64) - .unwrap(); + let filter = XorFilter::from_hashes(0..10_000_u64, XorFilterType::Xor8, 0).unwrap(); assert_eq!(filter.is_empty(), Some(false)); assert_eq!(filter.num_items(), Some(10_000)); @@ -91,9 +81,7 @@ fn metadata_describes_the_compact_payload() { #[test] fn empty_filter_is_well_formed() { - let filter = XorFilterBuilder::new(XorFilterType::Xor16) - .build_from_hashes([]) - .unwrap(); + let filter = XorFilter::from_hashes([], XorFilterType::Xor16, 0).unwrap(); assert_eq!(filter.is_empty(), Some(true)); assert_eq!(filter.num_items(), Some(0)); @@ -107,15 +95,9 @@ fn false_positive_rates_follow_fingerprint_width() { const NUM_ITEMS: u64 = 50_000; const NUM_QUERIES: u64 = 100_000; - let xor8 = XorFilterBuilder::new(XorFilterType::Xor8) - .build_from_hashes(0..NUM_ITEMS) - .unwrap(); - let xor16 = XorFilterBuilder::new(XorFilterType::Xor16) - .build_from_hashes(0..NUM_ITEMS) - .unwrap(); - let xor32 = XorFilterBuilder::new(XorFilterType::Xor32) - .build_from_hashes(0..NUM_ITEMS) - .unwrap(); + let xor8 = XorFilter::from_hashes(0..NUM_ITEMS, XorFilterType::Xor8, 0).unwrap(); + let xor16 = XorFilter::from_hashes(0..NUM_ITEMS, XorFilterType::Xor16, 0).unwrap(); + let xor32 = XorFilter::from_hashes(0..NUM_ITEMS, XorFilterType::Xor32, 0).unwrap(); let false_positives8 = (NUM_ITEMS..NUM_ITEMS + NUM_QUERIES) .filter(|&hash| xor8.contains_hash(hash)) diff --git a/tests-integration/tests/xor_test/parts.rs b/tests-integration/tests/xor_test/parts.rs index 40c0a6c1..cb1121de 100644 --- a/tests-integration/tests/xor_test/parts.rs +++ b/tests-integration/tests/xor_test/parts.rs @@ -17,8 +17,6 @@ use datasketches::error::ErrorKind; use datasketches::xor::XorFilter; -use datasketches::xor::XorFilterBuilder; -use datasketches::xor::XorFilterParts; use datasketches::xor::XorFilterType; use datasketches::xor::XorFingerprints; use xorf::Filter; @@ -59,7 +57,7 @@ fn raw_parts_interoperate_with_xorf_in_both_directions() { ) } }; - let imported = XorFilter::from_parts(XorFilterParts::new(seed, segment, payload)).unwrap(); + let imported = XorFilter::from_parts(seed, segment, payload, None).unwrap(); assert_eq!(imported.num_items(), None); assert_eq!(imported.bits_per_item(), None); assert_eq!(imported.is_empty(), None); @@ -71,14 +69,11 @@ fn raw_parts_interoperate_with_xorf_in_both_directions() { assert!(imported.contains_hash(key)); } - let built = XorFilterBuilder::new(kind) - .build_from_distinct_hashes(&keys) - .unwrap(); - let parts = built.clone().into_parts(); - assert_eq!(parts.num_items, Some(keys.len())); - let (seed, block_length) = (parts.seed, parts.segment_length); - let payload = parts.fingerprints; - // Compare all answers, including false positives, after transferring ownership. + let built = XorFilter::from_distinct_hashes(&keys, kind, 0).unwrap(); + assert_eq!(built.num_items(), Some(keys.len())); + let (seed, block_length) = (built.seed(), built.segment_length()); + let payload = built.fingerprints().clone(); + // Compare all answers, including false positives, after exporting the fingerprints. let reference: Box> = match payload { XorFingerprints::Xor8(fingerprints) => Box::new(xorf::Xor8 { seed, @@ -111,14 +106,15 @@ fn raw_parts_reject_invalid_layouts_and_counts() { (usize::MAX, 3, None), (1, 3, Some(4)), ] { - let parts = XorFilterParts { - seed: 0, - segment_length: segment, - fingerprints: XorFingerprints::Xor8(vec![0; length].into()), - num_items: count, - }; assert_eq!( - XorFilter::from_parts(parts).unwrap_err().kind(), + XorFilter::from_parts( + 0, + segment, + XorFingerprints::Xor8(vec![0; length].into()), + count + ) + .unwrap_err() + .kind(), ErrorKind::InvalidArgument ); } @@ -131,15 +127,11 @@ fn distinct_construction_is_bounded_and_does_not_deduplicate() { XorFilterType::Xor16, XorFilterType::Xor32, ] { - let error = XorFilterBuilder::new(kind) - .build_from_distinct_hashes(&[1, 2, 1]) - .unwrap_err(); + let error = XorFilter::from_distinct_hashes(&[1, 2, 1], kind, 0).unwrap_err(); assert_eq!(error.kind(), ErrorKind::InvalidArgument); assert!(error.message().contains("duplicate hashes")); for keys in [vec![], vec![0], vec![u64::MAX], vec![0, u64::MAX]] { - let f = XorFilterBuilder::new(kind) - .build_from_distinct_hashes(&keys) - .unwrap(); + let f = XorFilter::from_distinct_hashes(&keys, kind, 0).unwrap(); assert_eq!(f.num_items(), Some(keys.len())); for key in keys { assert!(f.contains_hash(key)); @@ -160,9 +152,7 @@ fn construction_recovers_after_an_unpeelable_first_attempt() { z ^ (z >> 31) }) .collect(); - let first_seed = XorFilterBuilder::new(XorFilterType::Xor8) - .seed(1) - .build_from_distinct_hashes(&[]) + let first_seed = XorFilter::from_distinct_hashes(&[], XorFilterType::Xor8, 1) .unwrap() .seed(); let reference = xorf::Xor8::from(&keys); @@ -172,10 +162,7 @@ fn construction_recovers_after_an_unpeelable_first_attempt() { XorFilterType::Xor16, XorFilterType::Xor32, ] { - let filter = XorFilterBuilder::new(kind) - .seed(1) - .build_from_distinct_hashes(&keys) - .unwrap(); + let filter = XorFilter::from_distinct_hashes(&keys, kind, 1).unwrap(); assert_eq!(filter.seed(), reference.seed); for &key in &keys { assert!(filter.contains_hash(key)); @@ -184,10 +171,7 @@ fn construction_recovers_after_an_unpeelable_first_attempt() { reversed.reverse(); assert_eq!( filter, - XorFilterBuilder::new(kind) - .seed(1) - .build_from_distinct_hashes(&reversed) - .unwrap() + XorFilter::from_distinct_hashes(&reversed, kind, 1).unwrap() ); } } @@ -199,13 +183,17 @@ fn unknown_count_does_not_infer_emptiness_from_zero_fingerprints() { XorFilterType::Xor16, XorFilterType::Xor32, ] { - let empty = XorFilterBuilder::new(kind).build_from_hashes([]).unwrap(); + let empty = XorFilter::from_hashes([], kind, 0).unwrap(); let zero_hash = empty.seed().wrapping_neg(); assert_eq!(empty.is_empty(), Some(true)); assert!(!empty.contains_hash(zero_hash)); - let mut parts = empty.into_parts(); - parts.num_items = None; - let unknown = XorFilter::from_parts(parts).unwrap(); + let unknown = XorFilter::from_parts( + empty.seed(), + empty.segment_length(), + empty.fingerprints().clone(), + None, + ) + .unwrap(); assert_eq!(unknown.is_empty(), None); assert_eq!(unknown.num_items(), None); assert_eq!(unknown.bits_per_item(), None); @@ -219,10 +207,5 @@ fn unknown_count_does_not_infer_emptiness_from_zero_fingerprints() { unknown.serialize().unwrap_err().kind(), ErrorKind::InvalidArgument ); - let parts = unknown.into_parts(); - assert_eq!(parts.num_items, None); - let restored = XorFilter::from_parts(parts).unwrap(); - assert_eq!(restored.is_empty(), None); - assert!(restored.contains_hash(zero_hash)); } } From d09c8232cfd0d3b5f537da042f31c82a30991fed Mon Sep 17 00:00:00 2001 From: tison Date: Thu, 5 Nov 2026 11:57:39 +0800 Subject: [PATCH 3/6] refactor: shorten names for local values and parameters --- datasketches/src/cpc/sketch.rs | 14 +++++++------- datasketches/src/cpc/union.rs | 14 +++++++------- datasketches/src/frequencies/sketch.rs | 14 +++++--------- datasketches/src/hll/union.rs | 14 +++++++------- datasketches/src/kll/capacity.rs | 4 ++-- datasketches/src/tdigest/sketch.rs | 8 ++++---- datasketches/src/xor/filter.rs | 20 ++++++-------------- datasketches/src/xor/serialization.rs | 6 +++--- tests-integration/tests/serde_tests/xor.rs | 18 +++++++++--------- tests-integration/tests/xor_test/filter.rs | 4 ++-- 10 files changed, 52 insertions(+), 64 deletions(-) diff --git a/datasketches/src/cpc/sketch.rs b/datasketches/src/cpc/sketch.rs index 3222cff3..d4f688a2 100644 --- a/datasketches/src/cpc/sketch.rs +++ b/datasketches/src/cpc/sketch.rs @@ -368,18 +368,18 @@ impl CpcSketch { self.surprising_value_table_mut().clear(); // the new number of surprises will be about the same - let mask_for_clearing_window = (0xFF << new_offset) ^ u64::MAX; - let mask_for_flipping_early_zone = (1u64 << new_offset) - 1; + let clear_mask = (0xFF << new_offset) ^ u64::MAX; + let flip_mask = (1u64 << new_offset) - 1; - let mut all_surprises_ored = 0u64; + let mut surprises = 0u64; for i in 0..k { let mut pattern = bit_matrix[i]; self.sliding_window[i] = ((pattern >> new_offset) & 0xff) as u8; - pattern &= mask_for_clearing_window; + pattern &= clear_mask; // The following line converts surprising 0's to 1's in the "early zone", // (and vice versa, which is essential for this procedure's O(k) time cost). - pattern ^= mask_for_flipping_early_zone; - all_surprises_ored |= pattern; // a cheap way to recalculate first_interesting_column + pattern ^= flip_mask; + surprises |= pattern; // a cheap way to recalculate first_interesting_column while pattern != 0 { let col = pattern.trailing_zeros(); pattern ^= 1 << col; // erase the 1 @@ -390,7 +390,7 @@ impl CpcSketch { } self.window_offset = new_offset; - self.first_interesting_column = all_surprises_ored.trailing_zeros() as u8; + self.first_interesting_column = surprises.trailing_zeros() as u8; if self.first_interesting_column > new_offset { self.first_interesting_column = new_offset; // corner case } diff --git a/datasketches/src/cpc/union.rs b/datasketches/src/cpc/union.rs index abd473dd..dac4f3a1 100644 --- a/datasketches/src/cpc/union.rs +++ b/datasketches/src/cpc/union.rs @@ -171,18 +171,18 @@ impl CpcUnion { let mut table = PairTable::new(new_table_lg_size, 6 + lg_k); // the following should work even when the offset is zero - let mask_for_clearing_window = (0xFFu64 << offset) ^ u64::MAX; - let mask_for_flipping_early_zone = (1u64 << offset) - 1; - let mut all_surprises_ored = 0; + let clear_mask = (0xFFu64 << offset) ^ u64::MAX; + let flip_mask = (1u64 << offset) - 1; + let mut surprises = 0; // The snowplow effect was caused by processing the rows in order, // but we have fixed it by using a sufficiently large hash table. for i in 0..k { let mut pattern = matrix[i]; sliding_window[i] = ((pattern >> offset) & 0xFF) as u8; - pattern &= mask_for_clearing_window; - pattern ^= mask_for_flipping_early_zone; // this flipping converts surprising 0's to 1's - all_surprises_ored |= pattern; + pattern &= clear_mask; + pattern ^= flip_mask; // this flipping converts surprising 0's to 1's + surprises |= pattern; while pattern != 0 { let col = pattern.trailing_zeros(); pattern ^= 1u64 << col; // erase the 1 @@ -194,7 +194,7 @@ impl CpcUnion { // at this point we could shrink an oversized hash table, but the relative waste // isn't very big - sketch.first_interesting_column = all_surprises_ored.trailing_zeros() as u8; + sketch.first_interesting_column = surprises.trailing_zeros() as u8; if sketch.first_interesting_column > offset { sketch.first_interesting_column = offset; // corner case } diff --git a/datasketches/src/frequencies/sketch.rs b/datasketches/src/frequencies/sketch.rs index 617b98ec..a96c912e 100644 --- a/datasketches/src/frequencies/sketch.rs +++ b/datasketches/src/frequencies/sketch.rs @@ -485,7 +485,7 @@ impl FrequentItemsSketch { /// Returns frequent items using the sketch maximum error as threshold. /// - /// This is equivalent to `frequent_items_with_threshold(error_type, self.maximum_error())`. + /// This is equivalent to `frequent_items_with_threshold(ty, self.maximum_error())`. /// /// # Examples /// @@ -499,11 +499,11 @@ impl FrequentItemsSketch { /// let rows = sketch.frequent_items(ErrorType::NoFalseNegatives); /// assert!(rows.iter().any(|row| *row.item() == 1)); /// ``` - pub fn frequent_items(&self, error_type: ErrorType) -> Vec> + pub fn frequent_items(&self, ty: ErrorType) -> Vec> where T: Clone, { - self.frequent_items_with_threshold(error_type, self.offset) + self.frequent_items_with_threshold(ty, self.offset) } /// Returns frequent items using a custom threshold. @@ -525,11 +525,7 @@ impl FrequentItemsSketch { /// let rows = sketch.frequent_items_with_threshold(ErrorType::NoFalsePositives, 3); /// assert!(rows.iter().any(|row| *row.item() == 1)); /// ``` - pub fn frequent_items_with_threshold( - &self, - error_type: ErrorType, - threshold: u64, - ) -> Vec> + pub fn frequent_items_with_threshold(&self, ty: ErrorType, threshold: u64) -> Vec> where T: Clone, { @@ -538,7 +534,7 @@ impl FrequentItemsSketch { for (item, count) in self.hash_map.iter() { let lower = count; let upper = count + self.offset; - let include = match error_type { + let include = match ty { ErrorType::NoFalseNegatives => upper > threshold, ErrorType::NoFalsePositives => lower > threshold, }; diff --git a/datasketches/src/hll/union.rs b/datasketches/src/hll/union.rs index f942fc00..a9f00933 100644 --- a/datasketches/src/hll/union.rs +++ b/datasketches/src/hll/union.rs @@ -555,9 +555,9 @@ fn merge_array_with_downsample(dst: &mut Array8, dst_lg_k: u8, src_mode: &Mode, /// Changing representation preserves the estimate state; replaying registers alone would change /// HIP. -fn convert_array8_to_type(src: &Array8, lg_config_k: u8, target_type: HllType) -> HllSketch { - let estimate_state = src.estimate_state(); - match target_type { +fn convert_array8_to_type(src: &Array8, lg_config_k: u8, ty: HllType) -> HllSketch { + let state = src.estimate_state(); + match ty { HllType::Hll8 => HllSketch::from_mode(lg_config_k, Mode::Array8(src.clone())), HllType::Hll6 => { let mut array6 = Array6::new(lg_config_k); @@ -569,7 +569,7 @@ fn convert_array8_to_type(src: &Array8, lg_config_k: u8, target_type: HllType) - array6.update(coupon); } } - array6.restore_estimate_state(estimate_state); + array6.restore_estimate_state(state); HllSketch::from_mode(lg_config_k, Mode::Array6(array6)) } @@ -582,7 +582,7 @@ fn convert_array8_to_type(src: &Array8, lg_config_k: u8, target_type: HllType) - array4.update(coupon); } } - array4.restore_estimate_state(estimate_state); + array4.restore_estimate_state(state); HllSketch::from_mode(lg_config_k, Mode::Array4(array4)) } @@ -601,7 +601,7 @@ fn copy_array46_via_coupons(dst: &mut Array8, num_registers: usize, get_value: i /// Preserves the source estimate state because the result represents the same logical sketch. fn copy_or_downsample(src_mode: &Mode, src_lg_k: u8, tgt_lg_k: u8) -> Array8 { - let estimate_state = get_array_estimate_state(src_mode); + let state = get_array_estimate_state(src_mode); let mut result = if src_lg_k <= tgt_lg_k { match src_mode { @@ -628,6 +628,6 @@ fn copy_or_downsample(src_mode: &Mode, src_lg_k: u8, tgt_lg_k: u8) -> Array8 { result }; - result.restore_estimate_state(estimate_state); + result.restore_estimate_state(state); result } diff --git a/datasketches/src/kll/capacity.rs b/datasketches/src/kll/capacity.rs index e4ad7f1f..1f979cbf 100644 --- a/datasketches/src/kll/capacity.rs +++ b/datasketches/src/kll/capacity.rs @@ -103,8 +103,8 @@ const fn capacity_at_depth(k: u16, depth: usize) -> u16 { } let first_depth = depth / 2; let remaining_depth = depth - first_depth; - let intermediate_capacity = capacity_at_shallow_depth(k, first_depth); - capacity_at_shallow_depth(intermediate_capacity, remaining_depth) + let capacity = capacity_at_shallow_depth(k, first_depth); + capacity_at_shallow_depth(capacity, remaining_depth) } const fn capacity_at_shallow_depth(k: u16, depth: usize) -> u16 { diff --git a/datasketches/src/tdigest/sketch.rs b/datasketches/src/tdigest/sketch.rs index e665a103..e04811e3 100644 --- a/datasketches/src/tdigest/sketch.rs +++ b/datasketches/src/tdigest/sketch.rs @@ -1211,8 +1211,8 @@ impl TDigestBuffer { /// values stay before existing centroids when their means are equal. fn into_centroids_for_compression(mut self) -> Vec { debug_assert_ne!(self.unmerged_tail_len, 0); - let compressed_prefix_len = self.compressed_prefix_len(); - self.centroids.rotate_left(compressed_prefix_len); + let prefix_len = self.compressed_prefix_len(); + self.centroids.rotate_left(prefix_len); self.centroids } @@ -1227,7 +1227,7 @@ impl TDigestBuffer { return self.centroids; } - let compressed_prefix_len = self.compressed_prefix_len(); + let prefix_len = self.compressed_prefix_len(); self.centroids.reserve(other.len()); let other_prefix_len = other.compressed_prefix_len(); self.centroids @@ -1236,7 +1236,7 @@ impl TDigestBuffer { .extend_from_slice(&other.centroids[..other_prefix_len]); // Preserve the stable tie order: left unmerged, right unmerged and compressed, then the // left compressed prefix. - self.centroids.rotate_left(compressed_prefix_len); + self.centroids.rotate_left(prefix_len); self.centroids.sort_by(centroid_cmp); self.centroids } diff --git a/datasketches/src/xor/filter.rs b/datasketches/src/xor/filter.rs index e0dfff89..b393a268 100644 --- a/datasketches/src/xor/filter.rs +++ b/datasketches/src/xor/filter.rs @@ -135,14 +135,10 @@ impl XorFilter { /// ``` pub fn from_values( items: impl IntoIterator, - filter_type: XorFilterType, + ty: XorFilterType, seed: u64, ) -> Result { - Self::from_hashes( - items.into_iter().map(|item| hash_item(&item)), - filter_type, - seed, - ) + Self::from_hashes(items.into_iter().map(|item| hash_item(&item)), ty, seed) } /// Creates a filter from precomputed 64-bit hashes, removing duplicates. @@ -161,13 +157,13 @@ impl XorFilter { /// [`serialize`](Self::serialize). pub fn from_hashes( hashes: impl IntoIterator, - filter_type: XorFilterType, + ty: XorFilterType, seed: u64, ) -> Result { let mut hashes: Vec<_> = hashes.into_iter().collect(); hashes.sort_unstable(); hashes.dedup(); - Self::from_distinct_hashes(&hashes, filter_type, seed) + Self::from_distinct_hashes(&hashes, ty, seed) } /// Creates a filter from distinct precomputed hashes without copying or sorting them. @@ -183,11 +179,7 @@ impl XorFilter { /// described by [`from_hashes`](Self::from_hashes), or if no peelable construction is found /// within 100 attempts, even for valid input. Duplicate hashes prevent construction and /// return an error; they are not removed by this method. - pub fn from_distinct_hashes( - keys: &[u64], - filter_type: XorFilterType, - seed: u64, - ) -> Result { + pub fn from_distinct_hashes(keys: &[u64], ty: XorFilterType, seed: u64) -> Result { let num_items = keys.len(); let capacity = compute_capacity(num_items)?; let segment_length = capacity / usize::from(NUM_HASHES); @@ -244,7 +236,7 @@ impl XorFilter { drop(count); drop(queue); - let fingerprints = match filter_type { + let fingerprints = match ty { XorFilterType::Xor8 => XorFingerprints::Xor8(assign_fingerprints( capacity, segment_length, diff --git a/datasketches/src/xor/serialization.rs b/datasketches/src/xor/serialization.rs index 6cb3e1c4..413be847 100644 --- a/datasketches/src/xor/serialization.rs +++ b/datasketches/src/xor/serialization.rs @@ -114,7 +114,7 @@ impl XorFilter { Family::XORFILTER.min_pre_longs..=Family::XORFILTER.max_pre_longs, preamble_longs, )?; - let filter_type = match bits_per_fingerprint { + let ty = match bits_per_fingerprint { 8 => XorFilterType::Xor8, 16 => XorFilterType::Xor16, 32 => XorFilterType::Xor32, @@ -167,7 +167,7 @@ impl XorFilter { } let fingerprint_bytes = capacity - .checked_mul(filter_type.bytes_per_fingerprint()) + .checked_mul(ty.bytes_per_fingerprint()) .ok_or_else(|| Error::deserial("xor filter fingerprint size overflow"))?; if cursor.remaining().len() < fingerprint_bytes { return Err(Error::insufficient_data_of( @@ -179,7 +179,7 @@ impl XorFilter { )); } - let fingerprints = match filter_type { + let fingerprints = match ty { XorFilterType::Xor8 => { let mut values = vec![0_u8; capacity].into_boxed_slice(); cursor diff --git a/tests-integration/tests/serde_tests/xor.rs b/tests-integration/tests/serde_tests/xor.rs index cf8d48f5..31238b6c 100644 --- a/tests-integration/tests/serde_tests/xor.rs +++ b/tests-integration/tests/serde_tests/xor.rs @@ -45,12 +45,12 @@ const JAVA_XOR16_IMAGE: &str = concat!( fn java_images_round_trip_byte_for_byte() { let values = [1_u64, 2, 3, 1_u64 << 63]; - for (filter_type, image) in [ + for (ty, image) in [ (XorFilterType::Xor8, JAVA_XOR8_IMAGE), (XorFilterType::Xor16, JAVA_XOR16_IMAGE), ] { let expected = decode_hex(image); - let built = XorFilter::from_values(values, filter_type, 42).unwrap(); + let built = XorFilter::from_values(values, ty, 42).unwrap(); let restored = XorFilter::deserialize(&expected).unwrap(); assert_eq!(restored.serialize().unwrap(), expected); @@ -59,15 +59,15 @@ fn java_images_round_trip_byte_for_byte() { assert_eq!( built.serialize().unwrap()[..24], expected[..24], - "{filter_type:?} header differs from Java" + "{ty:?} header differs from Java" ); - assert_eq!(restored.filter_type(), filter_type); + assert_eq!(restored.filter_type(), ty); assert_eq!(restored.num_items(), Some(values.len())); for value in values { assert!(built.contains(&value)); assert!( restored.contains(&value), - "{filter_type:?} image did not contain {value}" + "{ty:?} image did not contain {value}" ); } } @@ -75,13 +75,13 @@ fn java_images_round_trip_byte_for_byte() { #[test] fn serialization_round_trips_standard_fingerprint_types() { - for filter_type in [ + for ty in [ XorFilterType::Xor8, XorFilterType::Xor16, XorFilterType::Xor32, ] { for count in [0, 1, 10_000_u64] { - let original = XorFilter::from_hashes(0..count, filter_type, 0).unwrap(); + let original = XorFilter::from_hashes(0..count, ty, 0).unwrap(); let bytes = original.serialize().unwrap(); assert_eq!(bytes.len(), original.serialized_size().unwrap()); @@ -99,12 +99,12 @@ fn serialization_round_trips_standard_fingerprint_types() { #[test] fn every_truncated_image_is_rejected() { - for filter_type in [ + for ty in [ XorFilterType::Xor8, XorFilterType::Xor16, XorFilterType::Xor32, ] { - let bytes = XorFilter::from_hashes(0..100_u64, filter_type, 0) + let bytes = XorFilter::from_hashes(0..100_u64, ty, 0) .unwrap() .serialize() .unwrap(); diff --git a/tests-integration/tests/xor_test/filter.rs b/tests-integration/tests/xor_test/filter.rs index 251d3d09..33b903bd 100644 --- a/tests-integration/tests/xor_test/filter.rs +++ b/tests-integration/tests/xor_test/filter.rs @@ -33,12 +33,12 @@ fn precomputed_hashes_have_no_false_negatives() { .map(|value| value.wrapping_mul(0x9e37_79b9_7f4a_7c15)) .collect::>(); - for filter_type in [ + for ty in [ XorFilterType::Xor8, XorFilterType::Xor16, XorFilterType::Xor32, ] { - let filter = XorFilter::from_hashes(hashes.iter().copied(), filter_type, 0).unwrap(); + let filter = XorFilter::from_hashes(hashes.iter().copied(), ty, 0).unwrap(); for &hash in &hashes { assert!(filter.contains_hash(hash), "false negative for {hash}"); } From e3b508e88374a231d0ac91837cf3b81470552e58 Mon Sep 17 00:00:00 2001 From: tison Date: Thu, 5 Nov 2026 12:07:37 +0800 Subject: [PATCH 4/6] refactor(xor): unify hash construction and encapsulate serialization --- CHANGELOG.md | 2 +- benchmarks/xor/build.rs | 36 +-- benchmarks/xor/build_batches.rs | 9 +- benchmarks/xor/query.rs | 18 +- datasketches/src/xor/filter.rs | 282 ++++++++++++++++++--- datasketches/src/xor/mod.rs | 3 +- datasketches/src/xor/serialization.rs | 243 ------------------ tests-integration/tests/serde_tests/xor.rs | 15 +- tests-integration/tests/xor_test/filter.rs | 20 +- tests-integration/tests/xor_test/parts.rs | 17 +- 10 files changed, 298 insertions(+), 347 deletions(-) delete mode 100644 datasketches/src/xor/serialization.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index c625b29a..8fc9c254 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ All significant changes to this project will be documented in this file. ### New features * Add `HllUnion::serialize` to serialize union results directly. -* Add immutable `XorFilter` behind the `xor` feature, with 8-, 16-, and 32-bit fingerprints, direct construction from values or precomputed hashes, borrowed distinct-hash construction, bounded seeded retries and compact temporary storage, optional cardinality metadata for imported filters, standard serialization for all three widths (Java-compatible for 8/16-bit filters), and validated imports for application-defined storage. +* Add immutable `XorFilter` behind the `xor` feature, with 8-, 16-, and 32-bit fingerprints, construction from values with automatic deduplication or borrowed distinct precomputed hashes, bounded seeded retries and compact temporary storage, optional cardinality metadata for imported filters, standard serialization for all three widths (Java-compatible for 8/16-bit filters), and validated imports for application-defined storage. ### Improvements diff --git a/benchmarks/xor/build.rs b/benchmarks/xor/build.rs index bc602dbc..f203a2fc 100644 --- a/benchmarks/xor/build.rs +++ b/benchmarks/xor/build.rs @@ -31,9 +31,8 @@ fn repeated_hashes(rows: usize) -> Vec { fn distinct_8(bencher: Bencher, cardinality: usize) { let keys = hashes(cardinality); - bencher.bench_local(|| { - black_box(XorFilter::from_distinct_hashes(&keys, XorFilterType::Xor8, 0).unwrap()) - }); + bencher + .bench_local(|| black_box(XorFilter::from_hashes(&keys, XorFilterType::Xor8, 0).unwrap())); } fn xorf_distinct_8(bencher: Bencher, cardinality: usize) { @@ -44,14 +43,17 @@ fn xorf_distinct_8(bencher: Bencher, cardinality: usize) { fn deduplicate_8(bencher: Bencher, rows: usize) { let keys = repeated_hashes(rows); bencher.bench_local(|| { - black_box(XorFilter::from_hashes(keys.iter().copied(), XorFilterType::Xor8, 0).unwrap()) + let mut distinct = keys.clone(); + distinct.sort_unstable(); + distinct.dedup(); + black_box(XorFilter::from_hashes(&distinct, XorFilterType::Xor8, 0).unwrap()) }); } fn xorf_deduplicate_8(bencher: Bencher, rows: usize) { let keys = repeated_hashes(rows); bencher.bench_local(|| { - // xorf requires distinct input, so include the caller's preprocessing. + // Include the caller's deduplication, as for DataSketches. let mut distinct = keys.clone(); distinct.sort_unstable(); distinct.dedup(); @@ -61,9 +63,8 @@ fn xorf_deduplicate_8(bencher: Bencher, rows: usize) { fn distinct_16(bencher: Bencher, cardinality: usize) { let keys = hashes(cardinality); - bencher.bench_local(|| { - black_box(XorFilter::from_distinct_hashes(&keys, XorFilterType::Xor16, 0).unwrap()) - }); + bencher + .bench_local(|| black_box(XorFilter::from_hashes(&keys, XorFilterType::Xor16, 0).unwrap())); } fn xorf_distinct_16(bencher: Bencher, cardinality: usize) { @@ -74,14 +75,17 @@ fn xorf_distinct_16(bencher: Bencher, cardinality: usize) { fn deduplicate_16(bencher: Bencher, rows: usize) { let keys = repeated_hashes(rows); bencher.bench_local(|| { - black_box(XorFilter::from_hashes(keys.iter().copied(), XorFilterType::Xor16, 0).unwrap()) + let mut distinct = keys.clone(); + distinct.sort_unstable(); + distinct.dedup(); + black_box(XorFilter::from_hashes(&distinct, XorFilterType::Xor16, 0).unwrap()) }); } fn xorf_deduplicate_16(bencher: Bencher, rows: usize) { let keys = repeated_hashes(rows); bencher.bench_local(|| { - // xorf requires distinct input, so include the caller's preprocessing. + // Include the caller's deduplication, as for DataSketches. let mut distinct = keys.clone(); distinct.sort_unstable(); distinct.dedup(); @@ -91,9 +95,8 @@ fn xorf_deduplicate_16(bencher: Bencher, rows: usize) { fn distinct_32(bencher: Bencher, cardinality: usize) { let keys = hashes(cardinality); - bencher.bench_local(|| { - black_box(XorFilter::from_distinct_hashes(&keys, XorFilterType::Xor32, 0).unwrap()) - }); + bencher + .bench_local(|| black_box(XorFilter::from_hashes(&keys, XorFilterType::Xor32, 0).unwrap())); } fn xorf_distinct_32(bencher: Bencher, cardinality: usize) { @@ -104,14 +107,17 @@ fn xorf_distinct_32(bencher: Bencher, cardinality: usize) { fn deduplicate_32(bencher: Bencher, rows: usize) { let keys = repeated_hashes(rows); bencher.bench_local(|| { - black_box(XorFilter::from_hashes(keys.iter().copied(), XorFilterType::Xor32, 0).unwrap()) + let mut distinct = keys.clone(); + distinct.sort_unstable(); + distinct.dedup(); + black_box(XorFilter::from_hashes(&distinct, XorFilterType::Xor32, 0).unwrap()) }); } fn xorf_deduplicate_32(bencher: Bencher, rows: usize) { let keys = repeated_hashes(rows); bencher.bench_local(|| { - // xorf requires distinct input, so include the caller's preprocessing. + // Include the caller's deduplication, as for DataSketches. let mut distinct = keys.clone(); distinct.sort_unstable(); distinct.dedup(); diff --git a/benchmarks/xor/build_batches.rs b/benchmarks/xor/build_batches.rs index a8cc46df..0d40bf00 100644 --- a/benchmarks/xor/build_batches.rs +++ b/benchmarks/xor/build_batches.rs @@ -26,7 +26,6 @@ use crate::xor::hashes_with_offset; fn build(bencher: Bencher, cardinality: usize, build: impl Fn(&[u64]) -> F) { // Multiple independent sets keep one set's retry count from dominating the comparison. - // The deduplicating constructor receives the same already-distinct inputs. let inputs: Vec<_> = (0..32) .map(|i| hashes_with_offset(cardinality, i * cardinality as u64)) .collect(); @@ -46,13 +45,7 @@ pub fn register(runner: &mut Runner) { &[ ("distinct", &|b| { build(b, cardinality, |keys| { - XorFilter::from_distinct_hashes(keys, XorFilterType::Xor8, 1).unwrap() - }) - }), - ("deduplicate", &|b| { - build(b, cardinality, |keys| { - XorFilter::from_hashes(keys.iter().copied(), XorFilterType::Xor8, 1) - .unwrap() + XorFilter::from_hashes(keys, XorFilterType::Xor8, 1).unwrap() }) }), ("xorf", &|b| { diff --git a/benchmarks/xor/query.rs b/benchmarks/xor/query.rs index 008465f0..36542b67 100644 --- a/benchmarks/xor/query.rs +++ b/benchmarks/xor/query.rs @@ -27,8 +27,7 @@ use crate::xor::hashes; fn datasketches_8_present(bencher: Bencher, cardinality: usize) { let keys = hashes(cardinality * 2); - let filter = - XorFilter::from_distinct_hashes(&keys[..cardinality], XorFilterType::Xor8, 0).unwrap(); + let filter = XorFilter::from_hashes(&keys[..cardinality], XorFilterType::Xor8, 0).unwrap(); bencher.bench_local(|| { for key in &keys[..cardinality] { black_box(filter.contains_hash(black_box(*key))); @@ -38,8 +37,7 @@ fn datasketches_8_present(bencher: Bencher, cardinality: usize) { fn datasketches_8_absent(bencher: Bencher, cardinality: usize) { let keys = hashes(cardinality * 2); - let filter = - XorFilter::from_distinct_hashes(&keys[..cardinality], XorFilterType::Xor8, 0).unwrap(); + let filter = XorFilter::from_hashes(&keys[..cardinality], XorFilterType::Xor8, 0).unwrap(); bencher.bench_local(|| { for key in &keys[cardinality..] { black_box(filter.contains_hash(black_box(*key))); @@ -69,8 +67,7 @@ fn xorf_8_absent(bencher: Bencher, cardinality: usize) { fn datasketches_16_present(bencher: Bencher, cardinality: usize) { let keys = hashes(cardinality * 2); - let filter = - XorFilter::from_distinct_hashes(&keys[..cardinality], XorFilterType::Xor16, 0).unwrap(); + let filter = XorFilter::from_hashes(&keys[..cardinality], XorFilterType::Xor16, 0).unwrap(); bencher.bench_local(|| { for key in &keys[..cardinality] { black_box(filter.contains_hash(black_box(*key))); @@ -80,8 +77,7 @@ fn datasketches_16_present(bencher: Bencher, cardinality: usize) { fn datasketches_16_absent(bencher: Bencher, cardinality: usize) { let keys = hashes(cardinality * 2); - let filter = - XorFilter::from_distinct_hashes(&keys[..cardinality], XorFilterType::Xor16, 0).unwrap(); + let filter = XorFilter::from_hashes(&keys[..cardinality], XorFilterType::Xor16, 0).unwrap(); bencher.bench_local(|| { for key in &keys[cardinality..] { black_box(filter.contains_hash(black_box(*key))); @@ -111,8 +107,7 @@ fn xorf_16_absent(bencher: Bencher, cardinality: usize) { fn datasketches_32_present(bencher: Bencher, cardinality: usize) { let keys = hashes(cardinality * 2); - let filter = - XorFilter::from_distinct_hashes(&keys[..cardinality], XorFilterType::Xor32, 0).unwrap(); + let filter = XorFilter::from_hashes(&keys[..cardinality], XorFilterType::Xor32, 0).unwrap(); bencher.bench_local(|| { for key in &keys[..cardinality] { black_box(filter.contains_hash(black_box(*key))); @@ -122,8 +117,7 @@ fn datasketches_32_present(bencher: Bencher, cardinality: usize) { fn datasketches_32_absent(bencher: Bencher, cardinality: usize) { let keys = hashes(cardinality * 2); - let filter = - XorFilter::from_distinct_hashes(&keys[..cardinality], XorFilterType::Xor32, 0).unwrap(); + let filter = XorFilter::from_hashes(&keys[..cardinality], XorFilterType::Xor32, 0).unwrap(); bencher.bench_local(|| { for key in &keys[cardinality..] { black_box(filter.contains_hash(black_box(*key))); diff --git a/datasketches/src/xor/filter.rs b/datasketches/src/xor/filter.rs index b393a268..39401ab8 100644 --- a/datasketches/src/xor/filter.rs +++ b/datasketches/src/xor/filter.rs @@ -19,10 +19,18 @@ use std::hash::Hash; use std::hash::Hasher; use std::ops::BitXor; +use crate::codec::SketchBytes; +use crate::codec::SketchSlice; +use crate::codec::assert::ensure_preamble_longs_in_range; +use crate::codec::assert::ensure_serial_version_is; +use crate::codec::assert::insufficient_data; +use crate::codec::family::Family; use crate::error::Error; use crate::hash::XxHash64; const NUM_HASHES: u8 = 3; +const SERIAL_VERSION: u8 = 1; +const PREAMBLE_BYTES: usize = 3 * size_of::(); const HASH_SEED: u64 = 0; const LOAD_FACTOR: f64 = 1.23; const CAPACITY_OFFSET: u64 = 32; @@ -57,7 +65,7 @@ impl XorFilterType { } } - pub(crate) const fn bytes_per_fingerprint(self) -> usize { + const fn bytes_per_fingerprint(self) -> usize { (self.bits_per_fingerprint() / 8) as usize } } @@ -84,7 +92,7 @@ impl XorFingerprints { } } - pub(crate) fn byte_len(&self) -> usize { + fn byte_len(&self) -> usize { match self { Self::Xor8(values) => size_of_val::<[u8]>(values), Self::Xor16(values) => size_of_val::<[u16]>(values), @@ -102,14 +110,13 @@ impl XorFingerprints { /// /// Ordinary values passed to [`contains`](Self::contains) are reduced to 64-bit hashes with /// xxHash64 and seed `0`. Use [`contains_hash`](Self::contains_hash) only with the same hashes that -/// were supplied to [`from_hashes`](Self::from_hashes) or -/// [`from_distinct_hashes`](Self::from_distinct_hashes). +/// were supplied to [`from_hashes`](Self::from_hashes). #[derive(Debug, Clone, PartialEq, Eq)] pub struct XorFilter { - pub(crate) segment_length: usize, - pub(crate) num_items: Option, - pub(crate) seed: u64, - pub(crate) fingerprints: XorFingerprints, + segment_length: usize, + num_items: Option, + seed: u64, + fingerprints: XorFingerprints, } impl XorFilter { @@ -121,7 +128,8 @@ impl XorFilter { /// /// # Errors /// - /// Returns an error under the same conditions as [`from_hashes`](Self::from_hashes). + /// Returns an error under the size and construction-attempt limits of + /// [`from_hashes`](Self::from_hashes). Duplicate values are removed before construction. /// /// # Examples /// @@ -138,48 +146,28 @@ impl XorFilter { ty: XorFilterType, seed: u64, ) -> Result { - Self::from_hashes(items.into_iter().map(|item| hash_item(&item)), ty, seed) - } - - /// Creates a filter from precomputed 64-bit hashes, removing duplicates. - /// - /// This method bypasses xxHash64. Query the resulting filter with - /// [`contains_hash`](Self::contains_hash) using hashes from the same source. - /// The construction `seed` selects a deterministic sequence of attempts. - /// Pass `hashes.iter().copied()` to retain the original input after construction. - /// - /// # Errors - /// - /// Returns [`InvalidArgument`](crate::error::ErrorKind::InvalidArgument) if the fingerprint - /// slot count exceeds `u32::MAX` or a scratch allocation would exceed `isize::MAX` bytes, - /// or if no peelable construction is found within 100 attempts. Valid input can also - /// exhaust the construction attempts. A successful build may still exceed the limits of - /// [`serialize`](Self::serialize). - pub fn from_hashes( - hashes: impl IntoIterator, - ty: XorFilterType, - seed: u64, - ) -> Result { - let mut hashes: Vec<_> = hashes.into_iter().collect(); + let mut hashes: Vec<_> = items.into_iter().map(|item| hash_item(&item)).collect(); hashes.sort_unstable(); hashes.dedup(); - Self::from_distinct_hashes(&hashes, ty, seed) + Self::from_hashes(&hashes, ty, seed) } - /// Creates a filter from distinct precomputed hashes without copying or sorting them. + /// Creates a filter from distinct precomputed 64-bit hashes without copying or sorting them. /// - /// The caller must supply distinct hashes. Use [`from_hashes`](Self::from_hashes) when - /// duplicates may be present. This method borrows the input without modifying it. + /// The caller must supply distinct hashes. Duplicate hashes return an error; this method + /// does not remove them. The input is borrowed without modification. + /// Query with [`contains_hash`](Self::contains_hash) using hashes from the same source. /// The construction `seed` selects a deterministic sequence of attempts; input order /// does not affect the result. /// /// # Errors /// - /// Returns [`InvalidArgument`](crate::error::ErrorKind::InvalidArgument) under the size limits - /// described by [`from_hashes`](Self::from_hashes), or if no peelable construction is found - /// within 100 attempts, even for valid input. Duplicate hashes prevent construction and - /// return an error; they are not removed by this method. - pub fn from_distinct_hashes(keys: &[u64], ty: XorFilterType, seed: u64) -> Result { + /// Returns [`InvalidArgument`](crate::error::ErrorKind::InvalidArgument) if hashes contain + /// duplicates, the fingerprint slot count exceeds `u32::MAX`, a scratch allocation would + /// exceed `isize::MAX` bytes, or no peelable construction is found within 100 attempts. + /// Valid input can also exhaust the attempts. A successful build may still exceed the + /// limits of [`serialize`](Self::serialize). + pub fn from_hashes(keys: &[u64], ty: XorFilterType, seed: u64) -> Result { let num_items = keys.len(); let capacity = compute_capacity(num_items)?; let segment_length = capacity / usize::from(NUM_HASHES); @@ -288,7 +276,7 @@ impl XorFilter { /// use datasketches::xor::XorFilter; /// use datasketches::xor::XorFilterType; /// - /// let filter = XorFilter::from_hashes([10, 20, 30], XorFilterType::Xor8, 0)?; + /// let filter = XorFilter::from_hashes(&[10, 20, 30], XorFilterType::Xor8, 0)?; /// let imported = XorFilter::from_parts( /// filter.seed(), /// filter.segment_length(), @@ -431,6 +419,216 @@ impl XorFilter { pub fn estimated_size(&self) -> usize { size_of::() + self.fingerprints.byte_len() } + + /// Serializes the filter using DataSketches family `22`, version `1`. + /// + /// Supports 8-, 16-, and 32-bit fingerprints, recorded in the header's fingerprint width + /// field. Reserved fields are written as zero; integers and fingerprints are little-endian. + /// The 8- and 16-bit images are compatible with Java when input hashing agrees. Readers + /// must support the encoded fingerprint width; implementations limited to 8/16 bits cannot + /// read 32-bit images. + /// + /// # Errors + /// + /// Returns [`InvalidArgument`](crate::error::ErrorKind::InvalidArgument) for an unknown + /// item count, or an item count or segment length above `i32::MAX`. + /// The total number of slots must also fit `i32::MAX` so that Java can index all three + /// segments; this is an additional interoperability limit, not a header field. Construction + /// and raw parts allow larger capacities, so successful construction does not guarantee + /// standard serialization. Use raw parts for other storage formats. + pub fn serialize(&self) -> Result, Error> { + let size = self.serialized_size()?; + let num_items = self + .num_items + .expect("serialized_size checked the item count"); + let mut bytes = SketchBytes::with_capacity(size); + + bytes.write_u8(Family::XORFILTER.min_pre_longs); + bytes.write_u8(SERIAL_VERSION); + bytes.write_u8(Family::XORFILTER.id); + bytes.write_u8(0); // flags + bytes.write_u8(self.bits_per_fingerprint()); + bytes.write_u8(NUM_HASHES); + bytes.write_u16_le(0); // unused + bytes.write_u64_le(self.seed); + bytes.write_i32_le(self.segment_length as i32); + bytes.write_i32_le(num_items as i32); + + match &self.fingerprints { + XorFingerprints::Xor8(fingerprints) => bytes.write(fingerprints), + XorFingerprints::Xor16(fingerprints) => { + for &fingerprint in fingerprints.iter() { + bytes.write_u16_le(fingerprint); + } + } + XorFingerprints::Xor32(fingerprints) => { + for &fingerprint in fingerprints.iter() { + bytes.write_u32_le(fingerprint); + } + } + } + + Ok(bytes.into_bytes()) + } + + /// Deserializes an owned filter from bytes. + /// + /// Accepts 8-, 16-, and 32-bit images within the limits of [`serialize`](Self::serialize). + /// Reserved fields and trailing bytes are ignored. + /// + /// # Errors + /// + /// Returns an error if the image is truncated, belongs to another sketch family or version, or + /// contains metadata that could make a membership query index outside the fingerprint payload. + pub fn deserialize(bytes: &[u8]) -> Result { + let mut cursor = SketchSlice::new(bytes); + + let preamble_longs = cursor + .read_u8() + .map_err(insufficient_data("preamble_longs"))?; + let serial_version = cursor + .read_u8() + .map_err(insufficient_data("serial_version"))?; + let family_id = cursor.read_u8().map_err(insufficient_data("family_id"))?; + let _flags = cursor.read_u8().map_err(insufficient_data("flags"))?; + let bits_per_fingerprint = cursor + .read_u8() + .map_err(insufficient_data("bits_per_fingerprint"))?; + let num_hashes = cursor.read_u8().map_err(insufficient_data("num_hashes"))?; + let _unused = cursor.read_u16_le().map_err(insufficient_data("unused"))?; + + Family::XORFILTER.validate_id(family_id)?; + ensure_serial_version_is(SERIAL_VERSION, serial_version)?; + ensure_preamble_longs_in_range( + Family::XORFILTER.min_pre_longs..=Family::XORFILTER.max_pre_longs, + preamble_longs, + )?; + let ty = match bits_per_fingerprint { + 8 => XorFilterType::Xor8, + 16 => XorFilterType::Xor16, + 32 => XorFilterType::Xor32, + _ => { + return Err(Error::deserial( + "standard xor format requires 8-, 16-, or 32-bit fingerprints", + )); + } + }; + if num_hashes != NUM_HASHES { + return Err(Error::deserial(format!( + "invalid number of hashes: expected {NUM_HASHES}, got {num_hashes}" + ))); + } + + let seed = cursor.read_u64_le().map_err(insufficient_data("seed"))?; + let segment_length = cursor + .read_i32_le() + .map_err(insufficient_data("segment_length"))?; + if segment_length <= 0 { + return Err(Error::deserial(format!( + "invalid segment length: expected a positive value, got {segment_length}" + ))); + } + let segment_length = segment_length as usize; + + let num_items = cursor + .read_i32_le() + .map_err(insufficient_data("num_items"))?; + if num_items < 0 { + return Err(Error::deserial(format!( + "invalid item count: expected a non-negative value, got {num_items}" + ))); + } + let num_items = num_items as usize; + + let capacity = segment_length + .checked_mul(usize::from(NUM_HASHES)) + .ok_or_else(|| Error::deserial("xor filter capacity overflow"))?; + if capacity > i32::MAX as usize { + return Err(Error::deserial(format!( + "invalid xor filter capacity: maximum is {}, got {capacity}", + i32::MAX + ))); + } + if num_items > capacity { + return Err(Error::deserial(format!( + "invalid item count: capacity is {capacity}, got {num_items}" + ))); + } + + let fingerprint_bytes = capacity + .checked_mul(ty.bytes_per_fingerprint()) + .ok_or_else(|| Error::deserial("xor filter fingerprint size overflow"))?; + if cursor.remaining().len() < fingerprint_bytes { + return Err(Error::insufficient_data_of( + "fingerprints", + format!( + "expected {fingerprint_bytes} bytes, got {}", + cursor.remaining().len() + ), + )); + } + + let fingerprints = match ty { + XorFilterType::Xor8 => { + let mut values = vec![0_u8; capacity].into_boxed_slice(); + cursor + .read_exact(&mut values) + .map_err(insufficient_data("fingerprints"))?; + XorFingerprints::Xor8(values) + } + XorFilterType::Xor16 => { + let mut values = Vec::with_capacity(capacity); + for _ in 0..capacity { + values.push( + cursor + .read_u16_le() + .map_err(insufficient_data("fingerprints"))?, + ); + } + XorFingerprints::Xor16(values.into_boxed_slice()) + } + XorFilterType::Xor32 => { + let mut values = Vec::with_capacity(capacity); + for _ in 0..capacity { + values.push( + cursor + .read_u32_le() + .map_err(insufficient_data("fingerprints"))?, + ); + } + XorFingerprints::Xor32(values.into_boxed_slice()) + } + }; + + Ok(Self { + segment_length, + num_items: Some(num_items), + seed, + fingerprints, + }) + } + + /// Returns the standard serialized size in bytes. + /// + /// # Errors + /// + /// Returns an error under the same conditions as [`serialize`](Self::serialize). + pub fn serialized_size(&self) -> Result { + let count = self + .num_items + .ok_or_else(|| Error::invalid_argument("standard xor format requires an item count"))?; + if count > i32::MAX as usize + || self.segment_length > i32::MAX as usize + || self.capacity() > i32::MAX as usize + { + return Err(Error::invalid_argument( + "xor filter exceeds standard format limits", + )); + } + PREAMBLE_BYTES + .checked_add(self.fingerprints.byte_len()) + .ok_or_else(|| Error::invalid_argument("xor filter serialized size overflow")) + } } fn hash_item(item: &T) -> u64 { diff --git a/datasketches/src/xor/mod.rs b/datasketches/src/xor/mod.rs index b126ae56..104edc18 100644 --- a/datasketches/src/xor/mod.rs +++ b/datasketches/src/xor/mod.rs @@ -23,7 +23,7 @@ //! in the number of distinct hashes. //! //! [`XorFilter::from_values`] hashes ordinary Rust values with xxHash64. Call -//! [`XorFilter::from_hashes`] or [`XorFilter::from_distinct_hashes`] +//! [`XorFilter::from_hashes`] with distinct hashes //! when hashes have already been computed and must not be hashed again. //! //! # Cross-language hashing @@ -69,7 +69,6 @@ //! of Experimental Algorithmics 25 (2020). mod filter; -mod serialization; pub use self::filter::XorFilter; pub use self::filter::XorFilterType; diff --git a/datasketches/src/xor/serialization.rs b/datasketches/src/xor/serialization.rs deleted file mode 100644 index 413be847..00000000 --- a/datasketches/src/xor/serialization.rs +++ /dev/null @@ -1,243 +0,0 @@ -// 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::codec::SketchBytes; -use crate::codec::SketchSlice; -use crate::codec::assert::ensure_preamble_longs_in_range; -use crate::codec::assert::ensure_serial_version_is; -use crate::codec::assert::insufficient_data; -use crate::codec::family::Family; -use crate::error::Error; -use crate::xor::filter::XorFilter; -use crate::xor::filter::XorFilterType; -use crate::xor::filter::XorFingerprints; - -const SERIAL_VERSION: u8 = 1; -const NUM_HASHES: u8 = 3; -const PREAMBLE_BYTES: usize = 3 * size_of::(); - -impl XorFilter { - /// Serializes the filter using DataSketches family `22`, version `1`. - /// - /// Supports 8-, 16-, and 32-bit fingerprints, recorded in the header's fingerprint width - /// field. Reserved fields are written as zero; integers and fingerprints are little-endian. - /// The 8- and 16-bit images are compatible with Java when input hashing agrees. Readers - /// must support the encoded fingerprint width; implementations limited to 8/16 bits cannot - /// read 32-bit images. - /// - /// # Errors - /// - /// Returns [`InvalidArgument`](crate::error::ErrorKind::InvalidArgument) for an unknown - /// item count, or an item count or segment length above `i32::MAX`. - /// The total number of slots must also fit `i32::MAX` so that Java can index all three - /// segments; this is an additional interoperability limit, not a header field. Construction - /// and raw parts allow larger capacities, so successful construction does not guarantee - /// standard serialization. Use raw parts for other storage formats. - pub fn serialize(&self) -> Result, Error> { - let size = self.serialized_size()?; - let num_items = self - .num_items - .expect("serialized_size checked the item count"); - let mut bytes = SketchBytes::with_capacity(size); - - bytes.write_u8(Family::XORFILTER.min_pre_longs); - bytes.write_u8(SERIAL_VERSION); - bytes.write_u8(Family::XORFILTER.id); - bytes.write_u8(0); // flags - bytes.write_u8(self.bits_per_fingerprint()); - bytes.write_u8(NUM_HASHES); - bytes.write_u16_le(0); // unused - bytes.write_u64_le(self.seed); - bytes.write_i32_le(self.segment_length as i32); - bytes.write_i32_le(num_items as i32); - - match &self.fingerprints { - XorFingerprints::Xor8(fingerprints) => bytes.write(fingerprints), - XorFingerprints::Xor16(fingerprints) => { - for &fingerprint in fingerprints.iter() { - bytes.write_u16_le(fingerprint); - } - } - XorFingerprints::Xor32(fingerprints) => { - for &fingerprint in fingerprints.iter() { - bytes.write_u32_le(fingerprint); - } - } - } - - Ok(bytes.into_bytes()) - } - - /// Deserializes an owned filter from bytes. - /// - /// Accepts 8-, 16-, and 32-bit images within the limits of [`serialize`](Self::serialize). - /// Reserved fields and trailing bytes are ignored. - /// - /// # Errors - /// - /// Returns an error if the image is truncated, belongs to another sketch family or version, or - /// contains metadata that could make a membership query index outside the fingerprint payload. - pub fn deserialize(bytes: &[u8]) -> Result { - let mut cursor = SketchSlice::new(bytes); - - let preamble_longs = cursor - .read_u8() - .map_err(insufficient_data("preamble_longs"))?; - let serial_version = cursor - .read_u8() - .map_err(insufficient_data("serial_version"))?; - let family_id = cursor.read_u8().map_err(insufficient_data("family_id"))?; - let _flags = cursor.read_u8().map_err(insufficient_data("flags"))?; - let bits_per_fingerprint = cursor - .read_u8() - .map_err(insufficient_data("bits_per_fingerprint"))?; - let num_hashes = cursor.read_u8().map_err(insufficient_data("num_hashes"))?; - let _unused = cursor.read_u16_le().map_err(insufficient_data("unused"))?; - - Family::XORFILTER.validate_id(family_id)?; - ensure_serial_version_is(SERIAL_VERSION, serial_version)?; - ensure_preamble_longs_in_range( - Family::XORFILTER.min_pre_longs..=Family::XORFILTER.max_pre_longs, - preamble_longs, - )?; - let ty = match bits_per_fingerprint { - 8 => XorFilterType::Xor8, - 16 => XorFilterType::Xor16, - 32 => XorFilterType::Xor32, - _ => { - return Err(Error::deserial( - "standard xor format requires 8-, 16-, or 32-bit fingerprints", - )); - } - }; - if num_hashes != NUM_HASHES { - return Err(Error::deserial(format!( - "invalid number of hashes: expected {NUM_HASHES}, got {num_hashes}" - ))); - } - - let seed = cursor.read_u64_le().map_err(insufficient_data("seed"))?; - let segment_length = cursor - .read_i32_le() - .map_err(insufficient_data("segment_length"))?; - if segment_length <= 0 { - return Err(Error::deserial(format!( - "invalid segment length: expected a positive value, got {segment_length}" - ))); - } - let segment_length = segment_length as usize; - - let num_items = cursor - .read_i32_le() - .map_err(insufficient_data("num_items"))?; - if num_items < 0 { - return Err(Error::deserial(format!( - "invalid item count: expected a non-negative value, got {num_items}" - ))); - } - let num_items = num_items as usize; - - let capacity = segment_length - .checked_mul(usize::from(NUM_HASHES)) - .ok_or_else(|| Error::deserial("xor filter capacity overflow"))?; - if capacity > i32::MAX as usize { - return Err(Error::deserial(format!( - "invalid xor filter capacity: maximum is {}, got {capacity}", - i32::MAX - ))); - } - if num_items > capacity { - return Err(Error::deserial(format!( - "invalid item count: capacity is {capacity}, got {num_items}" - ))); - } - - let fingerprint_bytes = capacity - .checked_mul(ty.bytes_per_fingerprint()) - .ok_or_else(|| Error::deserial("xor filter fingerprint size overflow"))?; - if cursor.remaining().len() < fingerprint_bytes { - return Err(Error::insufficient_data_of( - "fingerprints", - format!( - "expected {fingerprint_bytes} bytes, got {}", - cursor.remaining().len() - ), - )); - } - - let fingerprints = match ty { - XorFilterType::Xor8 => { - let mut values = vec![0_u8; capacity].into_boxed_slice(); - cursor - .read_exact(&mut values) - .map_err(insufficient_data("fingerprints"))?; - XorFingerprints::Xor8(values) - } - XorFilterType::Xor16 => { - let mut values = Vec::with_capacity(capacity); - for _ in 0..capacity { - values.push( - cursor - .read_u16_le() - .map_err(insufficient_data("fingerprints"))?, - ); - } - XorFingerprints::Xor16(values.into_boxed_slice()) - } - XorFilterType::Xor32 => { - let mut values = Vec::with_capacity(capacity); - for _ in 0..capacity { - values.push( - cursor - .read_u32_le() - .map_err(insufficient_data("fingerprints"))?, - ); - } - XorFingerprints::Xor32(values.into_boxed_slice()) - } - }; - - Ok(Self { - segment_length, - num_items: Some(num_items), - seed, - fingerprints, - }) - } - - /// Returns the standard serialized size in bytes. - /// - /// # Errors - /// - /// Returns an error under the same conditions as [`serialize`](Self::serialize). - pub fn serialized_size(&self) -> Result { - let count = self - .num_items - .ok_or_else(|| Error::invalid_argument("standard xor format requires an item count"))?; - if count > i32::MAX as usize - || self.segment_length > i32::MAX as usize - || self.capacity() > i32::MAX as usize - { - return Err(Error::invalid_argument( - "xor filter exceeds standard format limits", - )); - } - PREAMBLE_BYTES - .checked_add(self.fingerprints.byte_len()) - .ok_or_else(|| Error::invalid_argument("xor filter serialized size overflow")) - } -} diff --git a/tests-integration/tests/serde_tests/xor.rs b/tests-integration/tests/serde_tests/xor.rs index 31238b6c..94f2ac5b 100644 --- a/tests-integration/tests/serde_tests/xor.rs +++ b/tests-integration/tests/serde_tests/xor.rs @@ -81,7 +81,8 @@ fn serialization_round_trips_standard_fingerprint_types() { XorFilterType::Xor32, ] { for count in [0, 1, 10_000_u64] { - let original = XorFilter::from_hashes(0..count, ty, 0).unwrap(); + let hashes: Vec<_> = (0..count).collect(); + let original = XorFilter::from_hashes(&hashes, ty, 0).unwrap(); let bytes = original.serialize().unwrap(); assert_eq!(bytes.len(), original.serialized_size().unwrap()); @@ -104,7 +105,8 @@ fn every_truncated_image_is_rejected() { XorFilterType::Xor16, XorFilterType::Xor32, ] { - let bytes = XorFilter::from_hashes(0..100_u64, ty, 0) + let hashes: Vec<_> = (0..100_u64).collect(); + let bytes = XorFilter::from_hashes(&hashes, ty, 0) .unwrap() .serialize() .unwrap(); @@ -118,7 +120,8 @@ fn every_truncated_image_is_rejected() { #[test] fn invalid_preamble_fields_are_rejected() { - let valid = XorFilter::from_hashes(0..100_u64, XorFilterType::Xor8, 0) + let hashes: Vec<_> = (0..100_u64).collect(); + let valid = XorFilter::from_hashes(&hashes, XorFilterType::Xor8, 0) .unwrap() .serialize() .unwrap(); @@ -142,7 +145,8 @@ fn invalid_preamble_fields_are_rejected() { #[test] fn unsafe_lengths_are_rejected_before_indexing() { - let valid = XorFilter::from_hashes(0..100_u64, XorFilterType::Xor8, 0) + let hashes: Vec<_> = (0..100_u64).collect(); + let valid = XorFilter::from_hashes(&hashes, XorFilterType::Xor8, 0) .unwrap() .serialize() .unwrap(); @@ -173,7 +177,8 @@ fn trailing_storage_is_ignored() { XorFilterType::Xor16, XorFilterType::Xor32, ] { - let original = XorFilter::from_hashes(0..100_u64, kind, 0).unwrap(); + let hashes: Vec<_> = (0..100_u64).collect(); + let original = XorFilter::from_hashes(&hashes, kind, 0).unwrap(); let mut bytes = original.serialize().unwrap(); bytes.extend_from_slice(&[0xaa; 16]); diff --git a/tests-integration/tests/xor_test/filter.rs b/tests-integration/tests/xor_test/filter.rs index 33b903bd..c7b32610 100644 --- a/tests-integration/tests/xor_test/filter.rs +++ b/tests-integration/tests/xor_test/filter.rs @@ -38,7 +38,7 @@ fn precomputed_hashes_have_no_false_negatives() { XorFilterType::Xor16, XorFilterType::Xor32, ] { - let filter = XorFilter::from_hashes(hashes.iter().copied(), ty, 0).unwrap(); + let filter = XorFilter::from_hashes(&hashes, ty, 0).unwrap(); for &hash in &hashes { assert!(filter.contains_hash(hash), "false negative for {hash}"); } @@ -58,15 +58,16 @@ fn borrowed_values_can_be_reused() { #[test] fn duplicates_are_removed_before_construction() { - let filter = XorFilter::from_hashes([7, 7, 11, 11, 11], XorFilterType::Xor8, 0).unwrap(); + let filter = XorFilter::from_values([7_u64, 7, 11, 11, 11], XorFilterType::Xor8, 0).unwrap(); assert_eq!(filter.num_items(), Some(2)); - assert!(filter.contains_hash(7)); - assert!(filter.contains_hash(11)); + assert!(filter.contains(&7_u64)); + assert!(filter.contains(&11_u64)); } #[test] fn metadata_describes_the_compact_payload() { - let filter = XorFilter::from_hashes(0..10_000_u64, XorFilterType::Xor8, 0).unwrap(); + let hashes: Vec<_> = (0..10_000_u64).collect(); + let filter = XorFilter::from_hashes(&hashes, XorFilterType::Xor8, 0).unwrap(); assert_eq!(filter.is_empty(), Some(false)); assert_eq!(filter.num_items(), Some(10_000)); @@ -81,7 +82,7 @@ fn metadata_describes_the_compact_payload() { #[test] fn empty_filter_is_well_formed() { - let filter = XorFilter::from_hashes([], XorFilterType::Xor16, 0).unwrap(); + let filter = XorFilter::from_hashes(&[], XorFilterType::Xor16, 0).unwrap(); assert_eq!(filter.is_empty(), Some(true)); assert_eq!(filter.num_items(), Some(0)); @@ -95,9 +96,10 @@ fn false_positive_rates_follow_fingerprint_width() { const NUM_ITEMS: u64 = 50_000; const NUM_QUERIES: u64 = 100_000; - let xor8 = XorFilter::from_hashes(0..NUM_ITEMS, XorFilterType::Xor8, 0).unwrap(); - let xor16 = XorFilter::from_hashes(0..NUM_ITEMS, XorFilterType::Xor16, 0).unwrap(); - let xor32 = XorFilter::from_hashes(0..NUM_ITEMS, XorFilterType::Xor32, 0).unwrap(); + let hashes: Vec<_> = (0..NUM_ITEMS).collect(); + let xor8 = XorFilter::from_hashes(&hashes, XorFilterType::Xor8, 0).unwrap(); + let xor16 = XorFilter::from_hashes(&hashes, XorFilterType::Xor16, 0).unwrap(); + let xor32 = XorFilter::from_hashes(&hashes, XorFilterType::Xor32, 0).unwrap(); let false_positives8 = (NUM_ITEMS..NUM_ITEMS + NUM_QUERIES) .filter(|&hash| xor8.contains_hash(hash)) diff --git a/tests-integration/tests/xor_test/parts.rs b/tests-integration/tests/xor_test/parts.rs index cb1121de..88dc64f9 100644 --- a/tests-integration/tests/xor_test/parts.rs +++ b/tests-integration/tests/xor_test/parts.rs @@ -69,7 +69,7 @@ fn raw_parts_interoperate_with_xorf_in_both_directions() { assert!(imported.contains_hash(key)); } - let built = XorFilter::from_distinct_hashes(&keys, kind, 0).unwrap(); + let built = XorFilter::from_hashes(&keys, kind, 0).unwrap(); assert_eq!(built.num_items(), Some(keys.len())); let (seed, block_length) = (built.seed(), built.segment_length()); let payload = built.fingerprints().clone(); @@ -127,11 +127,11 @@ fn distinct_construction_is_bounded_and_does_not_deduplicate() { XorFilterType::Xor16, XorFilterType::Xor32, ] { - let error = XorFilter::from_distinct_hashes(&[1, 2, 1], kind, 0).unwrap_err(); + let error = XorFilter::from_hashes(&[1, 2, 1], kind, 0).unwrap_err(); assert_eq!(error.kind(), ErrorKind::InvalidArgument); assert!(error.message().contains("duplicate hashes")); for keys in [vec![], vec![0], vec![u64::MAX], vec![0, u64::MAX]] { - let f = XorFilter::from_distinct_hashes(&keys, kind, 0).unwrap(); + let f = XorFilter::from_hashes(&keys, kind, 0).unwrap(); assert_eq!(f.num_items(), Some(keys.len())); for key in keys { assert!(f.contains_hash(key)); @@ -152,7 +152,7 @@ fn construction_recovers_after_an_unpeelable_first_attempt() { z ^ (z >> 31) }) .collect(); - let first_seed = XorFilter::from_distinct_hashes(&[], XorFilterType::Xor8, 1) + let first_seed = XorFilter::from_hashes(&[], XorFilterType::Xor8, 1) .unwrap() .seed(); let reference = xorf::Xor8::from(&keys); @@ -162,17 +162,14 @@ fn construction_recovers_after_an_unpeelable_first_attempt() { XorFilterType::Xor16, XorFilterType::Xor32, ] { - let filter = XorFilter::from_distinct_hashes(&keys, kind, 1).unwrap(); + let filter = XorFilter::from_hashes(&keys, kind, 1).unwrap(); assert_eq!(filter.seed(), reference.seed); for &key in &keys { assert!(filter.contains_hash(key)); } let mut reversed = keys.clone(); reversed.reverse(); - assert_eq!( - filter, - XorFilter::from_distinct_hashes(&reversed, kind, 1).unwrap() - ); + assert_eq!(filter, XorFilter::from_hashes(&reversed, kind, 1).unwrap()); } } @@ -183,7 +180,7 @@ fn unknown_count_does_not_infer_emptiness_from_zero_fingerprints() { XorFilterType::Xor16, XorFilterType::Xor32, ] { - let empty = XorFilter::from_hashes([], kind, 0).unwrap(); + let empty = XorFilter::from_hashes(&[], kind, 0).unwrap(); let zero_hash = empty.seed().wrapping_neg(); assert_eq!(empty.is_empty(), Some(true)); assert!(!empty.contains_hash(zero_hash)); From b96d88ffd203b8e743b18489b0b5b3aa71b6da48 Mon Sep 17 00:00:00 2001 From: tison Date: Thu, 5 Nov 2026 12:09:59 +0800 Subject: [PATCH 5/6] refactor(frequencies): retain descriptive error type parameters --- datasketches/src/frequencies/sketch.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/datasketches/src/frequencies/sketch.rs b/datasketches/src/frequencies/sketch.rs index a96c912e..617b98ec 100644 --- a/datasketches/src/frequencies/sketch.rs +++ b/datasketches/src/frequencies/sketch.rs @@ -485,7 +485,7 @@ impl FrequentItemsSketch { /// Returns frequent items using the sketch maximum error as threshold. /// - /// This is equivalent to `frequent_items_with_threshold(ty, self.maximum_error())`. + /// This is equivalent to `frequent_items_with_threshold(error_type, self.maximum_error())`. /// /// # Examples /// @@ -499,11 +499,11 @@ impl FrequentItemsSketch { /// let rows = sketch.frequent_items(ErrorType::NoFalseNegatives); /// assert!(rows.iter().any(|row| *row.item() == 1)); /// ``` - pub fn frequent_items(&self, ty: ErrorType) -> Vec> + pub fn frequent_items(&self, error_type: ErrorType) -> Vec> where T: Clone, { - self.frequent_items_with_threshold(ty, self.offset) + self.frequent_items_with_threshold(error_type, self.offset) } /// Returns frequent items using a custom threshold. @@ -525,7 +525,11 @@ impl FrequentItemsSketch { /// let rows = sketch.frequent_items_with_threshold(ErrorType::NoFalsePositives, 3); /// assert!(rows.iter().any(|row| *row.item() == 1)); /// ``` - pub fn frequent_items_with_threshold(&self, ty: ErrorType, threshold: u64) -> Vec> + pub fn frequent_items_with_threshold( + &self, + error_type: ErrorType, + threshold: u64, + ) -> Vec> where T: Clone, { @@ -534,7 +538,7 @@ impl FrequentItemsSketch { for (item, count) in self.hash_map.iter() { let lower = count; let upper = count + self.offset; - let include = match ty { + let include = match error_type { ErrorType::NoFalseNegatives => upper > threshold, ErrorType::NoFalsePositives => lower > threshold, }; From 11de76ff776af908e567866d5a8a921e51da1c93 Mon Sep 17 00:00:00 2001 From: tison Date: Thu, 5 Nov 2026 12:15:13 +0800 Subject: [PATCH 6/6] refactor(xor): keep serialization sizing private --- datasketches/src/xor/filter.rs | 12 +++--------- tests-integration/tests/serde_tests/xor.rs | 2 -- tests-integration/tests/xor_test/filter.rs | 2 -- tests-integration/tests/xor_test/parts.rs | 4 ---- 4 files changed, 3 insertions(+), 17 deletions(-) diff --git a/datasketches/src/xor/filter.rs b/datasketches/src/xor/filter.rs index 39401ab8..36fc374d 100644 --- a/datasketches/src/xor/filter.rs +++ b/datasketches/src/xor/filter.rs @@ -152,10 +152,9 @@ impl XorFilter { Self::from_hashes(&hashes, ty, seed) } - /// Creates a filter from distinct precomputed 64-bit hashes without copying or sorting them. + /// Creates a filter from distinct precomputed 64-bit hashes. /// - /// The caller must supply distinct hashes. Duplicate hashes return an error; this method - /// does not remove them. The input is borrowed without modification. + /// Duplicate hashes return an error. /// Query with [`contains_hash`](Self::contains_hash) using hashes from the same source. /// The construction `seed` selects a deterministic sequence of attempts; input order /// does not affect the result. @@ -608,12 +607,7 @@ impl XorFilter { }) } - /// Returns the standard serialized size in bytes. - /// - /// # Errors - /// - /// Returns an error under the same conditions as [`serialize`](Self::serialize). - pub fn serialized_size(&self) -> Result { + fn serialized_size(&self) -> Result { let count = self .num_items .ok_or_else(|| Error::invalid_argument("standard xor format requires an item count"))?; diff --git a/tests-integration/tests/serde_tests/xor.rs b/tests-integration/tests/serde_tests/xor.rs index 94f2ac5b..18de001c 100644 --- a/tests-integration/tests/serde_tests/xor.rs +++ b/tests-integration/tests/serde_tests/xor.rs @@ -84,7 +84,6 @@ fn serialization_round_trips_standard_fingerprint_types() { let hashes: Vec<_> = (0..count).collect(); let original = XorFilter::from_hashes(&hashes, ty, 0).unwrap(); let bytes = original.serialize().unwrap(); - assert_eq!(bytes.len(), original.serialized_size().unwrap()); let restored = XorFilter::deserialize(&bytes).unwrap(); assert_eq!(restored, original); @@ -258,7 +257,6 @@ fn xor32_image_preserves_full_width_little_endian_slots() { Some(1), ) .unwrap(); - assert_eq!(filter.serialized_size().unwrap(), bytes.len()); assert_eq!(filter.serialize().unwrap(), bytes); assert_eq!(XorFilter::deserialize(&bytes).unwrap(), filter); } diff --git a/tests-integration/tests/xor_test/filter.rs b/tests-integration/tests/xor_test/filter.rs index c7b32610..cc2a714d 100644 --- a/tests-integration/tests/xor_test/filter.rs +++ b/tests-integration/tests/xor_test/filter.rs @@ -77,7 +77,6 @@ fn metadata_describes_the_compact_payload() { assert_eq!(filter.capacity() % 3, 0); assert!((9.5..10.5).contains(&filter.bits_per_item().unwrap())); assert!(filter.estimated_size() >= filter.capacity()); - assert_eq!(filter.serialized_size().unwrap(), 24 + filter.capacity()); } #[test] @@ -88,7 +87,6 @@ fn empty_filter_is_well_formed() { assert_eq!(filter.num_items(), Some(0)); assert_eq!(filter.capacity(), 30); assert_eq!(filter.bits_per_item().unwrap(), 0.0); - assert_eq!(filter.serialized_size().unwrap(), 24 + 60); } #[test] diff --git a/tests-integration/tests/xor_test/parts.rs b/tests-integration/tests/xor_test/parts.rs index 88dc64f9..9b5272ae 100644 --- a/tests-integration/tests/xor_test/parts.rs +++ b/tests-integration/tests/xor_test/parts.rs @@ -196,10 +196,6 @@ fn unknown_count_does_not_infer_emptiness_from_zero_fingerprints() { assert_eq!(unknown.bits_per_item(), None); // An omitted count must preserve the raw fingerprint comparison, even for all-zero slots. assert!(unknown.contains_hash(zero_hash)); - assert_eq!( - unknown.serialized_size().unwrap_err().kind(), - ErrorKind::InvalidArgument - ); assert_eq!( unknown.serialize().unwrap_err().kind(), ErrorKind::InvalidArgument