Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alamb merged 1 commit into from
May 5, 2026
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 79 additions & 71 deletions arrow-array/src/array/fixed_size_binary_array.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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,

@alamb alamb May 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is an example of inconsistent terminology size and value_length used for the same concept

After this PR, the same concept has the same name in both parameters and fields

nulls,
len,
})
Expand Down Expand Up @@ -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,
}
}
Expand Down Expand Up @@ -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;
Expand All @@ -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 {}",
Expand All @@ -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.
Expand All @@ -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;
Expand All @@ -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
///
Expand All @@ -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);
Expand All @@ -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;
Expand All @@ -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,
})
}

Expand All @@ -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);
}
Expand All @@ -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,
})
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand All @@ -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"
);
}
}
Loading