⚡ Bolt: Optimize Value::integer() and Value::float() conversions

.jules/bolt.md

@@ -5,3 +5,7 @@
 ## 2024-05-25 - Avoid double lookup and lock contention in Context lookup
 **Learning:** `HashMap::get(name)` followed by `.unwrap()` inside a Mutex lock not only does double lookup but keeps the lock longer than necessary when executing an inner function or cloning a value.
 **Action:** Use a single lookup, clone the value or arc into a local variable (`Option<ContextValue>`), and release the lock immediately before execution.
+
+## 2024-05-26 - Optimize decimal conversion in Value
+**Learning:** `rust_decimal::Decimal` allows efficient and direct conversion to basic types like `i64` and `f64` via `to_i64` and `to_f64` using the `rust_decimal::prelude::ToPrimitive` trait. Converting to string and then parsing `val.to_string().parse()` is an anti-pattern as it incurs heap allocation overhead, which is bad for performance. When doing integer conversions from `Decimal`, it's critical to check `val.scale() == 0` first to maintain behavioral parity with string parsing (which would reject floats).
+**Action:** Always favor direct conversion traits like `rust_decimal::prelude::ToPrimitive` methods over stringification when converting `Decimal` values to native numeric types. Keep edge cases like `scale()` properties in mind when changing conversion methods.

src/value.rs

@@ -96,20 +96,20 @@ impl Value {
 
     pub fn integer(self) -> Result<i64> {
         match self {
-            Self::Number(val) => val
-                .to_string()
-                .parse()
-                .map_or(Err(Error::InvalidInteger), |num| Ok(num)),
+            Self::Number(val) => {
+                if val.scale() == 0 {
+                    val.to_i64().ok_or(Error::InvalidInteger)
+                } else {
+                    Err(Error::InvalidInteger)
+                }
+            }
             _ => Err(Error::InvalidInteger),
         }
     }
 
     pub fn float(self) -> Result<f64> {
         match self {
-            Self::Number(val) => val
-                .to_string()
-                .parse()
-                .map_or(Err(Error::InvalidFloat), |num| Ok(num)),
+            Self::Number(val) => val.to_f64().ok_or(Error::InvalidFloat),
             _ => Err(Error::InvalidFloat),
         }
     }
@@ -148,3 +148,35 @@ impl_value_from_for_number!(
     [f64, from_f64],
     [f32, from_f32]
 );
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_value_integer() {
+        assert_eq!(Value::from(10).integer().unwrap(), 10);
+        assert_eq!(Value::from(-5).integer().unwrap(), -5);
+
+        // Test that integer() fails when scale is not 0
+        let dec = Decimal::from_str("10.5").unwrap();
+        assert!(Value::Number(dec).integer().is_err());
+
+        // Even if the value represents an integer, but has a non-zero scale, it should fail to parse
+        // according to strict behavior checking `val.scale() == 0`
+        let dec_with_scale = Decimal::from_str("10.0").unwrap();
+        assert!(Value::Number(dec_with_scale).integer().is_err());
+    }
+
+    #[test]
+    fn test_value_float() {
+        assert_eq!(Value::from(10).float().unwrap(), 10.0);
+        assert_eq!(Value::from(-5).float().unwrap(), -5.0);
+
+        let dec = Decimal::from_str("10.5").unwrap();
+        assert_eq!(Value::Number(dec).float().unwrap(), 10.5);
+
+        let dec_with_scale = Decimal::from_str("10.0").unwrap();
+        assert_eq!(Value::Number(dec_with_scale).float().unwrap(), 10.0);
+    }
+}