From a0b81bf3eaf6c04e788f23638156934b27b47fc0 Mon Sep 17 00:00:00 2001 From: Max Freedom Pollard <272618364+MaxFreedomPollard@users.noreply.github.com> Date: Mon, 7 Sep 2026 14:12:10 -0400 Subject: [PATCH] fix(types): hash 0.0 and -0.0 to the same value The Float and Double wrappers compare zeros numerically, so 0.0 == -0.0, but they hashed the raw bit pattern, which differs by the sign bit. That breaks the Hash and Eq contract: a HashMap or HashSet keyed on these values, such as the cross-shard GROUP BY buffer, puts equal keys in separate buckets. Hash the bits of positive zero instead, matching what Postgres does in hashfloat4 and hashfloat8. --- pgdog-config/src/data_types.rs | 36 +++++++++++++++++++++++++++++ pgdog-postgres-types/src/datum.rs | 35 ++++++++++++++++++++++++++++ pgdog-postgres-types/src/double.rs | 33 +++++++++++++++++++++++--- pgdog-postgres-types/src/float.rs | 11 ++++++--- pgdog-vector/src/float.rs | 37 ++++++++++++++++++++++++++++++ 5 files changed, 146 insertions(+), 6 deletions(-) diff --git a/pgdog-config/src/data_types.rs b/pgdog-config/src/data_types.rs index 3fea2c6fd..af182a521 100644 --- a/pgdog-config/src/data_types.rs +++ b/pgdog-config/src/data_types.rs @@ -50,6 +50,11 @@ impl Hash for Float { if self.0.is_nan() { // All NaN values hash to the same value 0u8.hash(state); + } else if self.0 == 0.0 { + // 0.0 and -0.0 compare equal but have different bit patterns, + // so they must hash to the same value. Postgres normalizes the + // sign of zero the same way, in hashfloat4. + 0.0_f32.to_bits().hash(state); } else { // Use bit representation for consistent hashing self.0.to_bits().hash(state); @@ -184,3 +189,34 @@ impl From> for Vector { Self { values: value } } } + +#[cfg(test)] +mod test { + use super::*; + use std::collections::HashSet; + use std::collections::hash_map::DefaultHasher; + + fn hash_of(float: Float) -> u64 { + let mut hasher = DefaultHasher::new(); + float.hash(&mut hasher); + hasher.finish() + } + + #[test] + fn test_negative_zero_hashes_like_zero() { + assert_eq!(Float(0.0), Float(-0.0)); + assert_eq!(hash_of(Float(0.0)), hash_of(Float(-0.0))); + + let mut set = HashSet::new(); + set.insert(Vector::from(vec![0.0_f32, 1.0])); + set.insert(Vector::from(vec![-0.0_f32, 1.0])); + assert_eq!(set.len(), 1); + } + + #[test] + fn test_distinct_values_still_hash_apart() { + assert_ne!(hash_of(Float(1.0)), hash_of(Float(-1.0))); + assert_ne!(hash_of(Float(0.0)), hash_of(Float(1.0))); + assert_ne!(hash_of(Float(f32::NAN)), hash_of(Float(0.0))); + } +} diff --git a/pgdog-postgres-types/src/datum.rs b/pgdog-postgres-types/src/datum.rs index 8978fda71..71d1d2af0 100644 --- a/pgdog-postgres-types/src/datum.rs +++ b/pgdog-postgres-types/src/datum.rs @@ -343,6 +343,15 @@ mod tests { use super::*; use bytes::{BufMut, BytesMut}; use std::assert_matches; + use std::collections::HashSet; + use std::collections::hash_map::DefaultHasher; + use std::hash::{Hash, Hasher}; + + fn hash_of(datum: &Datum) -> u64 { + let mut hasher = DefaultHasher::new(); + datum.hash(&mut hasher); + hasher.finish() + } #[test] fn test_multidimensional_text_array_falls_back_to_unknown() { @@ -378,4 +387,30 @@ mod tests { assert_matches!(datum, Datum::Unknown(_)); assert_eq!(datum.encode(Format::Binary).unwrap(), input); } + + #[test] + fn test_negative_zero_datums_hash_like_zero() { + // Datum derives both Hash and PartialEq, and cross-shard GROUP BY + // keys a HashMap on the grouped Datums, so every float-carrying + // shape has to agree that -0.0 and 0.0 belong in one bucket. + let pairs = [ + (Datum::Float(Float(0.0)), Datum::Float(Float(-0.0))), + (Datum::Double(Double(0.0)), Datum::Double(Double(-0.0))), + ( + Datum::Vector(Vector::from(vec![0.0_f32, 1.0])), + Datum::Vector(Vector::from(vec![-0.0_f32, 1.0])), + ), + ]; + + for (zero, neg_zero) in pairs { + assert_eq!(zero, neg_zero); + assert_eq!(hash_of(&zero), hash_of(&neg_zero)); + + let mut set = HashSet::new(); + set.insert(zero.clone()); + set.insert(neg_zero); + assert_eq!(set.len(), 1); + assert!(set.contains(&zero)); + } + } } diff --git a/pgdog-postgres-types/src/double.rs b/pgdog-postgres-types/src/double.rs index 74249d00c..dcc2b17ab 100644 --- a/pgdog-postgres-types/src/double.rs +++ b/pgdog-postgres-types/src/double.rs @@ -142,6 +142,11 @@ impl Hash for Double { if self.0.is_nan() { // All NaN values hash to the same value 0u8.hash(state); + } else if self.0 == 0.0 { + // 0.0 and -0.0 compare equal but have different bit patterns, + // so they must hash to the same value. Postgres normalizes the + // sign of zero the same way, in hashfloat8. + 0.0_f64.to_bits().hash(state); } else { // Use bit representation for consistent hashing self.0.to_bits().hash(state); @@ -278,7 +283,7 @@ mod tests { assert_eq!(hash1, hash2); - // Different values should (likely) have different hashes + // 0.0 and -0.0 compare equal, so they must hash the same let mut hasher3 = DefaultHasher::new(); zero.hash(&mut hasher3); let hash3 = hasher3.finish(); @@ -287,8 +292,30 @@ mod tests { neg_zero.hash(&mut hasher4); let hash4 = hasher4.finish(); - // Note: 0.0 and -0.0 have different bit patterns - assert_ne!(hash3, hash4); + assert_eq!(zero, neg_zero); + assert_eq!(hash3, hash4); + + // Values that differ must still land on different hashes + let mut hasher5 = DefaultHasher::new(); + Double(1.0).hash(&mut hasher5); + assert_ne!(hash3, hasher5.finish()); + } + + #[test] + fn test_double_negative_zero_groups_with_zero() { + use std::collections::HashSet; + + // Cross-shard GROUP BY buckets rows in a HashMap keyed on the + // grouped values, so -0.0 coming back from one shard has to land + // in the same bucket as 0.0 from another, the way it would on a + // single Postgres node. + let mut set = HashSet::new(); + set.insert(Double(0.0)); + set.insert(Double(-0.0)); + + assert_eq!(set.len(), 1); + assert!(set.contains(&Double(-0.0))); + assert!(set.contains(&Double(0.0))); } #[test] diff --git a/pgdog-postgres-types/src/float.rs b/pgdog-postgres-types/src/float.rs index f1b64ad0b..1974e702c 100644 --- a/pgdog-postgres-types/src/float.rs +++ b/pgdog-postgres-types/src/float.rs @@ -192,7 +192,7 @@ mod tests { assert_eq!(hash1, hash2); - // Different values should (likely) have different hashes + // 0.0 and -0.0 compare equal, so they must hash the same let mut hasher3 = DefaultHasher::new(); zero.hash(&mut hasher3); let hash3 = hasher3.finish(); @@ -201,8 +201,13 @@ mod tests { neg_zero.hash(&mut hasher4); let hash4 = hasher4.finish(); - // Note: 0.0 and -0.0 have different bit patterns - assert_ne!(hash3, hash4); + assert_eq!(zero, neg_zero); + assert_eq!(hash3, hash4); + + // Values that differ must still land on different hashes + let mut hasher5 = DefaultHasher::new(); + Float(1.0).hash(&mut hasher5); + assert_ne!(hash3, hasher5.finish()); } #[test] diff --git a/pgdog-vector/src/float.rs b/pgdog-vector/src/float.rs index b2d84cc6c..e463d3779 100644 --- a/pgdog-vector/src/float.rs +++ b/pgdog-vector/src/float.rs @@ -47,6 +47,11 @@ impl Hash for Float { if self.0.is_nan() { // All NaN values hash to the same value 0u8.hash(state); + } else if self.0 == 0.0 { + // 0.0 and -0.0 compare equal but have different bit patterns, + // so they must hash to the same value. Postgres normalizes the + // sign of zero the same way, in hashfloat4. + 0.0_f32.to_bits().hash(state); } else { // Use bit representation for consistent hashing self.0.to_bits().hash(state); @@ -81,3 +86,35 @@ impl From for f32 { value.0 } } + +#[cfg(test)] +mod tests { + use super::*; + use std::collections::HashSet; + use std::collections::hash_map::DefaultHasher; + + fn hash_of(float: Float) -> u64 { + let mut hasher = DefaultHasher::new(); + float.hash(&mut hasher); + hasher.finish() + } + + #[test] + fn test_negative_zero_hashes_like_zero() { + assert_eq!(Float(0.0), Float(-0.0)); + assert_eq!(hash_of(Float(0.0)), hash_of(Float(-0.0))); + + let mut set = HashSet::new(); + set.insert(Float(0.0)); + set.insert(Float(-0.0)); + assert_eq!(set.len(), 1); + assert!(set.contains(&Float(-0.0))); + } + + #[test] + fn test_distinct_values_still_hash_apart() { + assert_ne!(hash_of(Float(1.0)), hash_of(Float(-1.0))); + assert_ne!(hash_of(Float(0.0)), hash_of(Float(1.0))); + assert_ne!(hash_of(Float(f32::NAN)), hash_of(Float(0.0))); + } +}