-
Notifications
You must be signed in to change notification settings - Fork 1k
Alamb/test boolean kernels #8793
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
34 commits
Select commit
Hold shift + click to select a range
c7d9267
add bitwise ops
rluvaton d14e5b7
add bitwise ops
rluvaton 739fe0a
cleanup
rluvaton 0e15b32
pub(crate) as I don't like that we have both mutable and only left mu…
rluvaton c442299
start adding tests
rluvaton 2f28dc3
add tests
rluvaton c4676a6
add trait for left
rluvaton da03628
format
rluvaton 652a256
revert changes
rluvaton 0c29f0e
fix validation
rluvaton bcd4863
remove many unsafe and cleanup
rluvaton 6b7bfe9
format
rluvaton aec92d6
add reproduction test
rluvaton db3e853
extract, cleanup and add comments
rluvaton 0a64bcb
add comments
rluvaton ca621f8
Merge remote-tracking branch 'apache/main' into add-bitwise-ops-to-bo…
alamb d63d72c
Update arrow-buffer/src/buffer/mutable_ops.rs
alamb 464e56c
Merge branch 'add-bitwise-ops-to-boolean-buffer-builder' of github.co…
alamb 07679d7
Revert changes to boolean
alamb bfdf381
Restore enough for the tests
alamb 246d4e2
Improve docs
alamb b9acb34
Move into mutable module
alamb d590ee1
Add example/doc tests
alamb ccf266f
Add tests for out of bounds
alamb 005c444
Add tests for unary ops
alamb 3a8e760
Add panic doc
alamb cf52bdf
fmt
alamb 6dbed0b
Move buffer modification to bit_utils
alamb 9ca7e45
Move tests and remove changes to MutableBufer
alamb 5cb50d5
Merge remote-tracking branch 'apache/main' into add-bitwise-ops-to-bo…
alamb 379d1ec
Update docs
alamb 1fb4981
fix docs
alamb b0cf38b
Use new `bitwise_binary_op` in boolean kernels
alamb 5e4e242
hack
alamb File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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 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 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
| // under the License. | ||
|
|
||
| use super::{Buffer, MutableBuffer}; | ||
| use crate::bit_util::{bitwise_binary_op, bitwise_unary_op}; | ||
| use crate::util::bit_util::ceil; | ||
|
|
||
| /// Apply a bitwise operation `op` to four inputs and return the result as a Buffer. | ||
|
|
@@ -60,39 +61,70 @@ where | |
|
|
||
| /// Apply a bitwise operation `op` to two inputs and return the result as a Buffer. | ||
| /// The inputs are treated as bitmaps, meaning that offsets and length are specified in number of bits. | ||
| /// | ||
| /// The output is guaranteed to have | ||
| /// 1. all bits outside the specified range set to zero | ||
| /// 2. start at offset zero | ||
| pub fn bitwise_bin_op_helper<F>( | ||
| left: &Buffer, | ||
| left_offset_in_bits: usize, | ||
| right: &Buffer, | ||
| right_offset_in_bits: usize, | ||
| len_in_bits: usize, | ||
| mut op: F, | ||
| op: F, | ||
| ) -> Buffer | ||
| where | ||
| F: FnMut(u64, u64) -> u64, | ||
| { | ||
| let left_chunks = left.bit_chunks(left_offset_in_bits, len_in_bits); | ||
| let right_chunks = right.bit_chunks(right_offset_in_bits, len_in_bits); | ||
| if len_in_bits == 0 { | ||
| return Buffer::default(); | ||
| } | ||
|
|
||
| let chunks = left_chunks | ||
| .iter() | ||
| .zip(right_chunks.iter()) | ||
| .map(|(left, right)| op(left, right)); | ||
| // Soundness: `BitChunks` is a `BitChunks` iterator which | ||
| // correctly reports its upper bound | ||
| let mut buffer = unsafe { MutableBuffer::from_trusted_len_iter(chunks) }; | ||
| // figure out the starting byte for left buffer | ||
| let start_byte = left_offset_in_bits / 8; | ||
| let starting_bit_in_byte = left_offset_in_bits % 8; | ||
|
|
||
| let remainder_bytes = ceil(left_chunks.remainder_len(), 8); | ||
| let rem = op(left_chunks.remainder_bits(), right_chunks.remainder_bits()); | ||
| // we are counting its starting from the least significant bit, to to_le_bytes should be correct | ||
| let rem = &rem.to_le_bytes()[0..remainder_bytes]; | ||
| buffer.extend_from_slice(rem); | ||
| let len_bytes = ceil(starting_bit_in_byte + len_in_bits, 8); | ||
| let mut result = left[start_byte..len_bytes].to_vec(); | ||
| bitwise_binary_op( | ||
| &mut result, | ||
| starting_bit_in_byte, | ||
| right, | ||
| right_offset_in_bits, | ||
| len_in_bits, | ||
| op, | ||
| ); | ||
|
|
||
| buffer.into() | ||
| // shift result to the left so that that it starts at offset zero (TODO do this a word at a time) | ||
| shift_left_by(&mut result, starting_bit_in_byte); | ||
| result.into() | ||
| } | ||
|
|
||
| /// Shift the bits in the buffer to the left by `shift` bits. | ||
| /// `shift` must be less than 8. | ||
| fn shift_left_by(buffer: &mut [u8], starting_bit_in_byte: usize) { | ||
| if starting_bit_in_byte == 0 { | ||
| return; | ||
| } | ||
| assert!(starting_bit_in_byte < 8); | ||
| let shift = 8 - starting_bit_in_byte; | ||
| let carry_mask = ((1u8 << starting_bit_in_byte) - 1) << shift; | ||
|
|
||
| let mut carry = 0; | ||
| // shift from right to left | ||
| for b in buffer.iter_mut().rev() { | ||
| let new_carry = (*b & carry_mask) >> shift; | ||
| *b = (*b << starting_bit_in_byte) | carry; | ||
| carry = new_carry; | ||
| } | ||
| } | ||
|
|
||
| /// Apply a bitwise operation `op` to one input and return the result as a Buffer. | ||
| /// The input is treated as a bitmap, meaning that offset and length are specified in number of bits. | ||
| /// | ||
| /// The output is guaranteed to have | ||
| /// 1. all bits outside the specified range set to zero | ||
| /// 2. start at offset zero | ||
| pub fn bitwise_unary_op_helper<F>( | ||
| left: &Buffer, | ||
| offset_in_bits: usize, | ||
|
|
@@ -102,26 +134,22 @@ pub fn bitwise_unary_op_helper<F>( | |
| where | ||
| F: FnMut(u64) -> u64, | ||
| { | ||
| // reserve capacity and set length so we can get a typed view of u64 chunks | ||
| let mut result = | ||
| MutableBuffer::new(ceil(len_in_bits, 8)).with_bitset(len_in_bits / 64 * 8, false); | ||
|
|
||
| let left_chunks = left.bit_chunks(offset_in_bits, len_in_bits); | ||
|
|
||
| let result_chunks = result.typed_data_mut::<u64>().iter_mut(); | ||
|
|
||
| result_chunks | ||
| .zip(left_chunks.iter()) | ||
| .for_each(|(res, left)| { | ||
| *res = op(left); | ||
| if len_in_bits == 0 { | ||
| return Buffer::default(); | ||
| } | ||
| // already byte aligned, copy over directly | ||
| let len_in_bytes = ceil(len_in_bits, 8); | ||
| let mut result; | ||
| if offset_in_bits == 0 { | ||
| result = left.as_slice()[0..len_in_bytes].to_vec(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This shouldn't be needed as well |
||
| bitwise_unary_op(&mut result, 0, len_in_bits, op); | ||
| } else { | ||
| // need to align bits | ||
| result = vec![0u8; len_in_bytes]; | ||
| bitwise_binary_op(&mut result, 0, left, offset_in_bits, len_in_bits, |_, b| { | ||
| op(b) | ||
| }); | ||
|
|
||
| let remainder_bytes = ceil(left_chunks.remainder_len(), 8); | ||
| let rem = op(left_chunks.remainder_bits()); | ||
| // we are counting its starting from the least significant bit, to to_le_bytes should be correct | ||
| let rem = &rem.to_le_bytes()[0..remainder_bytes]; | ||
| result.extend_from_slice(rem); | ||
|
|
||
| } | ||
| result.into() | ||
| } | ||
|
|
||
|
|
@@ -206,3 +234,39 @@ pub fn buffer_bin_and_not( | |
| pub fn buffer_unary_not(left: &Buffer, offset_in_bits: usize, len_in_bits: usize) -> Buffer { | ||
| bitwise_unary_op_helper(left, offset_in_bits, len_in_bits, |a| !a) | ||
| } | ||
|
|
||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| #[test] | ||
| fn test_shift_left_by() { | ||
| let input = vec![0b10110011, 0b00011100, 0b11111111]; | ||
| do_shift_left_by(&input, 0, &input); | ||
| do_shift_left_by(&input, 1, &[0b01100110, 0b00111001, 0b11111110]); | ||
| do_shift_left_by(&input, 2, &[0b11001100, 0b01110011, 0b11111100]); | ||
| do_shift_left_by(&input, 3, &[0b10011000, 0b11100111, 0b11111000]); | ||
| do_shift_left_by(&input, 4, &[0b00110001, 0b11001111, 0b11110000]); | ||
| do_shift_left_by(&input, 5, &[0b01100011, 0b10011111, 0b11100000]); | ||
| do_shift_left_by(&input, 6, &[0b11000111, 0b00111111, 0b11000000]); | ||
| do_shift_left_by(&input, 7, &[0b10001110, 0b01111111, 0b10000000]); | ||
|
|
||
| } | ||
| fn do_shift_left_by(input: &[u8], shift: usize, expected: &[u8]) { | ||
| let mut buffer = input.to_vec(); | ||
| super::shift_left_by(&mut buffer, shift); | ||
| assert_eq!(buffer, expected, | ||
| "\nshift_left_by({}, {})\nactual: {}\nexpected: {}", | ||
| buffer_string(input), shift, | ||
| buffer_string(&buffer), | ||
| buffer_string(expected) | ||
| ); | ||
| } | ||
| fn buffer_string(buffer: &[u8]) -> String { | ||
| use std::fmt::Write; | ||
| let mut s = String::new(); | ||
| for b in buffer { | ||
| write!(&mut s, "{:08b} ", b).unwrap(); | ||
| } | ||
| s | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does an extra copy which wasn't there before (using from_trusted_len_iter)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 you are right. We do need a new allocation to write into but don't need to copy the values
It is fascinating however, that this code is often still faster than the previous one (maybe due to fewer branches)
I'll see if I can perhaps optimize the case when the offsets are zero which I think is a common case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optimization works well: #8807