Copilot commented on code in PR #10025:
URL: https://github.com/apache/arrow-rs/pull/10025#discussion_r3311562817
##########
arrow-select/src/interleave.rs:
##########
@@ -373,13 +375,82 @@ fn interleave_struct(
Ok(Arc::new(struct_array))
}
+fn interleave_list_primitive_child<O: OffsetSizeTrait, T: ArrowPrimitiveType>(
+ interleaved: &Interleave<'_, GenericListArray<O>>,
+ indices: &[(usize, usize)],
+ capacity: usize,
+) -> ArrayRef {
+ let child_arrays: Vec<&PrimitiveArray<T>> = interleaved
+ .arrays
+ .iter()
+ .map(|list| list.values().as_primitive::<T>())
+ .collect();
+
+ let has_child_nulls = child_arrays.iter().any(|a| a.null_count() > 0);
+
+ // Build values buffer by copying contiguous slices
+ let mut values: Vec<T::Native> = Vec::with_capacity(capacity);
+ for &(array, row) in indices {
+ let o = interleaved.arrays[array].value_offsets();
+ let start = o[row].as_usize();
+ let end = o[row + 1].as_usize();
+ if end > start {
+
values.extend_from_slice(&child_arrays[array].values()[start..end]);
+ }
+ }
+
+ // Build null buffer. Pre-allocate with 0x00 (all null), then:
+ // - Sources with nulls: set_bits ORs in valid bits from source.
+ // - Sources without nulls: set the bit range to all 1s directly.
+ let nulls = if has_child_nulls {
+ let null_byte_len = bit_util::ceil(capacity, 8);
+ let mut null_buf = MutableBuffer::new(null_byte_len);
+ null_buf.resize(null_byte_len, 0);
+
+ let mut offset_write = 0;
+ for &(array, row) in indices {
+ let o = interleaved.arrays[array].value_offsets();
+ let start = o[row].as_usize();
+ let end = o[row + 1].as_usize();
+ let len = end - start;
+ if len > 0 {
+ match child_arrays[array].nulls() {
+ Some(null_buffer) => {
+ set_bits(
+ null_buf.as_slice_mut(),
+ null_buffer.validity(),
+ offset_write,
+ null_buffer.offset() + start,
+ len,
+ );
+ }
+ None => {
+ // Slow path. For a non-nullable source, set the bit
range to all 1s directly.
+ let buf = null_buf.as_slice_mut();
+ (offset_write..offset_write + len).for_each(|i|
bit_util::set_bit(buf, i));
+ }
+ }
+ }
+ offset_write += len;
+ }
+
+ let bool_buf = BooleanBuffer::new(null_buf.into(), 0, capacity);
+ Some(NullBuffer::new(bool_buf))
Review Comment:
`NullBuffer::new(bool_buf)` computes `null_count` by scanning the entire
bitmap (`count_set_bits`), adding an extra pass over `capacity` bits on what is
intended as a performance-optimized path. Since `set_bits` already returns the
number of 0-bits written for each segment, consider accumulating `null_count`
during the loop and constructing the null buffer with
`NullBuffer::new_unchecked` (or avoiding the null buffer entirely when the
accumulated null_count is 0).
##########
arrow-select/src/interleave.rs:
##########
@@ -373,13 +375,82 @@ fn interleave_struct(
Ok(Arc::new(struct_array))
}
+fn interleave_list_primitive_child<O: OffsetSizeTrait, T: ArrowPrimitiveType>(
+ interleaved: &Interleave<'_, GenericListArray<O>>,
+ indices: &[(usize, usize)],
+ capacity: usize,
+) -> ArrayRef {
+ let child_arrays: Vec<&PrimitiveArray<T>> = interleaved
+ .arrays
+ .iter()
+ .map(|list| list.values().as_primitive::<T>())
+ .collect();
+
+ let has_child_nulls = child_arrays.iter().any(|a| a.null_count() > 0);
+
+ // Build values buffer by copying contiguous slices
+ let mut values: Vec<T::Native> = Vec::with_capacity(capacity);
+ for &(array, row) in indices {
+ let o = interleaved.arrays[array].value_offsets();
+ let start = o[row].as_usize();
+ let end = o[row + 1].as_usize();
+ if end > start {
+
values.extend_from_slice(&child_arrays[array].values()[start..end]);
+ }
+ }
+
+ // Build null buffer. Pre-allocate with 0x00 (all null), then:
+ // - Sources with nulls: set_bits ORs in valid bits from source.
+ // - Sources without nulls: set the bit range to all 1s directly.
+ let nulls = if has_child_nulls {
+ let null_byte_len = bit_util::ceil(capacity, 8);
+ let mut null_buf = MutableBuffer::new(null_byte_len);
+ null_buf.resize(null_byte_len, 0);
+
+ let mut offset_write = 0;
+ for &(array, row) in indices {
+ let o = interleaved.arrays[array].value_offsets();
+ let start = o[row].as_usize();
+ let end = o[row + 1].as_usize();
+ let len = end - start;
+ if len > 0 {
+ match child_arrays[array].nulls() {
+ Some(null_buffer) => {
+ set_bits(
+ null_buf.as_slice_mut(),
+ null_buffer.validity(),
+ offset_write,
+ null_buffer.offset() + start,
+ len,
+ );
+ }
+ None => {
+ // Slow path. For a non-nullable source, set the bit
range to all 1s directly.
+ let buf = null_buf.as_slice_mut();
+ (offset_write..offset_write + len).for_each(|i|
bit_util::set_bit(buf, i));
+ }
Review Comment:
In the `None` (non-nullable source) branch, setting validity bits with a
per-bit loop is O(len) and can dominate runtime for large contiguous ranges.
Consider setting whole bytes/bit-ranges at once (e.g., fill full bytes with
0xFF and handle head/tail bits) to keep this primitive-child fast path
consistently efficient when mixing nullable and non-nullable sources.
##########
arrow-select/src/interleave.rs:
##########
@@ -373,13 +375,82 @@ fn interleave_struct(
Ok(Arc::new(struct_array))
}
+fn interleave_list_primitive_child<O: OffsetSizeTrait, T: ArrowPrimitiveType>(
+ interleaved: &Interleave<'_, GenericListArray<O>>,
+ indices: &[(usize, usize)],
+ capacity: usize,
+) -> ArrayRef {
+ let child_arrays: Vec<&PrimitiveArray<T>> = interleaved
+ .arrays
+ .iter()
+ .map(|list| list.values().as_primitive::<T>())
+ .collect();
+
+ let has_child_nulls = child_arrays.iter().any(|a| a.null_count() > 0);
+
+ // Build values buffer by copying contiguous slices
+ let mut values: Vec<T::Native> = Vec::with_capacity(capacity);
+ for &(array, row) in indices {
+ let o = interleaved.arrays[array].value_offsets();
+ let start = o[row].as_usize();
+ let end = o[row + 1].as_usize();
+ if end > start {
+
values.extend_from_slice(&child_arrays[array].values()[start..end]);
+ }
+ }
+
+ // Build null buffer. Pre-allocate with 0x00 (all null), then:
+ // - Sources with nulls: set_bits ORs in valid bits from source.
+ // - Sources without nulls: set the bit range to all 1s directly.
+ let nulls = if has_child_nulls {
+ let null_byte_len = bit_util::ceil(capacity, 8);
+ let mut null_buf = MutableBuffer::new(null_byte_len);
+ null_buf.resize(null_byte_len, 0);
+
+ let mut offset_write = 0;
+ for &(array, row) in indices {
+ let o = interleaved.arrays[array].value_offsets();
+ let start = o[row].as_usize();
+ let end = o[row + 1].as_usize();
+ let len = end - start;
+ if len > 0 {
+ match child_arrays[array].nulls() {
+ Some(null_buffer) => {
+ set_bits(
+ null_buf.as_slice_mut(),
+ null_buffer.validity(),
+ offset_write,
+ null_buffer.offset() + start,
+ len,
+ );
+ }
+ None => {
+ // Slow path. For a non-nullable source, set the bit
range to all 1s directly.
+ let buf = null_buf.as_slice_mut();
+ (offset_write..offset_write + len).for_each(|i|
bit_util::set_bit(buf, i));
+ }
+ }
+ }
+ offset_write += len;
+ }
+
+ let bool_buf = BooleanBuffer::new(null_buf.into(), 0, capacity);
+ Some(NullBuffer::new(bool_buf))
+ } else {
+ None
+ };
+
+ Arc::new(PrimitiveArray::<T>::new(values.into(), nulls))
+}
Review Comment:
The primitive-child fast path constructs `PrimitiveArray::<T>` without
preserving logical type parameters from `field.data_type()` (e.g. `Decimal*`
precision/scale and `Timestamp(_, Some(tz))` timezone). Since
`GenericListArray::try_new` requires `field.data_type() == values.data_type()`,
this can panic for lists of decimals or timezone-aware timestamps. Consider
applying `.with_data_type(field.data_type().clone())` (and adjusting the helper
signature to accept the child `DataType`) similar to `interleave_primitive`.
--
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]
