DecimalBuilder should use FixedSizeBinaryBuilder (#2092)

* decimal builder uses fixed binary builder Signed-off-by: remzi <13716567376yh@gmail.com> * deprecate from_fixed_size_binary_array Signed-off-by: remzi <13716567376yh@gmail.com>
apache · Jul 18, 2022 · 53e9388 · 53e9388
1 parent 02371d2
commit 53e9388
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 43 deletions.
diff --git a/arrow/src/array/array_decimal.rs b/arrow/src/array/array_decimal.rs
@@ -172,6 +172,11 @@ pub trait BasicDecimalArray<T: BasicDecimal, U: From<ArrayData>>:
         U::from(array_data)
     }
 
+    /// Build a decimal array from [`FixedSizeListArray`].
+    ///
+    /// NB: This function does not validate that each value is in the permissible
+    /// range for a decimal. And, the null buffer of the child array will be ignored.
+    #[deprecated(note = "please use `from_fixed_size_binary_array` instead")]
     fn from_fixed_size_list_array(
         v: FixedSizeListArray,
         precision: usize,
@@ -707,6 +712,7 @@ mod tests {
     }
 
     #[test]
+    #[allow(deprecated)]
     fn test_decimal_array_from_fixed_size_list() {
         let value_data = ArrayData::builder(DataType::UInt8)
             .offset(16)

diff --git a/arrow/src/array/builder/decimal_builder.rs b/arrow/src/array/builder/decimal_builder.rs
@@ -21,8 +21,7 @@ use std::sync::Arc;
 use crate::array::array_decimal::{BasicDecimalArray, Decimal256Array};
 use crate::array::ArrayRef;
 use crate::array::DecimalArray;
-use crate::array::UInt8Builder;
-use crate::array::{ArrayBuilder, FixedSizeListBuilder};
+use crate::array::{ArrayBuilder, FixedSizeBinaryBuilder};
 
 use crate::error::{ArrowError, Result};
 
@@ -35,7 +34,7 @@ use crate::util::decimal::{BasicDecimal, Decimal256};
 ///
 #[derive(Debug)]
 pub struct DecimalBuilder {
-    builder: FixedSizeListBuilder<UInt8Builder>,
+    builder: FixedSizeBinaryBuilder,
     precision: usize,
     scale: usize,
 
@@ -49,19 +48,18 @@ pub struct DecimalBuilder {
 /// See [`Decimal256Array`] for example.
 #[derive(Debug)]
 pub struct Decimal256Builder {
-    builder: FixedSizeListBuilder<UInt8Builder>,
+    builder: FixedSizeBinaryBuilder,
     precision: usize,
     scale: usize,
 }
 
 impl DecimalBuilder {
-    /// Creates a new `DecimalBuilder`, `capacity` is the number of bytes in the values
+    const BYTE_LENGTH: i32 = 16;
+    /// Creates a new [`DecimalBuilder`], `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 = 16;
         Self {
-            builder: FixedSizeListBuilder::new(values_builder, byte_width),
+            builder: FixedSizeBinaryBuilder::new(capacity, Self::BYTE_LENGTH),
             precision,
             scale,
             value_validation: true,
@@ -78,10 +76,7 @@ impl DecimalBuilder {
         self.value_validation = false;
     }
 
-    /// Appends a byte slice into the builder.
-    ///
-    /// Automatically calls the `append` method to delimit the slice appended in as a
-    /// distinct array element.
+    /// Appends a decimal value into the builder.
     #[inline]
     pub fn append_value(&mut self, value: impl Into<i128>) -> Result<()> {
         let value = if self.value_validation {
@@ -90,19 +85,14 @@ impl DecimalBuilder {
             value.into()
         };
 
-        let value_as_bytes = Self::from_i128_to_fixed_size_bytes(
-            value,
-            self.builder.value_length() as usize,
-        )?;
-        if self.builder.value_length() != value_as_bytes.len() as i32 {
+        let value_as_bytes =
+            Self::from_i128_to_fixed_size_bytes(value, Self::BYTE_LENGTH as usize)?;
+        if Self::BYTE_LENGTH != value_as_bytes.len() as i32 {
             return Err(ArrowError::InvalidArgumentError(
                 "Byte slice does not have the same length as DecimalBuilder value lengths".to_string()
             ));
         }
-        self.builder
-            .values()
-            .append_slice(value_as_bytes.as_slice())?;
-        self.builder.append(true)
+        self.builder.append_value(value_as_bytes.as_slice())
     }
 
     pub(crate) fn from_i128_to_fixed_size_bytes(v: i128, size: usize) -> Result<Vec<u8>> {
@@ -119,14 +109,12 @@ impl DecimalBuilder {
     /// Append a null value to the array.
     #[inline]
     pub fn append_null(&mut self) -> Result<()> {
-        let length: usize = self.builder.value_length() as usize;
-        self.builder.values().append_slice(&vec![0u8; length][..])?;
-        self.builder.append(false)
+        self.builder.append_null()
     }
 
     /// Builds the `DecimalArray` and reset this builder.
     pub fn finish(&mut self) -> DecimalArray {
-        DecimalArray::from_fixed_size_list_array(
+        DecimalArray::from_fixed_size_binary_array(
             self.builder.finish(),
             self.precision,
             self.scale,
@@ -167,52 +155,45 @@ impl ArrayBuilder for DecimalBuilder {
 }
 
 impl Decimal256Builder {
-    /// Creates a new `Decimal256Builder`, `capacity` is the number of bytes in the values
+    const BYTE_LENGTH: i32 = 32;
+    /// 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),
+            builder: FixedSizeBinaryBuilder::new(capacity, Self::BYTE_LENGTH),
             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.
+    /// Appends a [`Decimal256`] number into the builder.
     #[inline]
     pub fn append_value(&mut self, value: &Decimal256) -> Result<()> {
-        let value_as_bytes = value.raw_value();
-
         if self.precision != value.precision() || self.scale != value.scale() {
             return Err(ArrowError::InvalidArgumentError(
                 "Decimal value does not have the same precision or scale as Decimal256Builder".to_string()
             ));
         }
 
-        if self.builder.value_length() != value_as_bytes.len() as i32 {
+        let value_as_bytes = value.raw_value();
+
+        if Self::BYTE_LENGTH != value_as_bytes.len() as i32 {
             return Err(ArrowError::InvalidArgumentError(
                 "Byte slice does not have the same length as Decimal256Builder value lengths".to_string()
             ));
         }
-        self.builder.values().append_slice(value_as_bytes)?;
-        self.builder.append(true)
+        self.builder.append_value(value_as_bytes)
     }
 
     /// Append a null value to the array.
     #[inline]
     pub fn append_null(&mut self) -> Result<()> {
-        let length: usize = self.builder.value_length() as usize;
-        self.builder.values().append_slice(&vec![0u8; length][..])?;
-        self.builder.append(false)
+        self.builder.append_null()
     }
 
-    /// Builds the `Decimal256Array` and reset this builder.
+    /// Builds the [`Decimal256Array`] and reset this builder.
     pub fn finish(&mut self) -> Decimal256Array {
-        Decimal256Array::from_fixed_size_list_array(
+        Decimal256Array::from_fixed_size_binary_array(
             self.builder.finish(),
             self.precision,
             self.scale,