theirix commented on code in PR #9876:
URL: https://github.com/apache/arrow-rs/pull/9876#discussion_r3191625020
##########
arrow-string/src/concat_elements.rs:
##########
@@ -168,6 +168,85 @@ pub fn concat_elements_utf8_many<Offset: OffsetSizeTrait>(
Ok(unsafe { builder.build_unchecked() }.into())
}
+/// Returns the elementwise concatenation of a [`FixedSizeBinaryArray`].
+///
+/// The result has `value_length = left.value_length() + right.value_length()`.
+/// An index is null if either input is null at that position.
+///
+/// An error will be returned if `left` and `right` have different lengths.
+pub fn concat_elements_fixed_size_binary(
+ left: &FixedSizeBinaryArray,
+ right: &FixedSizeBinaryArray,
+) -> Result<FixedSizeBinaryArray, ArrowError> {
+ if left.len() != right.len() {
+ return Err(ArrowError::ComputeError(format!(
+ "Arrays must have the same length: {} != {}",
+ left.len(),
+ right.len()
+ )));
+ }
+
+ let left_size = left.value_length() as usize;
+ let right_size = right.value_length() as usize;
+ let output_size = left_size + right_size;
+
+ // Pre-compute combined null bitmap so the per-row NULL check is efficient
+ let nulls = NullBuffer::union(left.nulls(), right.nulls());
+
+ let mut result = FixedSizeBinaryBuilder::with_capacity(left.len(),
output_size as i32);
+ let mut buffer = MutableBuffer::with_capacity(output_size);
Review Comment:
It's a good idea. Let's introduce the functionality first, then the benches,
and then try interleaving - so the benches and the CI help review the effect.
##########
arrow-string/src/concat_elements.rs:
##########
@@ -168,6 +168,85 @@ pub fn concat_elements_utf8_many<Offset: OffsetSizeTrait>(
Ok(unsafe { builder.build_unchecked() }.into())
}
+/// Returns the elementwise concatenation of a [`FixedSizeBinaryArray`].
+///
+/// The result has `value_length = left.value_length() + right.value_length()`.
+/// An index is null if either input is null at that position.
+///
+/// An error will be returned if `left` and `right` have different lengths.
+pub fn concat_elements_fixed_size_binary(
+ left: &FixedSizeBinaryArray,
+ right: &FixedSizeBinaryArray,
+) -> Result<FixedSizeBinaryArray, ArrowError> {
+ if left.len() != right.len() {
+ return Err(ArrowError::ComputeError(format!(
+ "Arrays must have the same length: {} != {}",
+ left.len(),
+ right.len()
+ )));
+ }
+
+ let left_size = left.value_length() as usize;
+ let right_size = right.value_length() as usize;
+ let output_size = left_size + right_size;
+
+ // Pre-compute combined null bitmap so the per-row NULL check is efficient
+ let nulls = NullBuffer::union(left.nulls(), right.nulls());
+
+ let mut result = FixedSizeBinaryBuilder::with_capacity(left.len(),
output_size as i32);
+ let mut buffer = MutableBuffer::with_capacity(output_size);
+ for i in 0..left.len() {
+ if nulls.as_ref().is_some_and(|n| n.is_null(i)) {
+ result.append_null();
+ } else {
+ buffer.clear();
+ buffer.extend_from_slice(left.value(i));
+ buffer.extend_from_slice(right.value(i));
+ result.append_value(&buffer)?;
+ }
+ }
+
+ Ok(result.finish())
+}
+
+/// Concatenates two `BinaryViewArray`s element-wise.
+/// If either element is `Null`, the result element is also `Null`.
+///
+/// # Errors
+/// - Returns an error if the input arrays have different lengths.
+/// - Returns an error if any concatenated string exceeds `u32::MAX` in length.
+pub fn concat_elements_binary_view_array(
+ left: &BinaryViewArray,
+ right: &BinaryViewArray,
+) -> Result<BinaryViewArray, ArrowError> {
+ if left.len() != right.len() {
+ return Err(ArrowError::ComputeError(format!(
+ "Arrays must have the same length: {} != {}",
+ left.len(),
Review Comment:
Unfortunately, ByteViewType and GenericByteViewBuilder are not generic
enough to have only one generic function here because they operate over
`AsRef<T>`, not a binary slice.
I can propose extending that builder with something like
```rust
pub fn try_append_slice(mut self, value: &[u8])
```
to unblock generic usage.
So far, I have added a second function with close enough functionality.
##########
arrow-string/src/concat_elements.rs:
##########
@@ -355,6 +450,89 @@ mod tests {
assert_eq!(output, expected);
}
+ #[test]
+ fn test_fixed_size_binary_concat() {
Review Comment:
Added an empty test case. Different sizes have already been tested in
`test_fixed_size_binary_concat_error`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
