jorgecarleitao commented on a change in pull request #9965:
URL: https://github.com/apache/arrow/pull/9965#discussion_r610862801
##########
File path: rust/arrow/src/compute/kernels/boolean.rs
##########
@@ -102,41 +80,77 @@ where
valid_buffer.extend_from_slice(&[valid]);
};
- // To get rid off the additional remainder logic we would need an iterator
- // which contains a possible remainder word.
- let remainder = (
- (
- left_chunks.remainder_bits(),
- left_valid_chunks.remainder_bits(),
- ),
- (
- right_chunks.remainder_bits(),
- right_valid_chunks.remainder_bits(),
- ),
- );
+ let left_offset = left.offset();
+ let right_offset = right.offset();
- let base_iter = left_chunks
- .iter()
- .zip(left_valid_chunks.iter())
- .zip(right_chunks.iter().zip(right_valid_chunks.iter()))
- .chain(iter::once(remainder));
+ let left_buffer = left.values();
+ let right_buffer = right.values();
+
+ let left_chunks = left_buffer.bit_chunks(left_offset, len);
+ let right_chunks = right_buffer.bit_chunks(right_offset, len);
+
+ let left_rem = left_chunks.remainder_bits();
+ let right_rem = right_chunks.remainder_bits();
+
+ let opt_left_valid_chunks_and_rem = left
+ .data_ref()
+ .null_buffer()
+ .map(|b| b.bit_chunks(left_offset, len))
+ .map(|chunks| (chunks.iter(), chunks.remainder_bits()));
+ let opt_right_valid_chunks_and_rem = right
+ .data_ref()
+ .null_buffer()
+ .map(|b| b.bit_chunks(right_offset, len))
+ .map(|chunks| (chunks.iter(), chunks.remainder_bits()));
match (
- left.data_ref().null_buffer().is_some(),
- right.data_ref().null_buffer().is_some(),
+ opt_left_valid_chunks_and_rem,
+ opt_right_valid_chunks_and_rem,
) {
- (true, true) => base_iter.for_each(kleene_op),
- (true, false) => base_iter
- .map(|(left, (right_data, _))| (left, (right_data, u64::MAX)))
- .for_each(kleene_op),
- (false, true) => base_iter
- .map(|((left_data, _), right)| ((left_data, u64::MAX), right))
- .for_each(kleene_op),
- (false, false) => base_iter
- .map(|((left_data, _), (right_data, _))| {
- ((left_data, u64::MAX), (right_data, u64::MAX))
- })
- .for_each(kleene_op),
+ (
+ Some((left_valid_chunks, left_valid_rem)),
+ Some((right_valid_chunks, right_valid_rem)),
+ ) => {
+ left_chunks
Review comment:
this is just of a curiosity / knowledge sharing.
I ported this to arrow2 recently and my conclusion there is that the
abstractions needed for the kleene logic are a binary, ternary and quaternary
bitmap operators (e.g.
[here](https://github.com/jorgecarleitao/arrow2/blob/main/src/compute/boolean_kleene.rs#L125)),
as each of these branches is one of these operations on the validity.
Another optimization is that we can use `lhs.values() & rhs.values()`
regardless of the validity instead of a more complex logic on the values, since
the mask will nullify values accordingly.
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
