alamb commented on code in PR #11279:
URL: https://github.com/apache/arrow-rs/pull/11279#discussion_r4157745428
##########
arrow-select/src/interleave.rs:
##########
@@ -125,6 +128,89 @@ pub fn interleave(
}
}
+/// Interleaves byte view arrays into independently owned, compact buffers.
Review Comment:
In general having more flexible / options when calling kernels seems like a
good idea to me. However, I think the current free function approach gets out
of hand quickly. If we want to add more options to a kernel I would prefer we
switch to a more struct based approach -- something like
```rust
let interleaver = Interleaver::new()
.with_compact_byte_views(true);
// instead of interleave_byte_view_compact
let array = interleaver.interleave(input);
```
On the other hand, maintaining the free functions let downstream crates pick
which kernels they want to use (if we go with a struct the byte view
interleaving will be included in all downstream crates even if they don't use
it 🤔 )
##########
arrow-select/src/interleave.rs:
##########
@@ -125,6 +128,89 @@ pub fn interleave(
}
}
+/// Interleaves byte view arrays into independently owned, compact buffers.
+///
+/// Each pair in `indices` selects an array in `values` and a row in that
array,
+/// as in [`interleave`]. This supports both [`StringViewArray`] and
+/// [`BinaryViewArray`]. Only selected, non-null values are copied; inline
values
+/// need no data buffer. The result does not retain any input buffers.
+///
+/// Use this when selected rows must release their references to input storage,
+/// for example when partitioning a large batch for spilling. Unlike calling
+/// [`interleave`] followed by [`GenericByteViewArray::gc`], this does not
first
+/// construct an intermediate array referencing the input data buffers.
+///
+/// Copying can increase total memory usage while the inputs remain alive.
+/// Repeated selections copy their payload each time; values are not
deduplicated.
+/// Call [`interleave`] to share input data buffers instead.
+///
+/// # Errors
+///
+/// Returns an error if `values` is empty or the selected payload exceeds the
+/// supported size limits.
+///
+/// # Panics
+///
+/// Panics if an array or row index is out of bounds.
+///
+/// # Example
+///
+/// ```
+/// use arrow_array::StringViewArray;
+/// use arrow_select::interleave::interleave_byte_view_compact;
+///
+/// let a = StringViewArray::from(vec![Some("a long selected value"), None]);
+/// let b = StringViewArray::from(vec!["another selected value"]);
+/// let result = interleave_byte_view_compact(&[&a, &b], &[(1, 0), (0, 1), (0,
0)])?;
+/// assert_eq!(result, StringViewArray::from(vec![
+/// Some("another selected value"), None, Some("a long selected value")
+/// ]));
+/// # Ok::<(), arrow_schema::ArrowError>(())
+/// ```
+pub fn interleave_byte_view_compact<T: ByteViewType>(
Review Comment:
This feels like it may be similar to
https://docs.rs/arrow/latest/arrow/compute/struct.BatchCoalescer.html#method.push_batch_with_indices
-- which also does a bunch of work to copy string views only when needed.
The API isn't quite the same, but we could maybe add some sort of
interleaving to the coalescer 🤔
--
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]
