mbutrovich commented on code in PR #11174:
URL: https://github.com/apache/arrow-rs/pull/11174#discussion_r4325727027
##########
arrow-select/src/coalesce.rs:
##########
@@ -809,6 +930,26 @@ trait InProgressArray: std::fmt::Debug + Send + Sync {
}
}
+ /// Copy rows at `indices` from `source` into the in-progress array.
+ /// `indices` must already be bounds-checked against `source`.
+ fn copy_rows_by_indices(
+ &mut self,
+ source: &ArrayRef,
+ indices: &[u32],
Review Comment:
> these traits and methods are private so users cant call this code
arbitrarily. their respective conditions are outlined as safety comments for
both `primitve` and `View` types. I think this method should remain safe
That's fair. `copy_rows` in this trait already works the same way: its impls
call `get_unchecked` behind a `SAFETY` comment that relies on the caller
([primitive.rs](https://github.com/apache/arrow-rs/blob/d58a0fce35b67836ac9b2e20a7efe47a584e0a86/arrow-select/src/coalesce/primitive.rs#L167-L169),
[byte_view.rs](https://github.com/apache/arrow-rs/blob/d58a0fce35b67836ac9b2e20a7efe47a584e0a86/arrow-select/src/coalesce/byte_view.rs#L411-L412)).
I'm fine keeping `copy_rows_by_indices` safe.
The difference I see is where the guarantee comes from. `copy_rows` gets
ranges that the coalescer computed itself. Here the indices are caller input,
and the check that makes the reads safe lives in `push_batch_with_indices`, two
calls up and in another file. That check also moved in the last commit, from
`push_batch_with_index` up to its caller.
Could we add a `debug_assert!` next to each `SAFETY` comment that relies on
caller validation? Those are in
[`InProgressPrimitiveArray::copy_rows_by_indices`](https://github.com/apache/arrow-rs/blob/e31420bf3b6f3959e2a083bfdfa7dc68376580c3/arrow-select/src/coalesce/primitive.rs#L230-L243),
[`InProgressByteViewArray::copy_rows_by_indices`](https://github.com/apache/arrow-rs/blob/e31420bf3b6f3959e2a083bfdfa7dc68376580c3/arrow-select/src/coalesce/byte_view.rs#L657-L665)
(and [again
here](https://github.com/apache/arrow-rs/blob/e31420bf3b6f3959e2a083bfdfa7dc68376580c3/arrow-select/src/coalesce/byte_view.rs#L714-L715)),
[`selected_non_inline_bytes`](https://github.com/apache/arrow-rs/blob/e31420bf3b6f3959e2a083bfdfa7dc68376580c3/arrow-select/src/coalesce/byte_view.rs#L480-L488),
and
[`append_indexed_views_and_copy_strings_with_max_buffer_size`](https://github.com/apache/arrow-rs/blob/e31420bf3b6f3959e2a083bfdfa7dc68376580c3/arrow-select/src/coalesce/byte_view.rs#L383-L395).
The unit te
sts in `primitive.rs` and `byte_view.rs` call `copy_rows_by_indices` directly,
so they never pass through the assert in `push_batch_with_index`. Local asserts
would also cover new callers, such as the `push_interleave` impls you mentioned.
--
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]
