perf(arrow): build constant filter masks with BooleanBuffer instead of Vec<bool> - #3177
Conversation
…f Vec<bool> The predicate converter builds all-true and all-false boolean masks with BooleanArray::from(vec![b; n]), which allocates an n-byte Vec<bool> and then bit-packs it into the array. BooleanBuffer::new_set/new_unset write the packed bits directly, skipping the throwaway Vec and the pack pass. Covers the always-true/false predicates and the initial accumulators for is_in / not_in, built once per batch. Output is identical (all-set/all-unset, no nulls). Isolated construction is ~200x faster at 8192 rows; the real per-batch win is smaller since this is a fraction of filter evaluation.
275042b to
0b4a5f8
Compare
laskoviymishka
left a comment
There was a problem hiding this comment.
Nice optimization, going straight to a bit-packed BooleanBuffer instead of allocating a Vec<bool> and packing it is the right move, and it's output-identical to the old path at every batch size, empty included. No objection to the approvals.
I left a couple of non-blocking notes inline: the four construction sites are formatted inconsistently now (cargo fmt will want to collapse two of them), and none of these paths has a direct test to catch a future new_set/new_unset swap. Neither needs to hold this.
The one thing I'd ask for the perf claim itself — there's no benchmark in the diff, so could we link the profiling run, or drop in a small criterion bench, in the PR body? Makes the ~200x easy to defend the next time we bump Arrow. Not a blocker.
All minor. Will wait for some time before merging, to others to weigh in.
| fn build_always_true(&self) -> Result<Box<PredicateResult>> { | ||
| Ok(Box::new(|batch| { | ||
| Ok(BooleanArray::from(vec![true; batch.num_rows()])) | ||
| Ok(BooleanArray::new( |
There was a problem hiding this comment.
Small thing — these two helper sites got written multi-line while the r#in/not_in accumulator sites are single-liners, and at this indent they're only 80 chars, so cargo fmt will want to collapse them to match.
Since all four sites now repeat the same new_set/new_unset idiom, a tiny private constant_bool_array(n: usize, value: bool) -> BooleanArray would unify them and sidestep the formatting question entirely. wdyt?
There was a problem hiding this comment.
That is a good idea. I aded a helper function as you suggested.
| let left = project_column(&batch, idx)?; | ||
|
|
||
| let mut acc = BooleanArray::from(vec![false; batch.num_rows()]); | ||
| let mut acc = BooleanArray::new(BooleanBuffer::new_unset(batch.num_rows()), None); |
There was a problem hiding this comment.
These is_in/not_in accumulator seeds (and the always_true/always_false paths) aren't run against a batch by any test that asserts the output, so a swapped new_set/new_unset — or an n=0 regression — would pass silently.
Since the whole point here is output-identical behavior, I'd add a small test over a zero-row and a nonzero batch asserting the values are the expected constant and null_count() is zero. Cheap guard for a path that's easy to get subtly wrong on a future edit.
There was a problem hiding this comment.
Added test_constant_bool_array to do the assertion.
|
@laskoviymishka Thanks for the review. Updated the PR description with the details of perf run. I didn't commit the bench since it didn't seem worth it. |
What changes are included in this PR?
The predicate converter builds all-true and all-false boolean masks with BooleanArray::from(vec![b; n]), which allocates an n-byte Vec and then bit-packs it into the array. BooleanBuffer::new_set/new_unset write the packed bits directly, skipping the throwaway Vec and the pack pass.
Covers the always-true/false predicates and the initial accumulators for is_in / not_in, built once per batch. Output is identical (all-set/all-unset, no nulls). Isolated construction is ~125x faster at 8192 rows; the real per-batch win is smaller since this is a fraction of filter evaluation.
Profiling
Isolated construction, release build, 2M iterations with warmup, on an
Apple M4. Each iteration observes the whole buffer via
count_set_bits()so the fill can't be elided under inlining/LTO. This isa conservative floor.
Vec<bool>)BooleanBuffer)The per-batch win in real filter evaluation is a fraction of this, since
constant-mask construction is one step of many.
Throwaway harness
Are these changes tested?
Added
test_constant_bool_array