alamb commented on code in PR #11206:
URL: https://github.com/apache/arrow-rs/pull/11206#discussion_r4123078789
##########
arrow-select/src/coalesce.rs:
##########
@@ -609,7 +635,10 @@ impl BatchCoalescer {
filter: &BooleanArray,
selected_count: usize,
) -> FilterPredicate {
- let mut filter_builder = FilterBuilder::new_with_count(filter,
selected_count);
+ // SAFETY: `selected_count` is the count of `filter`, either computed
by
+ // `push_batch_with_filter` or guaranteed by the caller of
+ // `push_batch_with_filter_and_count`.
Review Comment:
technically speaking we should probably also mark
`filter_predicate_for_batch` as `unsafe` as well if this is the case
I think it would be better to change the API to take FilterBuilder directly
(see comment above) and then we can isolate the `unsafe` to the single place it
is needed
##########
arrow-select/src/coalesce.rs:
##########
@@ -262,7 +262,33 @@ impl BatchCoalescer {
batch: RecordBatch,
filter: &BooleanArray,
) -> Result<(), ArrowError> {
- self.push_batch_with_filtered_columns(batch, filter)
+ // SAFETY: the count is computed from `filter` itself.
+ unsafe { self.push_batch_with_filter_and_count(batch, filter,
filter.true_count()) }
+ }
+
+ /// Push a batch into the Coalescer after applying a filter whose number of
+ /// selected rows is already known.
+ ///
+ /// This is [`Self::push_batch_with_filter`] without the count of `filter`
+ /// it performs first. Callers that already track how many rows a filter
+ /// selects, for example to enforce a row limit, can pass that number here.
+ ///
+ /// # Safety
+ ///
+ /// `selected_count` must equal [`BooleanArray::true_count`] of `filter`.
It
+ /// sizes the copied rows; see [`FilterBuilder::new_with_count`].
+ pub unsafe fn push_batch_with_filter_and_count(
Review Comment:
I feel this is complicating the public API methods with different
combinations of arguments and can get unweildy (and will get more complicated
if we want to add more). What do you think about instead adding an API that
takes a `FilterBuilder` directly?
```rust
pub fn push_batch_with_filter_builder(
&mut self,
batch: RecordBatch,
filter_builder: FilterBuilder
) -> Result<(), ArrowError> {
```
That would also avoid the need for an additional unsafe API
--
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]
