[GitHub] [arrow-rs] alamb commented on a diff in pull request #1914: Add Decimal256 API

[GitBox <gi...@apache.org>]

alamb commented on code in PR #1914:
URL: https://github.com/apache/arrow-rs/pull/1914#discussion_r903732256


##########
arrow/src/util/decimal.rs:
##########
@@ -17,109 +17,201 @@
 
 //! Decimal related utils
 
-use std::cmp::Ordering;
+use crate::error::{ArrowError, Result};
+use num::bigint::BigInt;
+use std::cmp::{min, Ordering};
+
+pub trait BasicDecimal: PartialOrd + Ord + PartialEq + Eq {
+    /// The bit-width of the internal representation.
+    const BIT_WIDTH: usize;
+
+    /// Tries to create a decimal value from precision, scale and bytes.
+    /// If the length of bytes isn't same as the bit width of this decimal,
+    /// returning an error. The bytes should be stored in little-endian order.
+    ///
+    /// Safety:
+    /// This method doesn't validate if the decimal value represented by the bytes
+    /// can be fitted into the specified precision.
+    fn try_new_from_bytes(precision: usize, scale: usize, bytes: &[u8]) -> Result<Self>
+    where
+        Self: Sized,
+    {
+        if precision < scale {
+            return Err(ArrowError::InvalidArgumentError(format!(
+                "Precision {} is less than scale {}",
+                precision, scale
+            )));
+        }
+
+        if bytes.len() == Self::BIT_WIDTH / 8 {
+            Ok(Self::new(precision, scale, bytes))
+        } else {
+            Err(ArrowError::InvalidArgumentError(format!(
+                "Input to Decimal{} must be {} bytes",
+                Self::BIT_WIDTH,
+                Self::BIT_WIDTH / 8
+            )))
+        }
+    }
+
+    /// Creates a decimal value from precision, scale, and bytes.
+    ///
+    /// Safety:
+    /// This method doesn't check if the length of bytes is compatible with this decimal.
+    /// Use `try_new_from_bytes` for safe constructor.
+    fn new(precision: usize, scale: usize, bytes: &[u8]) -> Self;
+
+    /// Returns the raw bytes of the integer representation of the decimal.
+    fn raw_value(&self) -> &[u8];
+
+    /// Returns the precision of the decimal.
+    fn precision(&self) -> usize;
+
+    /// Returns the scale of the decimal.
+    fn scale(&self) -> usize;
+
+    /// Returns the string representation of the decimal.
+    /// If the string representation cannot be fitted with the precision of the decimal,
+    /// the string will be truncated.
+    fn as_string(&self) -> String {

Review Comment:
   ```suggestion
       fn to_string(&self) -> String {
   ```
   
   I think `to` is a more rust idomatic name -- functions with `as_*` names are typically used for cheap conversions



##########
arrow/src/util/decimal.rs:
##########
@@ -131,10 +223,60 @@ mod tests {
     }
 
     #[test]
-    fn decimal_128_from_bytes() {
+    fn decimal_invalid_precision_scale() {
         let bytes = 100_i128.to_le_bytes();
-        let value = Decimal128::new_from_bytes(5, 2, &bytes);
+        let err = Decimal128::try_new_from_bytes(5, 6, &bytes);
+        assert!(err.is_err());
+    }
+
+    #[test]
+    fn decimal_128_from_bytes() {
+        let mut bytes = 100_i128.to_le_bytes();
+        let value = Decimal128::try_new_from_bytes(5, 2, &bytes).unwrap();
         assert_eq!(value.as_string(), "1.00");
+
+        bytes = (-1_i128).to_le_bytes();
+        let value = Decimal128::try_new_from_bytes(5, 2, &bytes).unwrap();
+        assert_eq!(value.as_string(), "-0.01");
+
+        bytes = i128::MAX.to_le_bytes();
+        let value = Decimal128::try_new_from_bytes(38, 2, &bytes).unwrap();
+        assert_eq!(value.as_string(), "170141183460469231731687303715884105.72");
+
+        bytes = i128::MIN.to_le_bytes();
+        let value = Decimal128::try_new_from_bytes(38, 2, &bytes).unwrap();
+        assert_eq!(
+            value.as_string(),
+            "-170141183460469231731687303715884105.72"
+        );
+
+        // Truncated
+        bytes = 12345_i128.to_le_bytes();
+        let value = Decimal128::try_new_from_bytes(3, 2, &bytes).unwrap();
+        assert_eq!(value.as_string(), "1.23");
+
+        bytes = (-12345_i128).to_le_bytes();
+        let value = Decimal128::try_new_from_bytes(3, 2, &bytes).unwrap();
+        assert_eq!(value.as_string(), "-1.23");
+    }
+
+    #[test]
+    fn decimal_256_from_bytes() {
+        let mut bytes = vec![0; 32];
+        bytes[0..16].clone_from_slice(&100_i128.to_le_bytes());
+        let value = Decimal256::try_new_from_bytes(5, 2, bytes.as_slice()).unwrap();
+        assert_eq!(value.as_string(), "1.00");
+
+        bytes[0..16].clone_from_slice(&i128::MAX.to_le_bytes());

Review Comment:
   it would be nice to check at least one value that is larger and one that is smaller than would fit in `i128` 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org