fix integer/floating point promotion logic in v3 value compaction

Author: tobz

lib/saluki-components/src/encoders/datadog/metrics/v3/types.rs

@@ -80,27 +80,51 @@ impl V3ValueType {
     pub fn as_u64(self) -> u64 {
         self as u64
     }
+}
 
-    /// Determines the best value type for a given f64 value.
-    ///
-    /// Prefers smaller representations when lossless:
-    /// - Zero for 0.0
-    /// - Sint64 for integers that fit in 49 bits
-    /// - Float32 for values representable as f32
-    /// - Float64 otherwise
-    pub fn for_value(v: f64) -> Self {
+/// Intermediate point classification for value type compaction.
+///
+/// This provides finer-grained classification than [`V3ValueType`] to avoid
+/// precision loss when combining different value types. In particular, it
+/// distinguishes small integers (that fit losslessly in f32) from large integers
+/// (that don't), so that mixing a large integer with a Float32 value correctly
+/// escalates to Float64 rather than silently truncating the integer.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+#[repr(u8)]
+enum PointKind {
+    /// Value is zero.
+    Zero = 0,
+    /// Integer with |v| <= 2^24, fits losslessly in both sint64 and f32.
+    Int24 = 1,
+    /// Integer with |v| > 2^24, fits in sint64 varint but NOT losslessly in f32.
+    Int48 = 2,
+    /// Fractional value exactly representable as f32.
+    Float32 = 3,
+    /// Everything else — requires full f64 precision.
+    Float64 = 4,
+}
+
+/// Maximum integer magnitude that fits losslessly in f32 (2^24).
+const F32_INT_MAX: i64 = 1 << 24;
+
+impl PointKind {
+    /// Classifies a single f64 value.
+    fn for_value(v: f64) -> Self {
         if v == 0.0 {
             return Self::Zero;
         }
 
-        // Varint range that fits in 7 bytes or less (49 bits)
+        // Varint range that fits in 7 bytes or less (49 bits).
         const VARINT_WIDTH: i32 = 7 * 7 - 1;
         const MAX_INT: i64 = 1 << VARINT_WIDTH;
         const MIN_INT: i64 = -MAX_INT;
 
         let i = v as i64;
         if (MIN_INT..MAX_INT).contains(&i) && (i as f64) == v {
-            return Self::Sint64;
+            if (-F32_INT_MAX..=F32_INT_MAX).contains(&i) {
+                return Self::Int24;
+            }
+            return Self::Int48;
         }
 
         if (v as f32 as f64) == v {
@@ -110,46 +134,114 @@ impl V3ValueType {
         Self::Float64
     }
 
-    /// Returns the maximum (largest encoding) of two value types.
-    pub fn max(self, other: Self) -> Self {
-        if (other as u8) > (self as u8) {
-            other
-        } else {
-            self
+    /// Combines two point kinds into the smallest kind that can represent both.
+    ///
+    /// This is `max(self, other)` in all cases **except**:
+    /// - `Int48 + Float32 = Float64` (and vice versa), because large integers
+    ///   lose precision in f32, and fractional values can't be stored as sint64.
+    fn union(self, other: Self) -> Self {
+        match (self, other) {
+            (Self::Int48, Self::Float32) | (Self::Float32, Self::Int48) => Self::Float64,
+            _ => self.max(other),
         }
     }
+
+    /// Converts to the wire-format value type.
+    fn to_value_type(self) -> V3ValueType {
+        match self {
+            Self::Zero => V3ValueType::Zero,
+            Self::Int24 | Self::Int48 => V3ValueType::Sint64,
+            Self::Float32 => V3ValueType::Float32,
+            Self::Float64 => V3ValueType::Float64,
+        }
+    }
+}
+
+/// Determines the best [`V3ValueType`] for a set of f64 values.
+///
+/// Uses [`PointKind`] internally to avoid precision loss when mixing
+/// large integers with fractional float32 values.
+pub(super) fn value_type_for_values(values: impl Iterator<Item = f64>) -> V3ValueType {
+    let mut kind = PointKind::Zero;
+    for v in values {
+        kind = kind.union(PointKind::for_value(v));
+    }
+    kind.to_value_type()
 }
 
 #[cfg(test)]
 mod tests {
     use super::*;
 
     #[test]
-    fn test_value_type_for_value() {
-        assert_eq!(V3ValueType::for_value(0.0), V3ValueType::Zero);
-        assert_eq!(V3ValueType::for_value(100.0), V3ValueType::Sint64);
-        assert_eq!(V3ValueType::for_value(-100.0), V3ValueType::Sint64);
-        assert_eq!(V3ValueType::for_value(1.5), V3ValueType::Float32);
-        assert_eq!(V3ValueType::for_value(2.75), V3ValueType::Float32);
-
-        // Large integers that don't fit in 49 bits AND can't be exactly represented in f32
-        // Powers of 2 like (1 << 50) can be exactly represented in f32, so we add 1
-        // to make it an odd number that requires more precision than f32 provides
+    fn test_point_kind_classification() {
+        // Zero
+        assert_eq!(PointKind::for_value(0.0), PointKind::Zero);
+
+        // Small integers (fit in f32)
+        assert_eq!(PointKind::for_value(100.0), PointKind::Int24);
+        assert_eq!(PointKind::for_value(-100.0), PointKind::Int24);
+        assert_eq!(PointKind::for_value((1 << 24) as f64), PointKind::Int24);
+        assert_eq!(PointKind::for_value(-((1 << 24) as f64)), PointKind::Int24);
+
+        // Large integers (don't fit losslessly in f32)
+        assert_eq!(PointKind::for_value(((1 << 24) + 1) as f64), PointKind::Int48);
+        assert_eq!(PointKind::for_value((1i64 << 30) as f64), PointKind::Int48);
+
+        // Float32
+        assert_eq!(PointKind::for_value(1.5), PointKind::Float32);
+        assert_eq!(PointKind::for_value(2.75), PointKind::Float32);
+
+        // Float64
+        assert_eq!(PointKind::for_value(std::f64::consts::PI), PointKind::Float64);
         let large = ((1i64 << 50) + 1) as f64;
-        assert_eq!(V3ValueType::for_value(large), V3ValueType::Float64);
+        assert_eq!(PointKind::for_value(large), PointKind::Float64);
+    }
 
-        // Values that require f64 precision - use PI which has more precision than f32 can hold
-        // and isn't an integer, so it won't be stored as Sint64
-        let pi = std::f64::consts::PI;
-        // PI requires full f64 precision to store exactly
-        assert_eq!(V3ValueType::for_value(pi), V3ValueType::Float64);
+    #[test]
+    fn test_point_kind_union() {
+        // Standard widening (max)
+        assert_eq!(PointKind::Zero.union(PointKind::Int24), PointKind::Int24);
+        assert_eq!(PointKind::Int24.union(PointKind::Int48), PointKind::Int48);
+        assert_eq!(PointKind::Int24.union(PointKind::Float32), PointKind::Float32);
+        assert_eq!(PointKind::Float32.union(PointKind::Float64), PointKind::Float64);
+        assert_eq!(PointKind::Float64.union(PointKind::Zero), PointKind::Float64);
+
+        // The critical case: large integer + float32 must escalate to float64
+        assert_eq!(PointKind::Int48.union(PointKind::Float32), PointKind::Float64);
+        assert_eq!(PointKind::Float32.union(PointKind::Int48), PointKind::Float64);
     }
 
     #[test]
-    fn test_value_type_max() {
-        assert_eq!(V3ValueType::Zero.max(V3ValueType::Sint64), V3ValueType::Sint64);
-        assert_eq!(V3ValueType::Sint64.max(V3ValueType::Float32), V3ValueType::Float32);
-        assert_eq!(V3ValueType::Float32.max(V3ValueType::Float64), V3ValueType::Float64);
-        assert_eq!(V3ValueType::Float64.max(V3ValueType::Zero), V3ValueType::Float64);
+    fn test_value_type_for_values() {
+        // All zeros
+        assert_eq!(value_type_for_values([0.0, 0.0].into_iter()), V3ValueType::Zero);
+
+        // Small integers
+        assert_eq!(value_type_for_values([100.0, 200.0].into_iter()), V3ValueType::Sint64);
+
+        // Large integers
+        assert_eq!(
+            value_type_for_values([(1i64 << 30) as f64, 200.0].into_iter()),
+            V3ValueType::Sint64
+        );
+
+        // Small integer + float32 → Float32 (safe, small int fits in f32)
+        assert_eq!(value_type_for_values([100.0, 1.5].into_iter()), V3ValueType::Float32);
+
+        // Large integer + float32 → Float64 (the bug fix!)
+        assert_eq!(
+            value_type_for_values([(1i64 << 30) as f64, 1.5].into_iter()),
+            V3ValueType::Float64
+        );
+
+        // Float64 value forces Float64
+        assert_eq!(
+            value_type_for_values([100.0, std::f64::consts::PI].into_iter()),
+            V3ValueType::Float64
+        );
+
+        // Empty iterator
+        assert_eq!(value_type_for_values(std::iter::empty()), V3ValueType::Zero);
     }
 }

lib/saluki-components/src/encoders/datadog/metrics/v3/writer.rs

@@ -7,7 +7,7 @@ use protobuf::CodedOutputStream;
 use saluki_error::GenericError;
 
 use super::interner::Interner;
-use super::types::{field_numbers, V3MetricType, V3ValueType};
+use super::types::{field_numbers, value_type_for_values, V3MetricType, V3ValueType};
 
 const METRIC_TYPE_DEFAULT: i32 = 0;
 const METRIC_TYPE_AGENT_HIDDEN: i32 = 9;
@@ -474,13 +474,8 @@ impl<'a> V3MetricBuilder<'a> {
         let start = self.point_start_idx;
         let end = self.writer.vals_float64.len();
 
-        // Determine the maximum value type needed
-        let mut val_ty = V3ValueType::Zero;
-        for i in start..end {
-            let val = self.writer.vals_float64[i];
-            let pnt_val_ty = V3ValueType::for_value(val);
-            val_ty = val_ty.max(pnt_val_ty);
-        }
+        // Determine the best value type for all points in this metric.
+        let val_ty = value_type_for_values(self.writer.vals_float64[start..end].iter().copied());
 
         // Update the type field
         self.writer.types[self.metric_idx] |= val_ty.as_u64();
@@ -676,6 +671,50 @@ mod tests {
         assert!(output.is_empty());
     }
 
+    #[test]
+    fn test_value_compaction_large_int_plus_float32() {
+        // Regression test: a large integer (> 2^24) mixed with a fractional
+        // float32 value must use Float64, not Float32, to avoid precision loss.
+        let mut writer = V3Writer::new();
+
+        {
+            let mut metric = writer.write(V3MetricType::Gauge, "mixed.metric");
+            metric.add_point(1000, (1i64 << 30) as f64); // large int, doesn't fit in f32
+            metric.add_point(2000, 1.5); // fractional, fits in f32
+            metric.close();
+        }
+
+        let data = writer.finalize_inner();
+
+        // Must be stored in float64, not float32
+        assert!(
+            data.vals_float32.is_empty(),
+            "large int should not be stored as float32"
+        );
+        assert_eq!(data.vals_float64, vec![(1i64 << 30) as f64, 1.5]);
+        assert!(data.vals_sint64.is_empty());
+    }
+
+    #[test]
+    fn test_value_compaction_small_int_plus_float32() {
+        // Small integers (|v| <= 2^24) mixed with float32 values should
+        // compact to Float32, since small ints fit losslessly in f32.
+        let mut writer = V3Writer::new();
+
+        {
+            let mut metric = writer.write(V3MetricType::Gauge, "small.mixed");
+            metric.add_point(1000, 100.0);
+            metric.add_point(2000, 1.5);
+            metric.close();
+        }
+
+        let data = writer.finalize_inner();
+
+        assert!(data.vals_float64.is_empty());
+        assert_eq!(data.vals_float32, vec![100.0, 1.5]);
+        assert!(data.vals_sint64.is_empty());
+    }
+
     #[test]
     fn test_serialize_basic_metric() {
         let mut writer = V3Writer::new();