mbutrovich commented on code in PR #11918:
URL: https://github.com/apache/arrow-rs/pull/11918#discussion_r4446176678
##########
arrow-select/src/coalesce/boolean.rs:
##########
@@ -156,4 +184,172 @@ mod tests {
assert_eq!(second.as_boolean(), &source.slice(2, 2));
assert_eq!(in_progress.finish().unwrap().len(), 0);
}
+
+ const FILTER_LEN: usize = 269;
+ const SOURCE_OFFSET: usize = 3;
+
+ #[test]
+ fn filtered_words_without_nulls_match_filter_kernel() {
+ let len = SOURCE_OFFSET + FILTER_LEN;
+ let values = (0..len).map(source_value).collect();
+ let all_valid = BooleanArray::new(values,
Some(NullBuffer::new_valid(len)))
+ .slice(SOURCE_OFFSET, FILTER_LEN);
+ let without_validity = non_null_source();
+ assert!(without_validity.nulls().is_none());
+ assert!(all_valid.nulls().is_some());
+
+ for (case, source) in [
+ ("without validity", without_validity),
+ ("all-valid validity", all_valid),
+ ] {
+ assert_eq!(source.null_count(), 0);
+ assert_filtered_words_match_filter_kernel(case, source);
+ }
+ }
+
+ #[test]
+ fn filtered_words_with_nulls_match_filter_kernel() {
+ let source = BooleanArray::from_iter(
+ (0..SOURCE_OFFSET + FILTER_LEN)
+ .map(|index| (index % 11 != 0).then_some(index % 3 == 0 ||
index % 7 == 0)),
+ )
+ .slice(SOURCE_OFFSET, FILTER_LEN);
+ assert!(source.null_count() > 0);
+
+ assert_filtered_words_match_filter_kernel("with nulls", source);
+ }
Review Comment:
Could we add a source whose validity has a different bit offset than its
values? Every source in these tests comes from one `slice`, so values and
validity always share `SOURCE_OFFSET`. The fused path builds a separate
`BitChunks` for each buffer, and nothing here would catch a version that reused
the values offset for validity. A `BooleanArray` built from two differently
sliced buffers covers it:
```rust
#[test]
fn filtered_words_with_unaligned_validity_match_filter_kernel() {
let values: BooleanBuffer = (0..SOURCE_OFFSET +
FILTER_LEN).map(source_value).collect();
let validity: BooleanBuffer = (0..FILTER_LEN + 11).map(|index| index %
11 != 0).collect();
let source = BooleanArray::new(
values.slice(SOURCE_OFFSET, FILTER_LEN),
Some(NullBuffer::new(validity.slice(11, FILTER_LEN))),
);
assert_ne!(source.values().offset(), source.nulls().unwrap().offset());
assert_filtered_words_match_filter_kernel("unaligned validity", source);
}
```
##########
arrow-select/src/coalesce/boolean.rs:
##########
@@ -77,8 +77,35 @@ impl InProgressArray for InProgressBooleanArray {
source: ArrayRef,
filter: &FilterPredicate,
) -> Result<(), ArrowError> {
- let filtered = filter.filter(source.as_ref())?;
- self.append_array(filtered.as_boolean());
+ if filter.filter_len() > source.len() {
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "Filter predicate of length {} is larger than target array of
length {}",
+ filter.filter_len(),
+ source.len()
+ )));
+ }
+
+ let source = source.as_boolean();
+ let output_values = self
+ .values
+ .get_or_insert_with(|| BooleanBufferBuilder::new(self.batch_size));
+ // Treat all-valid sources as non-null to keep output validity lazy.
+ if let Some(source_nulls) = source.nulls().filter(|nulls|
nulls.null_count() != 0) {
+ filter_bits_compress_pair(
+ source.values(),
+ source_nulls.inner(),
+ filter,
+ |values, validity, count| {
+ output_values.append_word(values, count);
+ self.nulls.append_word(validity, count);
+ },
+ );
+ } else {
+ filter_bits_compress(source.values(), filter, |values, count| {
+ output_values.append_word(values, count)
+ });
+ self.nulls.append_n_non_nulls(filter.count());
+ }
Review Comment:
What should happen here for a precomputed `Indices` predicate that keeps at
most one row per 64? Before this PR, `filter.filter()` sent that case to
[`filter_bits`](https://github.com/apache/arrow-rs/blob/f059b1ecbb148409f813fe79189252c138b4bac5/arrow-select/src/filter.rs#L743-L757),
which gathers by index because compressing "loses to precomputed `Indices`
once fewer than one bit per word is kept". This path compresses every non-empty
mask word whatever the strategy.
The coalescer optimizes the predicate for any batch with more than one
column
([coalesce.rs](https://github.com/apache/arrow-rs/blob/f059b1ecbb148409f813fe79189252c138b4bac5/arrow-select/src/coalesce.rs#L707-L718)),
and every primitive type qualifies for the sparse path
([coalesce.rs](https://github.com/apache/arrow-rs/blob/f059b1ecbb148409f813fe79189252c138b4bac5/arrow-select/src/coalesce.rs#L46-L52)).
So a batch like `(Int64, Boolean)` at 0.1% selectivity reaches this code with
`Indices`. `single_boolean` has one column, so its predicate is never optimized
and the bot runs don't cover this case.
Could we keep that fallback and send `Indices` at or below `len / 64` to
`filter.filter()` as before? If compressing holds up there instead, could we
add a multi-column Boolean case to `coalesce_kernels` that shows it?
--
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]
