Rich-T-kid commented on code in PR #10813:
URL: https://github.com/apache/arrow-rs/pull/10813#discussion_r3896036971
##########
arrow-select/src/take.rs:
##########
@@ -94,11 +94,15 @@ pub fn take(
let options = options.unwrap_or_default();
downcast_integer_array!(
indices => {
+ let indices = indices.to_indices();
if options.check_bounds {
- check_bounds(values.len(), indices)?;
+ // Pre-verify all index values once: take_impl can skip safe
accessor overhead.
+ check_bounds(values.len(), &indices)?;
+ take_impl::<_, false>(values, &indices)
+ } else {
+ // No pre-check: take_impl uses safe accessors that panic on
OOB instead of UB.
+ take_impl::<_, true>(values, &indices)
Review Comment:
similar point to below, going to use the always validate path
(`take_impl<_,true>`) for now and will introduce the false variant in a
separate PR
##########
arrow-select/src/take.rs:
##########
@@ -456,43 +460,268 @@ fn take_native<T: ArrowNativeType, I:
ArrowPrimitiveType>(
}
}
+/// Read the bit at `src_bit_idx` from `src` and, if it is set, write a `1` to
`dst_bit_idx`
+/// in `dst`. Leaves `dst_bit_idx` unchanged (zero) when the source bit is
unset.
+///
+/// ```text
+/// src = 0b00100000 (bit 5 is set)
+/// copy_bit_if_set(src, 5, dst, 2) → dst bit 2 becomes 1
+/// ```
+///
+/// # Safety
+/// - `src` must be valid for reads up to byte `src_bit_idx / 8`.
+/// - `dst` must be valid for writes up to byte `dst_bit_idx / 8`.
+#[inline(always)]
+unsafe fn copy_bit_if_set(src: *const u8, src_bit_idx: usize, dst: *mut u8,
dst_bit_idx: usize) {
+ unsafe {
+ if bit_util::get_bit_raw(src, src_bit_idx) {
+ bit_util::set_bit_raw(dst, dst_bit_idx);
+ }
+ }
+}
+
+/// Read the bit at `bit_idx` from `src` and return it shifted to `out_pos`,
ready to be
+/// OR'd into an output byte accumulator.
+///
+/// ```text
+/// src = 0b10100000 (bit 5 is set)
+/// pack_bit(src, 5, 2) → 0b00000100 (bit from position 5, placed at
position 2)
+/// ```
+///
+/// # Safety
+/// `src` must be valid for reads up to byte `bit_idx / 8`.
+#[inline(always)]
+unsafe fn pack_bit(src: *const u8, bit_idx: usize, out_pos: usize) -> u8 {
+ let byte = unsafe { *src.add(bit_idx >> 3) }; // byte containing bit
`bit_idx`
+ ((byte >> (bit_idx & 7)) & 1) << out_pos // extract the bit, shift to
output position
+}
+
#[inline(never)]
fn take_bits<I: ArrowPrimitiveType, const CHECKED: bool>(
Review Comment:
thinking about this a bit more `CHECKED` is a bad variable name. it should
be `BOUNDS_PRECHECKED` or something of the sort.
CHECKED=true meaning check the bounds isn't very intuitive 😬
if that make sense Ill do it in a seperate PR, probably the same one that
introduces `take_record_batch_unchecked`
--
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]
