[arrow-array] Use consistent value_length name in FixedSizeBinaryArray (apache#9905)

# Which issue does this PR close?
- Part of apache#9906
- First follow on to apache#9872

# Rationale for this change

While trying to avoid overflows due to using i32 arithmetic in
FixedSizeBinaryArray, I found the use of the term `size` in parameters
to be confusing when the field name is called `value_length`

# What changes are included in this PR?

Change several parameter / variable names to `value_length` to keep the
code consistent

# Are these changes tested?

By CI

# Are there any user-facing changes?

No this is an internal code refactor

Author: alamb

arrow-array/src/array/fixed_size_binary_array.rs

@@ -96,44 +96,48 @@ pub struct FixedSizeBinaryArray {
 }
 
 impl FixedSizeBinaryArray {
-    /// Create a new [`FixedSizeBinaryArray`] with `size` element size, panicking on failure
+    /// Create a new [`FixedSizeBinaryArray`] with `value_length` bytes per element, panicking on
+    /// failure
     ///
     /// # Panics
     ///
     /// Panics if [`Self::try_new`] returns an error
-    pub fn new(size: i32, values: Buffer, nulls: Option<NullBuffer>) -> Self {
-        Self::try_new(size, values, nulls).unwrap()
+    pub fn new(value_length: i32, values: Buffer, nulls: Option<NullBuffer>) -> Self {
+        Self::try_new(value_length, values, nulls).unwrap()
     }
 
     /// Create a new [`Scalar`] from `value`
     pub fn new_scalar(value: impl AsRef<[u8]>) -> Scalar<Self> {
         let v = value.as_ref();
-        let size = i32::try_from(v.len()).expect("FixedSizeBinaryArray value length exceeds i32");
-        Scalar::new(Self::new(size, Buffer::from(v), None))
+        let value_length =
+            i32::try_from(v.len()).expect("FixedSizeBinaryArray value length exceeds i32");
+        Scalar::new(Self::new(value_length, Buffer::from(v), None))
     }
 
     /// Create a new [`FixedSizeBinaryArray`] from the provided parts, returning an error on failure
     ///
-    /// Creating an arrow with `size == 0` will try to get the length from the null buffer. If
-    /// no null buffer is provided, the resulting array will have length zero.
+    /// Creating an array with `value_length == 0` will try to get the length from the null
+    /// buffer. If no null buffer is provided, the resulting array will have length zero.
     ///
     /// # Errors
     ///
-    /// * `size < 0`
-    /// * `values.len() / size != nulls.len()`
-    /// * `size == 0 && values.len() != 0`
-    /// * `len * size > i32::MAX`
+    /// * `value_length < 0`
+    /// * `values.len() / value_length != nulls.len()`
+    /// * `value_length == 0 && values.len() != 0`
+    /// * `len * value_length > i32::MAX`
     pub fn try_new(
-        size: i32,
+        value_length: i32,
         values: Buffer,
         nulls: Option<NullBuffer>,
     ) -> Result<Self, ArrowError> {
-        let data_type = DataType::FixedSizeBinary(size);
-        let s = size.to_usize().ok_or_else(|| {
-            ArrowError::InvalidArgumentError(format!("Size cannot be negative, got {size}"))
+        let data_type = DataType::FixedSizeBinary(value_length);
+        let value_size = value_length.to_usize().ok_or_else(|| {
+            ArrowError::InvalidArgumentError(format!(
+                "Value length cannot be negative, got {value_length}"
+            ))
         })?;
 
-        let len = match values.len().checked_div(s) {
+        let len = match values.len().checked_div(value_size) {
             Some(len) => {
                 if let Some(n) = nulls.as_ref() {
                     if n.len() != len {
@@ -150,21 +154,21 @@ impl FixedSizeBinaryArray {
             None => {
                 if !values.is_empty() {
                     return Err(ArrowError::InvalidArgumentError(
-                        "Buffer cannot have non-zero length if the item size is zero".to_owned(),
+                        "Buffer cannot have non-zero length if the value length is zero".to_owned(),
                     ));
                 }
 
-                // If the item size is zero, try to determine the length from the null buffer
+                // If the value length is zero, try to determine the length from the null buffer
                 nulls.as_ref().map(|n| n.len()).unwrap_or(0)
             }
         };
 
-        Self::validate_lengths(s, len)?;
+        Self::validate_lengths(value_size, len)?;
 
         Ok(Self {
             data_type,
             value_data: values,
-            value_length: size,
+            value_length,
             nulls,
             len,
         })
@@ -203,21 +207,21 @@ impl FixedSizeBinaryArray {
     ///
     /// Panics if
     ///
-    /// * `size < 0`
-    /// * `size * len` would overflow `usize`
-    /// * `size * len > i32::MAX`
-    /// * `size * len * 8` would overflow `usize`
-    pub fn new_null(size: i32, len: usize) -> Self {
+    /// * `value_length < 0`
+    /// * `value_length * len` would overflow `usize`
+    /// * `value_length * len > i32::MAX`
+    /// * `value_length * len * 8` would overflow `usize`
+    pub fn new_null(value_length: i32, len: usize) -> Self {
         const BITS_IN_A_BYTE: usize = 8;
-        let size_usize = size.to_usize().unwrap();
-        Self::validate_lengths(size_usize, len).unwrap();
-        let capacity_in_bytes = size_usize.checked_mul(len).unwrap();
+        let value_size = value_length.to_usize().unwrap();
+        Self::validate_lengths(value_size, len).unwrap();
+        let capacity_in_bytes = value_size.checked_mul(len).unwrap();
         let capacity_in_bits = capacity_in_bytes.checked_mul(BITS_IN_A_BYTE).unwrap();
         Self {
-            data_type: DataType::FixedSizeBinary(size),
+            data_type: DataType::FixedSizeBinary(value_length),
             value_data: MutableBuffer::new_null(capacity_in_bits).into(),
             nulls: Some(NullBuffer::new_null(len)),
-            value_length: size,
+            value_length,
             len,
         }
     }
@@ -351,7 +355,7 @@ impl FixedSizeBinaryArray {
         U: AsRef<[u8]>,
     {
         let mut len = 0;
-        let mut size = None;
+        let mut value_size = None;
         let mut byte = 0;
 
         let iter_size_hint = iter.size_hint().0;
@@ -369,7 +373,7 @@ impl FixedSizeBinaryArray {
 
             if let Some(slice) = item {
                 let slice = slice.as_ref();
-                if let Some(size) = size {
+                if let Some(size) = value_size {
                     if size != slice.len() {
                         return Err(ArrowError::InvalidArgumentError(format!(
                             "Nested array size mismatch: one is {}, and the other is {}",
@@ -379,7 +383,7 @@ impl FixedSizeBinaryArray {
                     }
                 } else {
                     let len = slice.len();
-                    size = Some(len);
+                    value_size = Some(len);
                     // Now that we know how large each element is we can reserve
                     // sufficient capacity in the underlying mutable buffer for
                     // the data.
@@ -396,7 +400,7 @@ impl FixedSizeBinaryArray {
                 }
                 bit_util::set_bit(null_buf.as_slice_mut(), len);
                 buffer.extend_from_slice(slice);
-            } else if let Some(size) = size {
+            } else if let Some(size) = value_size {
                 buffer.extend_zeros(size);
             } else {
                 prepend += 1;
@@ -415,27 +419,27 @@ impl FixedSizeBinaryArray {
 
         let nulls = NullBuffer::from_unsliced_buffer(null_buf, len);
 
-        let size = size.unwrap_or(0);
-        Self::validate_lengths(size, len)?;
-        let size = size.try_into().map_err(|_| {
+        let value_size = value_size.unwrap_or(0);
+        Self::validate_lengths(value_size, len)?;
+        let value_length = value_size.try_into().map_err(|_| {
             ArrowError::InvalidArgumentError(format!(
-                "FixedSizeBinaryArray value length exceeds i32, got {size}"
+                "FixedSizeBinaryArray value length exceeds i32, got {value_size}"
             ))
         })?;
         Ok(Self {
-            data_type: DataType::FixedSizeBinary(size),
+            data_type: DataType::FixedSizeBinary(value_length),
             value_data: buffer.into(),
             nulls,
-            value_length: size,
+            value_length,
             len,
         })
     }
 
     /// Create an array from an iterable argument of sparse byte slices.
     /// Sparsity means that items returned by the iterator are optional, i.e input argument can
     /// contain `None` items. In cases where the iterator returns only `None` values, this
-    /// also takes a size parameter to ensure that the a valid FixedSizeBinaryArray is still
-    /// created.
+    /// also takes a `value_length` parameter to ensure that a valid
+    /// [`FixedSizeBinaryArray`] is still created.
     ///
     /// # Examples
     ///
@@ -455,22 +459,27 @@ impl FixedSizeBinaryArray {
     /// # Errors
     ///
     /// Returns error if argument has length zero, or sizes of nested slices don't match.
-    pub fn try_from_sparse_iter_with_size<T, U>(mut iter: T, size: i32) -> Result<Self, ArrowError>
+    pub fn try_from_sparse_iter_with_size<T, U>(
+        mut iter: T,
+        value_length: i32,
+    ) -> Result<Self, ArrowError>
     where
         T: Iterator<Item = Option<U>>,
         U: AsRef<[u8]>,
     {
-        let size_usize = size.to_usize().ok_or_else(|| {
-            ArrowError::InvalidArgumentError(format!("Size cannot be negative, got {size}"))
+        let value_size = value_length.to_usize().ok_or_else(|| {
+            ArrowError::InvalidArgumentError(format!(
+                "Value length cannot be negative, got {value_length}"
+            ))
         })?;
         let mut len = 0;
         let mut byte = 0;
 
         let iter_size_hint = iter.size_hint().0;
         let mut null_buf = MutableBuffer::new(bit_util::ceil(iter_size_hint, 8));
-        let capacity = iter_size_hint.checked_mul(size_usize).ok_or_else(|| {
+        let capacity = iter_size_hint.checked_mul(value_size).ok_or_else(|| {
             ArrowError::InvalidArgumentError(format!(
-                "FixedSizeBinaryArray error: value size {size_usize} * len hint {iter_size_hint} exceeds usize"
+                "FixedSizeBinaryArray error: value size {value_size} * len hint {iter_size_hint} exceeds usize"
             ))
         })?;
         let mut buffer = MutableBuffer::new(capacity);
@@ -485,18 +494,18 @@ impl FixedSizeBinaryArray {
 
             if let Some(slice) = item {
                 let slice = slice.as_ref();
-                if size_usize != slice.len() {
+                if value_size != slice.len() {
                     return Err(ArrowError::InvalidArgumentError(format!(
                         "Nested array size mismatch: one is {}, and the other is {}",
-                        size,
+                        value_length,
                         slice.len()
                     )));
                 }
 
                 bit_util::set_bit(null_buf.as_slice_mut(), len);
                 buffer.extend_from_slice(slice);
             } else {
-                buffer.extend_zeros(size_usize);
+                buffer.extend_zeros(value_size);
             }
 
             len += 1;
@@ -505,14 +514,14 @@ impl FixedSizeBinaryArray {
         })?;
 
         let nulls = NullBuffer::from_unsliced_buffer(null_buf, len);
-        Self::validate_lengths(size_usize, len)?;
+        Self::validate_lengths(value_size, len)?;
 
         Ok(Self {
-            data_type: DataType::FixedSizeBinary(size),
+            data_type: DataType::FixedSizeBinary(value_length),
             value_data: buffer.into(),
             nulls,
             len,
-            value_length: size,
+            value_length,
         })
     }
 
@@ -539,23 +548,22 @@ impl FixedSizeBinaryArray {
         U: AsRef<[u8]>,
     {
         let mut len = 0;
-        let mut size = None;
+        let mut value_size = None;
         let iter_size_hint = iter.size_hint().0;
         let mut buffer = MutableBuffer::new(0);
 
         iter.try_for_each(|item| -> Result<(), ArrowError> {
             let slice = item.as_ref();
-            if let Some(size) = size {
-                if size != slice.len() {
+            if let Some(value_size) = value_size {
+                if value_size != slice.len() {
                     return Err(ArrowError::InvalidArgumentError(format!(
-                        "Nested array size mismatch: one is {}, and the other is {}",
-                        size,
+                        "Nested array size mismatch: one is {value_size}, and the other is {}",
                         slice.len()
                     )));
                 }
             } else {
                 let len = slice.len();
-                size = Some(len);
+                value_size = Some(len);
                 if let Some(capacity) = iter_size_hint.checked_mul(len) {
                     buffer.reserve(capacity);
                 }
@@ -574,18 +582,18 @@ impl FixedSizeBinaryArray {
             ));
         }
 
-        let size = size.unwrap_or(0);
-        Self::validate_lengths(size, len)?;
-        let size = size.try_into().map_err(|_| {
+        let value_size = value_size.unwrap_or(0);
+        Self::validate_lengths(value_size, len)?;
+        let value_length = value_size.try_into().map_err(|_| {
             ArrowError::InvalidArgumentError(format!(
-                "FixedSizeBinaryArray value length exceeds i32, got {size}"
+                "FixedSizeBinaryArray value length exceeds i32, got {value_size}"
             ))
         })?;
         Ok(Self {
-            data_type: DataType::FixedSizeBinary(size),
+            data_type: DataType::FixedSizeBinary(value_length),
             value_data: buffer.into(),
             nulls: None,
-            value_length: size,
+            value_length,
             len,
         })
     }
@@ -615,14 +623,14 @@ impl From<ArrayData> for FixedSizeBinaryArray {
             _ => panic!("Expected data type to be FixedSizeBinary"),
         };
 
-        let size = value_length
+        let value_size = value_length
             .to_usize()
             .expect("FixedSizeBinaryArray value length must be non-negative");
-        Self::validate_lengths(size, len)
+        Self::validate_lengths(value_size, len)
             .expect("FixedSizeBinaryArray offsets must fit within i32");
         let value_data = buffers[0].slice_with_length(
-            offset.checked_mul(size).expect("offset overflow"),
-            len.checked_mul(size).expect("length overflow"),
+            offset.checked_mul(value_size).expect("offset overflow"),
+            len.checked_mul(value_size).expect("length overflow"),
         );
 
         Self {
@@ -1157,7 +1165,7 @@ mod tests {
 
         assert_eq!(
             err.to_string(),
-            "Invalid argument error: Size cannot be negative, got -1"
+            "Invalid argument error: Value length cannot be negative, got -1"
         );
 
         let nulls = NullBuffer::new_null(3);
@@ -1182,7 +1190,7 @@ mod tests {
             FixedSizeBinaryArray::try_new(0, buffer, None).unwrap_err();
         assert_eq!(
             zero_sized_with_non_empty_buffer_err.to_string(),
-            "Invalid argument error: Buffer cannot have non-zero length if the item size is zero"
+            "Invalid argument error: Buffer cannot have non-zero length if the value length is zero"
         );
     }
 }