[GitHub] [arrow-rs] alamb commented on a diff in pull request #2000: Add Decimal256Builder and Decimal256Array

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

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


##########
arrow/src/array/builder/decimal_builder.rs:
##########
@@ -154,10 +166,59 @@ impl ArrayBuilder for DecimalBuilder {
     }
 }
 
+impl Decimal256Builder {
+    /// Creates a new `Decimal256Builder`, `capacity` is the number of bytes in the values
+    /// array
+    pub fn new(capacity: usize, precision: usize, scale: usize) -> Self {
+        let values_builder = UInt8Builder::new(capacity);
+        let byte_width = 32;
+        Self {
+            builder: FixedSizeListBuilder::new(values_builder, byte_width),
+            precision,
+            scale,
+        }
+    }
+
+    /// Appends a byte slice into the builder.
+    ///
+    /// Automatically calls the `append` method to delimit the slice appended in as a
+    /// distinct array element.
+    #[inline]
+    pub fn append_value(&mut self, value: &Decimal256) -> Result<()> {
+        let value_as_bytes = value.raw_value();
+
+        if self.builder.value_length() != value_as_bytes.len() as i32 {

Review Comment:
   I wonder if we also need to check the `precision` and `scale` of `value` (and either if they don't match the builder or do conversion if required) 🤔 



##########
arrow/src/array/builder/decimal_builder.rs:
##########
@@ -197,4 +258,42 @@ mod tests {
         assert_eq!(32, decimal_array.value_offset(2));
         assert_eq!(16, decimal_array.value_length());
     }
+
+    #[test]
+    fn test_decimal256_builder() {
+        let mut builder = Decimal256Builder::new(30, 40, 6);
+
+        let mut bytes = vec![0; 32];
+        bytes[0..16].clone_from_slice(&8_887_000_000_i128.to_le_bytes());
+        let value = Decimal256::try_new_from_bytes(40, 6, bytes.as_slice()).unwrap();
+        builder.append_value(&value).unwrap();
+
+        builder.append_null().unwrap();
+
+        bytes = vec![255; 32];
+        let value = Decimal256::try_new_from_bytes(40, 6, bytes.as_slice()).unwrap();
+        builder.append_value(&value).unwrap();
+
+        bytes = vec![0; 32];
+        bytes[0..16].clone_from_slice(&0_i128.to_le_bytes());
+        bytes[15] = 128;
+        let value = Decimal256::try_new_from_bytes(40, 6, bytes.as_slice()).unwrap();

Review Comment:
   I think we should add a test for appending a `value` that has a precision/scale other than `40. 6`, as I mentioned above



-- 
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