WIP: Boolean mask backed RowSelection #8902
Draft
+2,073
−435
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Nov 21, 2025
alamb
commented
Nov 25, 2025
| /// Optimized version of `boolean_buffer_and_then` using BMI2 PDEP instructions. | ||
| /// This function performs the same operation but uses bit manipulation instructions | ||
| /// for better performance on supported x86_64 CPUs. | ||
| pub fn boolean_buffer_and_then(left: &BooleanBuffer, right: &BooleanBuffer) -> BooleanBuffer { |
Contributor
Author
There was a problem hiding this comment.
@XiangpengHao did this it in liquid cache:
I will copy that new version over here in this PR
alamb
force-pushed
the
alamb/sketch_boolean_selection
branch
from
826e01f to
e4b2283
Compare
rluvaton
reviewed
Dec 1, 2025
rluvaton
reviewed
Dec 1, 2025
Comment on lines
37
to
38
| let mut buffer = MutableBuffer::from_len_zeroed(left.values().len()); | ||
| buffer.copy_from_slice(left.values()); | ||
| let mut builder = BooleanBufferBuilder::new_from_buffer(buffer, left.len()); |
Member
There was a problem hiding this comment.
Note that you have a bug here and also copy more data than needed.
BooleanBuffer have an offset to start from the values and here you:
- add more data than needed by creating a buffer with everything
- the left set_indices here need to be offseted by the
left.offset()in order to set the correct bit (you should fix to not copy unneeded values and then you can keep this)
Contributor
Author
There was a problem hiding this comment.
This was a really good call. When I was porting over the tests they caught exactly this problem
I actually found we could do the same thing, but probably faster via
let mut builder = BooleanBufferBuilder::new(left.len());
builder.append_buffer(&left);
Which then calls BooleanBufferBuilder::append_packed_range
(conveniently I know someone who just optimized that function 😆 )
rluvaton
reviewed
Dec 1, 2025
|
|
||
| if left.len() == right.len() { | ||
| debug_assert_eq!(left.count_set_bits(), left.len()); | ||
| return right.clone(); |
Contributor
Author
There was a problem hiding this comment.
I think in this case left is always (and asserted) to be all 1s so no AND is necessary
rluvaton
reviewed
Dec 1, 2025
Comment on lines
+43
to
+50
| for bit_idx in left.set_indices() { | ||
| let predicate = other_bits | ||
| .next() | ||
| .expect("Mismatch in set bits between self and other"); | ||
| if !predicate { | ||
| builder.set_bit(bit_idx, false); | ||
| } |
Member
There was a problem hiding this comment.
maybe a faster approach would be to use set slices and then use AND with right starting from the last location
alamb
force-pushed
the
alamb/sketch_boolean_selection
branch
4 times, most recently
from
f767c2e to
4e01ca4
Compare
alamb
force-pushed
the
alamb/sketch_boolean_selection
branch
from
d312a8c to
8d9b155
Compare
Contributor
Author
|
run benchmark arrow_reader_clickbench |
|
🤖 |
Contributor
Author
|
show benchmark queue |
|
🤖 Hi @alamb, you asked to view the benchmark queue (#8902 (comment)).
|
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
Author
|
🤔 going the wrong direction |
Contributor
Author
|
run benchmark arrow_reader_clickbench |
Contributor
Author
|
run benchmark arrow_reader |
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
Author
|
I have been profiling this morning, and basically I have concluded I need to do two things: #8844 (comment) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
TODOs
Rationale for this change
Make the parquet predicate evaluation faster by not converting back/forth between BooleanArray and RowSelection as much
What changes are included in this PR?
Are these changes tested?
TBD
Are there any user-facing changes?
Internal notes for myself