alamb opened a new pull request, #12206:
URL: https://github.com/apache/arrow-rs/pull/12206
# Which issue does this PR close?
- Follow-up to review discussion on #11174
# Rationale for this change
`take_bits` and `take_bits_with_validity` in `arrow-select/src/take.rs` are
instantiated with `VALIDATE_INDICES = false` after `check_bounds` has run. In
that mode they read the source bits through raw pointers (`pack_bit`,
`copy_bit_if_set`) with no bounds check, so an out-of-bounds index is undefined
behavior.
Today they are safe `fn`s whose soundness depends on every caller having
validated `indices` first. A safe function must not be able to cause undefined
behavior for any input, regardless of who calls it, so the `unsafe` boundary
belongs on these functions, not implicitly on their callers. #11174 makes
`take_bits` `pub(crate)` and adds a second caller outside `take.rs`, which
widens the surface where that implicit contract can be broken.
# What changes are included in this PR?
- `take_bits` and `take_bits_with_validity` become `unsafe fn` with a `#
Safety` section describing the precondition when `VALIDATE_INDICES` is `false`
(and noting there is none when it is `true`).
- The three call sites in `take_nulls` and `take_boolean` are wrapped in
`unsafe { }` with SAFETY comments that trace the precondition back to
`check_bounds` in `take`/`take_record_batch`.
No behavior change. This will conflict trivially with #11174, which also
touches the `take_bits` signature; whichever lands second needs a one-line
rebase.
The remaining `VALIDATE_INDICES = false` helpers in this file
(`take_native`, `take_nulls`, `take_impl`, and the per-type `take_*` functions)
have the same shape and could be moved behind the same boundary in a follow-up.
This PR keeps the scope to the two functions that do the raw reads directly.
# Are these changes tested?
Covered by the existing `take` tests, which exercise both the validated and
unvalidated instantiations. `cargo fmt --check`, `cargo clippy -p arrow-select
--all-targets --all-features -- -D warnings`, and `cargo test -p arrow-select
--lib take` pass locally.
# Are there any user-facing changes?
No. Both functions are private to `arrow-select`.
--
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]
