From 7fe28701352045e35b044af554c11896bf53baad Mon Sep 17 00:00:00 2001 From: tison Date: Fri, 28 Aug 2026 23:38:15 +0800 Subject: [PATCH 1/3] fix(req): use canonical float ordering --- datasketches/src/common/float.rs | 53 +++++++++++++++++++ datasketches/src/common/mod.rs | 1 + .../src/hash/value/canonical_float.rs | 33 ++++++++---- datasketches/src/req/value.rs | 53 +++++++++++++++---- tests-integration/tests/req_test/query.rs | 16 ++++++ 5 files changed, 135 insertions(+), 21 deletions(-) create mode 100644 datasketches/src/common/float.rs diff --git a/datasketches/src/common/float.rs b/datasketches/src/common/float.rs new file mode 100644 index 00000000..bed490a4 --- /dev/null +++ b/datasketches/src/common/float.rs @@ -0,0 +1,53 @@ +// 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. + +#[cfg(feature = "req")] +use std::cmp::Ordering; + +/// Returns a canonical `f64` bit pattern for DataSketches hashing. +#[inline(always)] +pub(crate) fn canonical_f64_bits(value: f64) -> u64 { + if value.is_nan() { + // Java's Double.doubleToLongBits() NaN value. + 0x7ff8000000000000u64 + } else { + // -0.0 + 0.0 == +0.0 under IEEE754 roundTiesToEven rounding mode, + // which Rust guarantees. Thus, adding a positive zero canonicalizes + // signed zero without a branch. + (value + 0.0).to_bits() + } +} + +/// Compares `f32` values with signed zeros equal and all NaNs equal and ordered last. +#[cfg(feature = "req")] +#[inline(always)] +pub(crate) fn canonical_cmp_f32(left: &f32, right: &f32) -> Ordering { + match left.partial_cmp(right) { + Some(ordering) => ordering, + None => left.is_nan().cmp(&right.is_nan()), + } +} + +/// Compares `f64` values with signed zeros equal and all NaNs equal and ordered last. +#[cfg(feature = "req")] +#[inline(always)] +pub(crate) fn canonical_cmp_f64(left: &f64, right: &f64) -> Ordering { + match left.partial_cmp(right) { + Some(ordering) => ordering, + None => left.is_nan().cmp(&right.is_nan()), + } +} diff --git a/datasketches/src/common/mod.rs b/datasketches/src/common/mod.rs index 6d4c6c6e..f0cb056f 100644 --- a/datasketches/src/common/mod.rs +++ b/datasketches/src/common/mod.rs @@ -22,5 +22,6 @@ mod resize; pub use self::num_std_dev::NumStdDev; pub use self::resize::ResizeFactor; +pub(crate) mod float; #[cfg(any(feature = "cpc", feature = "hll"))] pub(crate) mod inv_pow2; diff --git a/datasketches/src/hash/value/canonical_float.rs b/datasketches/src/hash/value/canonical_float.rs index 4c29b5bd..202e9329 100644 --- a/datasketches/src/hash/value/canonical_float.rs +++ b/datasketches/src/hash/value/canonical_float.rs @@ -26,6 +26,7 @@ use std::hash::Hash; use std::hash::Hasher; +use crate::common::float::canonical_f64_bits; use crate::hash::value::HashStrategy; use crate::hash::value::Value; @@ -106,15 +107,27 @@ impl HashStrategy for CanonicalFloatStrategy { impl HashStrategy for CanonicalFloatStrategy { fn hash(value: &f64, state: &mut H) { - let canonical = if value.is_nan() { - // Java's Double.doubleToLongBits() NaN value. - 0x7ff8000000000000u64 - } else { - // -0.0 + 0.0 == +0.0 under IEEE754 roundTiesToEven rounding mode, - // which Rust guarantees. Thus, by adding a positive zero we - // canonicalize signed zero without any branches in one instruction. - (value + 0.0).to_bits() - }; - canonical.hash(state); + canonical_f64_bits(*value).hash(state); + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::hash::value::calculate_hash; + + #[test] + fn canonical_hash_equates_signed_zeros_and_nans() { + assert_eq!( + calculate_hash(from_f64(-0.0)), + calculate_hash(from_f64(0.0)) + ); + + let positive_nan = f64::from_bits(0x7ff8000000000001); + let negative_nan = f64::from_bits(0xfff8000000000002); + assert_eq!( + calculate_hash(from_f64(positive_nan)), + calculate_hash(from_f64(negative_nan)) + ); } } diff --git a/datasketches/src/req/value.rs b/datasketches/src/req/value.rs index b065ae39..9c0d571e 100644 --- a/datasketches/src/req/value.rs +++ b/datasketches/src/req/value.rs @@ -21,6 +21,8 @@ use std::cmp::Ordering; use crate::codec::SketchBytes; use crate::codec::SketchSlice; +use crate::common::float::canonical_cmp_f32; +use crate::common::float::canonical_cmp_f64; use crate::error::Error; /// Trait for types that can be stored in a [`ReqSketch`](crate::req::ReqSketch). @@ -32,8 +34,7 @@ pub trait ReqValue: Sized + Clone + PartialOrd { /// Total ordering used for sketch operations (sort, compaction, rank, quantile). /// /// For integer types this is equivalent to [`Ord::cmp`]. For floating-point types - /// this delegates to [`f32::total_cmp`] / [`f64::total_cmp`] so NaN comparisons are - /// deterministic. + /// signed zeros compare equal, all NaNs compare equal, and NaNs sort after other values. fn total_cmp(&self, other: &Self) -> Ordering; /// Returns true if this value is the floating-point NaN sentinel. @@ -122,10 +123,10 @@ impl_req_value_primitive!(i64, read_i64_le, write_i64_le, Ord::cmp); impl_req_value_primitive!(u32, read_u32_le, write_u32_le, Ord::cmp); impl_req_value_primitive!(u64, read_u64_le, write_u64_le, Ord::cmp); impl_req_value_primitive!(f32, read_f32_le, write_f32_le, - |a: &f32, b: &f32| if let Some(o) = a.partial_cmp(b) { o } else { f32::total_cmp(a, b) }, + canonical_cmp_f32, nan: |x: &f32| f32::is_nan(*x)); impl_req_value_primitive!(f64, read_f64_le, write_f64_le, - |a: &f64, b: &f64| if let Some(o) = a.partial_cmp(b) { o } else { f64::total_cmp(a, b) }, + canonical_cmp_f64, nan: |x: &f64| f64::is_nan(*x)); #[cfg(test)] @@ -173,13 +174,43 @@ mod tests { } #[test] - fn total_cmp_handles_nan_for_floats() { - // Pure NaN comparisons under PartialOrd return None; total_cmp must give a definite - // Ordering. - let nan = f64::NAN; - let one = 1.0_f64; - assert_ne!(::total_cmp(&nan, &one), Ordering::Equal); - assert_eq!(::total_cmp(&nan, &nan), Ordering::Equal); + fn total_cmp_for_f32_uses_canonical_order() { + let positive_nan = f32::from_bits(0x7fc00001); + let negative_nan = f32::from_bits(0xffc00002); + + assert_eq!(::total_cmp(&-0.0, &0.0), Ordering::Equal); + assert_eq!( + ::total_cmp(&positive_nan, &negative_nan), + Ordering::Equal + ); + assert_eq!( + ::total_cmp(&positive_nan, &f32::INFINITY), + Ordering::Greater + ); + assert_eq!( + ::total_cmp(&f32::INFINITY, &positive_nan), + Ordering::Less + ); + } + + #[test] + fn total_cmp_for_f64_uses_canonical_order() { + let positive_nan = f64::from_bits(0x7ff8000000000001); + let negative_nan = f64::from_bits(0xfff8000000000002); + + assert_eq!(::total_cmp(&-0.0, &0.0), Ordering::Equal); + assert_eq!( + ::total_cmp(&positive_nan, &negative_nan), + Ordering::Equal + ); + assert_eq!( + ::total_cmp(&positive_nan, &f64::INFINITY), + Ordering::Greater + ); + assert_eq!( + ::total_cmp(&f64::INFINITY, &positive_nan), + Ordering::Less + ); } #[test] diff --git a/tests-integration/tests/req_test/query.rs b/tests-integration/tests/req_test/query.rs index c07bff93..30f30922 100644 --- a/tests-integration/tests/req_test/query.rs +++ b/tests-integration/tests/req_test/query.rs @@ -191,3 +191,19 @@ fn search_criteria_rank_consistency() -> Result<(), Error> { Ok(()) } + +#[test] +fn signed_zeros_share_rank_and_cannot_be_distinct_splits() -> Result<(), Error> { + let mut sketch = ReqSketch::default(); + sketch.update(-0.0_f64); + sketch.update(0.0_f64); + + for value in [-0.0, 0.0] { + assert_eq!(sketch.rank(&value, SearchCriteria::Exclusive)?, 0.0); + assert_eq!(sketch.rank(&value, SearchCriteria::Inclusive)?, 1.0); + } + + assert!(sketch.pmf(&[-0.0, 0.0], SearchCriteria::Inclusive).is_err()); + + Ok(()) +} From d1be6f3207df1db4a85884e516366b9f9f7226c3 Mon Sep 17 00:00:00 2001 From: tison Date: Sat, 29 Aug 2026 08:47:24 +0800 Subject: [PATCH 2/3] refactor(req): narrow value comparison contract --- datasketches/src/common/float.rs | 53 --------------- datasketches/src/common/mod.rs | 1 - .../src/hash/value/canonical_float.rs | 33 +++------ datasketches/src/req/compactor.rs | 14 ++-- datasketches/src/req/sketch.rs | 14 ++-- datasketches/src/req/sorted_view.rs | 16 +++-- datasketches/src/req/value.rs | 68 +++++++------------ .../tests/req_test/sorted_view_api.rs | 22 +++++- 8 files changed, 77 insertions(+), 144 deletions(-) delete mode 100644 datasketches/src/common/float.rs diff --git a/datasketches/src/common/float.rs b/datasketches/src/common/float.rs deleted file mode 100644 index bed490a4..00000000 --- a/datasketches/src/common/float.rs +++ /dev/null @@ -1,53 +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. - -#[cfg(feature = "req")] -use std::cmp::Ordering; - -/// Returns a canonical `f64` bit pattern for DataSketches hashing. -#[inline(always)] -pub(crate) fn canonical_f64_bits(value: f64) -> u64 { - if value.is_nan() { - // Java's Double.doubleToLongBits() NaN value. - 0x7ff8000000000000u64 - } else { - // -0.0 + 0.0 == +0.0 under IEEE754 roundTiesToEven rounding mode, - // which Rust guarantees. Thus, adding a positive zero canonicalizes - // signed zero without a branch. - (value + 0.0).to_bits() - } -} - -/// Compares `f32` values with signed zeros equal and all NaNs equal and ordered last. -#[cfg(feature = "req")] -#[inline(always)] -pub(crate) fn canonical_cmp_f32(left: &f32, right: &f32) -> Ordering { - match left.partial_cmp(right) { - Some(ordering) => ordering, - None => left.is_nan().cmp(&right.is_nan()), - } -} - -/// Compares `f64` values with signed zeros equal and all NaNs equal and ordered last. -#[cfg(feature = "req")] -#[inline(always)] -pub(crate) fn canonical_cmp_f64(left: &f64, right: &f64) -> Ordering { - match left.partial_cmp(right) { - Some(ordering) => ordering, - None => left.is_nan().cmp(&right.is_nan()), - } -} diff --git a/datasketches/src/common/mod.rs b/datasketches/src/common/mod.rs index f0cb056f..6d4c6c6e 100644 --- a/datasketches/src/common/mod.rs +++ b/datasketches/src/common/mod.rs @@ -22,6 +22,5 @@ mod resize; pub use self::num_std_dev::NumStdDev; pub use self::resize::ResizeFactor; -pub(crate) mod float; #[cfg(any(feature = "cpc", feature = "hll"))] pub(crate) mod inv_pow2; diff --git a/datasketches/src/hash/value/canonical_float.rs b/datasketches/src/hash/value/canonical_float.rs index 202e9329..4c29b5bd 100644 --- a/datasketches/src/hash/value/canonical_float.rs +++ b/datasketches/src/hash/value/canonical_float.rs @@ -26,7 +26,6 @@ use std::hash::Hash; use std::hash::Hasher; -use crate::common::float::canonical_f64_bits; use crate::hash::value::HashStrategy; use crate::hash::value::Value; @@ -107,27 +106,15 @@ impl HashStrategy for CanonicalFloatStrategy { impl HashStrategy for CanonicalFloatStrategy { fn hash(value: &f64, state: &mut H) { - canonical_f64_bits(*value).hash(state); - } -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::hash::value::calculate_hash; - - #[test] - fn canonical_hash_equates_signed_zeros_and_nans() { - assert_eq!( - calculate_hash(from_f64(-0.0)), - calculate_hash(from_f64(0.0)) - ); - - let positive_nan = f64::from_bits(0x7ff8000000000001); - let negative_nan = f64::from_bits(0xfff8000000000002); - assert_eq!( - calculate_hash(from_f64(positive_nan)), - calculate_hash(from_f64(negative_nan)) - ); + let canonical = if value.is_nan() { + // Java's Double.doubleToLongBits() NaN value. + 0x7ff8000000000000u64 + } else { + // -0.0 + 0.0 == +0.0 under IEEE754 roundTiesToEven rounding mode, + // which Rust guarantees. Thus, by adding a positive zero we + // canonicalize signed zero without any branches in one instruction. + (value + 0.0).to_bits() + }; + canonical.hash(state); } } diff --git a/datasketches/src/req/compactor.rs b/datasketches/src/req/compactor.rs index da79181c..07a6f38a 100644 --- a/datasketches/src/req/compactor.rs +++ b/datasketches/src/req/compactor.rs @@ -35,7 +35,7 @@ fn normalized_sort_state(items: &[T], claimed_sorted: bool) -> Resu if item.is_nan() { return Err(Error::deserial("REQ compactor contains a NaN item")); } - if sorted && previous.is_some_and(|previous| previous.total_cmp(item).is_gt()) { + if sorted && previous.is_some_and(|previous| previous.compare(item).is_gt()) { sorted = false; } previous = Some(item); @@ -140,7 +140,7 @@ where self.merge_sorted(&other.items); } else { let mut other_items = other.items.clone(); - other_items.sort_unstable_by(|a, b| a.total_cmp(b)); + other_items.sort_unstable_by(|a, b| a.compare(b)); self.merge_sorted(&other_items); } } @@ -158,15 +158,15 @@ where pub(super) fn count_below(&self, item: &T, inclusive: bool) -> usize { if self.is_sorted { if inclusive { - self.items.partition_point(|x| x.total_cmp(item).is_le()) + self.items.partition_point(|x| x.compare(item).is_le()) } else { - self.items.partition_point(|x| x.total_cmp(item).is_lt()) + self.items.partition_point(|x| x.compare(item).is_lt()) } } else { self.items .iter() .filter(|x| { - let ord = x.total_cmp(item); + let ord = x.compare(item); if inclusive { ord.is_le() } else { ord.is_lt() } }) .count() @@ -201,7 +201,7 @@ where // Two-pointer merge into scratch buffer while i < a.len() && j < b.len() { - if a[i].total_cmp(&b[j]).is_le() { + if a[i].compare(&b[j]).is_le() { self.scratch_buffer.push(a[i].clone()); i += 1; } else { @@ -229,7 +229,7 @@ where pub(super) fn sort(&mut self) { if !self.is_sorted { // Use unstable sort for better performance (stable not needed for REQ sketch) - self.items.sort_unstable_by(|a, b| a.total_cmp(b)); + self.items.sort_unstable_by(|a, b| a.compare(b)); self.is_sorted = true; } } diff --git a/datasketches/src/req/sketch.rs b/datasketches/src/req/sketch.rs index 6c0249b4..e37eb860 100644 --- a/datasketches/src/req/sketch.rs +++ b/datasketches/src/req/sketch.rs @@ -133,12 +133,12 @@ impl ReqSketch { } match &mut self.min_item { None => self.min_item = Some(item.clone()), - Some(cur) if item.total_cmp(cur).is_lt() => *cur = item.clone(), + Some(cur) if item.compare(cur).is_lt() => *cur = item.clone(), _ => {} } match &mut self.max_item { None => self.max_item = Some(item.clone()), - Some(cur) if item.total_cmp(cur).is_gt() => *cur = item.clone(), + Some(cur) if item.compare(cur).is_gt() => *cur = item.clone(), _ => {} } @@ -285,14 +285,14 @@ impl ReqSketch { if let Some(m) = &other.min_item { match &self.min_item { None => self.min_item = Some(m.clone()), - Some(cur) if m.total_cmp(cur).is_lt() => self.min_item = Some(m.clone()), + Some(cur) if m.compare(cur).is_lt() => self.min_item = Some(m.clone()), _ => {} } } if let Some(m) = &other.max_item { match &self.max_item { None => self.max_item = Some(m.clone()), - Some(cur) if m.total_cmp(cur).is_gt() => self.max_item = Some(m.clone()), + Some(cur) if m.compare(cur).is_gt() => self.max_item = Some(m.clone()), _ => {} } } @@ -631,7 +631,7 @@ impl ReqSketch { if min.is_nan() || max.is_nan() { return Err(Error::deserial("REQ sketch min or max item is NaN")); } - if min.total_cmp(max).is_gt() { + if min.compare(max).is_gt() { return Err(Error::deserial( "REQ sketch min item is greater than max item", )); @@ -667,10 +667,10 @@ impl ReqSketch { let mut mn = first.clone(); let mut mx = first.clone(); for x in iter { - if x.total_cmp(&mn).is_lt() { + if x.compare(&mn).is_lt() { mn = x.clone(); } - if x.total_cmp(&mx).is_gt() { + if x.compare(&mx).is_gt() { mx = x.clone(); } } diff --git a/datasketches/src/req/sorted_view.rs b/datasketches/src/req/sorted_view.rs index a8c1d985..5c2d5a1b 100644 --- a/datasketches/src/req/sorted_view.rs +++ b/datasketches/src/req/sorted_view.rs @@ -60,7 +60,7 @@ where } // Sort by item value - use unstable sort for better performance - weighted_items.sort_unstable_by(|a, b| a.0.total_cmp(&b.0)); + weighted_items.sort_unstable_by(|a, b| a.0.compare(&b.0)); let mut items: Vec = Vec::with_capacity(weighted_items.len()); let mut cumulative_weights = Vec::with_capacity(weighted_items.len()); @@ -68,7 +68,7 @@ where for (item, weight) in weighted_items { if let Some(last) = items.last() { - if matches!(last.total_cmp(&item), std::cmp::Ordering::Equal) { + if matches!(last.compare(&item), std::cmp::Ordering::Equal) { cumulative_weight += weight; let last_idx = cumulative_weights.len() - 1; cumulative_weights[last_idx] = cumulative_weight; @@ -122,7 +122,7 @@ where SearchCriteria::Inclusive => { // Find the last position where items[i] <= item // partition_point finds first index where predicate is false - let pos = self.items.partition_point(|x| x.total_cmp(item).is_le()); + let pos = self.items.partition_point(|x| x.compare(item).is_le()); if pos == 0 { Ok(0.0) } else { @@ -131,7 +131,7 @@ where } SearchCriteria::Exclusive => { // Find the last position where items[i] < item - let pos = self.items.partition_point(|x| x.total_cmp(item).is_lt()); + let pos = self.items.partition_point(|x| x.compare(item).is_lt()); if pos == 0 { Ok(0.0) } else { @@ -259,9 +259,11 @@ where // Private helper methods fn validate_split_points(&self, split_points: &[T]) -> Result<(), Error> { - // Check that split points are monotonically increasing - for i in 1..split_points.len() { - if split_points[i - 1].total_cmp(&split_points[i]).is_ge() { + for (i, split_point) in split_points.iter().enumerate() { + if split_point.is_nan() { + return Err(Error::invalid_argument("Split points must not be NaN")); + } + if i > 0 && split_points[i - 1].compare(split_point).is_ge() { return Err(Error::invalid_argument( "Split points must be unique and monotonically increasing".to_string(), )); diff --git a/datasketches/src/req/value.rs b/datasketches/src/req/value.rs index 9c0d571e..21e64efd 100644 --- a/datasketches/src/req/value.rs +++ b/datasketches/src/req/value.rs @@ -21,21 +21,15 @@ use std::cmp::Ordering; use crate::codec::SketchBytes; use crate::codec::SketchSlice; -use crate::common::float::canonical_cmp_f32; -use crate::common::float::canonical_cmp_f64; use crate::error::Error; /// Trait for types that can be stored in a [`ReqSketch`](crate::req::ReqSketch). /// -/// Provides total ordering (so floating-point types with NaN are well-defined under -/// sketch operations) and binary serialization compatible with the Apache DataSketches +/// Provides ordering and binary serialization compatible with the Apache DataSketches /// REQ wire format used by the C++ and Java reference implementations. pub trait ReqValue: Sized + Clone + PartialOrd { - /// Total ordering used for sketch operations (sort, compaction, rank, quantile). - /// - /// For integer types this is equivalent to [`Ord::cmp`]. For floating-point types - /// signed zeros compare equal, all NaNs compare equal, and NaNs sort after other values. - fn total_cmp(&self, other: &Self) -> Ordering; + /// Compares two values. See each implementation for its ordering semantics. + fn compare(&self, other: &Self) -> Ordering; /// Returns true if this value is the floating-point NaN sentinel. /// @@ -61,7 +55,7 @@ macro_rules! impl_req_value_primitive { ($t:ty, $read:ident, $write:ident, $cmp:expr, nan: $nan:expr) => { impl ReqValue for $t { #[inline(always)] - fn total_cmp(&self, other: &Self) -> Ordering { + fn compare(&self, other: &Self) -> Ordering { $cmp(self, other) } @@ -93,7 +87,7 @@ macro_rules! impl_req_value_primitive { ($t:ty, $read:ident, $write:ident, $cmp:expr) => { impl ReqValue for $t { #[inline(always)] - fn total_cmp(&self, other: &Self) -> Ordering { + fn compare(&self, other: &Self) -> Ordering { $cmp(self, other) } @@ -123,10 +117,10 @@ impl_req_value_primitive!(i64, read_i64_le, write_i64_le, Ord::cmp); impl_req_value_primitive!(u32, read_u32_le, write_u32_le, Ord::cmp); impl_req_value_primitive!(u64, read_u64_le, write_u64_le, Ord::cmp); impl_req_value_primitive!(f32, read_f32_le, write_f32_le, - canonical_cmp_f32, + |left: &f32, right: &f32| left.partial_cmp(right).expect("REQ values must not be NaN"), nan: |x: &f32| f32::is_nan(*x)); impl_req_value_primitive!(f64, read_f64_le, write_f64_le, - canonical_cmp_f64, + |left: &f64, right: &f64| left.partial_cmp(right).expect("REQ values must not be NaN"), nan: |x: &f64| f64::is_nan(*x)); #[cfg(test)] @@ -174,49 +168,33 @@ mod tests { } #[test] - fn total_cmp_for_f32_uses_canonical_order() { - let positive_nan = f32::from_bits(0x7fc00001); - let negative_nan = f32::from_bits(0xffc00002); - - assert_eq!(::total_cmp(&-0.0, &0.0), Ordering::Equal); - assert_eq!( - ::total_cmp(&positive_nan, &negative_nan), - Ordering::Equal - ); + fn compare_for_f32_uses_numeric_order() { + assert_eq!(::compare(&-0.0, &0.0), Ordering::Equal); assert_eq!( - ::total_cmp(&positive_nan, &f32::INFINITY), - Ordering::Greater - ); - assert_eq!( - ::total_cmp(&f32::INFINITY, &positive_nan), + ::compare(&f32::NEG_INFINITY, &f32::INFINITY), Ordering::Less ); } #[test] - fn total_cmp_for_f64_uses_canonical_order() { - let positive_nan = f64::from_bits(0x7ff8000000000001); - let negative_nan = f64::from_bits(0xfff8000000000002); - - assert_eq!(::total_cmp(&-0.0, &0.0), Ordering::Equal); + fn compare_for_f64_uses_numeric_order() { + assert_eq!(::compare(&-0.0, &0.0), Ordering::Equal); assert_eq!( - ::total_cmp(&positive_nan, &negative_nan), - Ordering::Equal - ); - assert_eq!( - ::total_cmp(&positive_nan, &f64::INFINITY), - Ordering::Greater - ); - assert_eq!( - ::total_cmp(&f64::INFINITY, &positive_nan), + ::compare(&f64::NEG_INFINITY, &f64::INFINITY), Ordering::Less ); } #[test] - fn total_cmp_for_integers_matches_ord() { - assert_eq!(::total_cmp(&3, &5), Ordering::Less); - assert_eq!(::total_cmp(&5, &5), Ordering::Equal); - assert_eq!(::total_cmp(&7, &5), Ordering::Greater); + #[should_panic(expected = "REQ values must not be NaN")] + fn compare_for_floats_rejects_nan() { + ::compare(&f64::NAN, &0.0); + } + + #[test] + fn compare_for_integers_matches_ord() { + assert_eq!(::compare(&3, &5), Ordering::Less); + assert_eq!(::compare(&5, &5), Ordering::Equal); + assert_eq!(::compare(&7, &5), Ordering::Greater); } } diff --git a/tests-integration/tests/req_test/sorted_view_api.rs b/tests-integration/tests/req_test/sorted_view_api.rs index b5ed000b..32a518b5 100644 --- a/tests-integration/tests/req_test/sorted_view_api.rs +++ b/tests-integration/tests/req_test/sorted_view_api.rs @@ -119,18 +119,38 @@ fn view_rank_is_primary_query_name() { } #[test] -fn nan_query_items_are_rejected() { +fn nan_query_items_and_split_points_are_rejected() { let sketch = populated_sketch(100); let error = sketch .rank(&f64::NAN, SearchCriteria::Inclusive) .unwrap_err(); assert_eq!(error.kind(), ErrorKind::InvalidArgument); + assert_eq!( + sketch + .pmf(&[f64::NAN], SearchCriteria::Inclusive) + .unwrap_err() + .kind(), + ErrorKind::InvalidArgument + ); + assert_eq!( + sketch + .cdf(&[50.0, f64::NAN], SearchCriteria::Inclusive) + .unwrap_err() + .kind(), + ErrorKind::InvalidArgument + ); let view = sketch.sorted_view(); assert_that!( view.rank(&f64::NAN, SearchCriteria::Inclusive), err(anything()) ); + assert_eq!( + view.pmf(&[f64::NAN], SearchCriteria::Inclusive) + .unwrap_err() + .kind(), + ErrorKind::InvalidArgument + ); } #[test] From 6145ffb04872895600f616f9171236a254eb55be Mon Sep 17 00:00:00 2001 From: tison Date: Sat, 29 Aug 2026 09:31:55 +0800 Subject: [PATCH 3/3] refactor(req): expand primitive value implementations --- datasketches/src/req/value.rs | 207 ++++++++++++++++++++++------------ 1 file changed, 136 insertions(+), 71 deletions(-) diff --git a/datasketches/src/req/value.rs b/datasketches/src/req/value.rs index 21e64efd..a2c5ed4b 100644 --- a/datasketches/src/req/value.rs +++ b/datasketches/src/req/value.rs @@ -18,9 +18,11 @@ //! Trait for types storable in a [`ReqSketch`](crate::req::ReqSketch). use std::cmp::Ordering; +use std::mem::size_of; use crate::codec::SketchBytes; use crate::codec::SketchSlice; +use crate::codec::assert::insufficient_data; use crate::error::Error; /// Trait for types that can be stored in a [`ReqSketch`](crate::req::ReqSketch). @@ -50,78 +52,141 @@ pub trait ReqValue: Sized + Clone + PartialOrd { fn deserialize_value(cursor: &mut SketchSlice<'_>) -> Result; } -macro_rules! impl_req_value_primitive { - // Form with explicit is_nan body (for float types). - ($t:ty, $read:ident, $write:ident, $cmp:expr, nan: $nan:expr) => { - impl ReqValue for $t { - #[inline(always)] - fn compare(&self, other: &Self) -> Ordering { - $cmp(self, other) - } - - fn serialize_size(_item: &Self) -> usize { - std::mem::size_of::<$t>() - } - - fn serialize_value(&self, bytes: &mut SketchBytes) { - bytes.$write(*self); - } - - fn deserialize_value(cursor: &mut SketchSlice<'_>) -> Result { - cursor.$read().map_err(|_| { - Error::insufficient_data(concat!( - "failed to read ", - stringify!($t), - " from REQ sketch" - )) - }) - } - - #[inline(always)] - fn is_nan(&self) -> bool { - $nan(self) - } - } - }; - // Form without is_nan (for integer types — default returns false). - ($t:ty, $read:ident, $write:ident, $cmp:expr) => { - impl ReqValue for $t { - #[inline(always)] - fn compare(&self, other: &Self) -> Ordering { - $cmp(self, other) - } - - fn serialize_size(_item: &Self) -> usize { - std::mem::size_of::<$t>() - } - - fn serialize_value(&self, bytes: &mut SketchBytes) { - bytes.$write(*self); - } - - fn deserialize_value(cursor: &mut SketchSlice<'_>) -> Result { - cursor.$read().map_err(|_| { - Error::insufficient_data(concat!( - "failed to read ", - stringify!($t), - " from REQ sketch" - )) - }) - } - } - }; +impl ReqValue for i32 { + #[inline(always)] + fn compare(&self, other: &Self) -> Ordering { + self.cmp(other) + } + + fn serialize_size(_item: &Self) -> usize { + size_of::() + } + + fn serialize_value(&self, bytes: &mut SketchBytes) { + bytes.write_i32_le(*self); + } + + fn deserialize_value(cursor: &mut SketchSlice<'_>) -> Result { + cursor + .read_i32_le() + .map_err(insufficient_data("failed to read i32 from REQ sketch")) + } } -impl_req_value_primitive!(i32, read_i32_le, write_i32_le, Ord::cmp); -impl_req_value_primitive!(i64, read_i64_le, write_i64_le, Ord::cmp); -impl_req_value_primitive!(u32, read_u32_le, write_u32_le, Ord::cmp); -impl_req_value_primitive!(u64, read_u64_le, write_u64_le, Ord::cmp); -impl_req_value_primitive!(f32, read_f32_le, write_f32_le, - |left: &f32, right: &f32| left.partial_cmp(right).expect("REQ values must not be NaN"), - nan: |x: &f32| f32::is_nan(*x)); -impl_req_value_primitive!(f64, read_f64_le, write_f64_le, - |left: &f64, right: &f64| left.partial_cmp(right).expect("REQ values must not be NaN"), - nan: |x: &f64| f64::is_nan(*x)); +impl ReqValue for i64 { + #[inline(always)] + fn compare(&self, other: &Self) -> Ordering { + self.cmp(other) + } + + fn serialize_size(_item: &Self) -> usize { + size_of::() + } + + fn serialize_value(&self, bytes: &mut SketchBytes) { + bytes.write_i64_le(*self); + } + + fn deserialize_value(cursor: &mut SketchSlice<'_>) -> Result { + cursor + .read_i64_le() + .map_err(insufficient_data("failed to read i64 from REQ sketch")) + } +} + +impl ReqValue for u32 { + #[inline(always)] + fn compare(&self, other: &Self) -> Ordering { + self.cmp(other) + } + + fn serialize_size(_item: &Self) -> usize { + size_of::() + } + + fn serialize_value(&self, bytes: &mut SketchBytes) { + bytes.write_u32_le(*self); + } + + fn deserialize_value(cursor: &mut SketchSlice<'_>) -> Result { + cursor + .read_u32_le() + .map_err(insufficient_data("failed to read u32 from REQ sketch")) + } +} + +impl ReqValue for u64 { + #[inline(always)] + fn compare(&self, other: &Self) -> Ordering { + self.cmp(other) + } + + fn serialize_size(_item: &Self) -> usize { + size_of::() + } + + fn serialize_value(&self, bytes: &mut SketchBytes) { + bytes.write_u64_le(*self); + } + + fn deserialize_value(cursor: &mut SketchSlice<'_>) -> Result { + cursor + .read_u64_le() + .map_err(insufficient_data("failed to read u64 from REQ sketch")) + } +} + +impl ReqValue for f32 { + #[inline(always)] + fn compare(&self, other: &Self) -> Ordering { + self.partial_cmp(other).unwrap() + } + + #[inline(always)] + fn is_nan(&self) -> bool { + f32::is_nan(*self) + } + + fn serialize_size(_item: &Self) -> usize { + size_of::() + } + + fn serialize_value(&self, bytes: &mut SketchBytes) { + bytes.write_f32_le(*self); + } + + fn deserialize_value(cursor: &mut SketchSlice<'_>) -> Result { + cursor + .read_f32_le() + .map_err(insufficient_data("failed to read f32 from REQ sketch")) + } +} + +impl ReqValue for f64 { + #[inline(always)] + fn compare(&self, other: &Self) -> Ordering { + self.partial_cmp(other).unwrap() + } + + #[inline(always)] + fn is_nan(&self) -> bool { + f64::is_nan(*self) + } + + fn serialize_size(_item: &Self) -> usize { + size_of::() + } + + fn serialize_value(&self, bytes: &mut SketchBytes) { + bytes.write_f64_le(*self); + } + + fn deserialize_value(cursor: &mut SketchSlice<'_>) -> Result { + cursor + .read_f64_le() + .map_err(insufficient_data("failed to read f64 from REQ sketch")) + } +} #[cfg(test)] mod tests { @@ -186,7 +251,7 @@ mod tests { } #[test] - #[should_panic(expected = "REQ values must not be NaN")] + #[should_panic] fn compare_for_floats_rejects_nan() { ::compare(&f64::NAN, &0.0); }