Dandandan commented on a change in pull request #8900:
URL: https://github.com/apache/arrow/pull/8900#discussion_r541772220
##########
File path: rust/arrow/src/compute/kernels/comparison.rs
##########
@@ -47,9 +47,18 @@ macro_rules! compare_op {
let null_bit_buffer =
combine_option_bitmap($left.data_ref(), $right.data_ref(),
$left.len())?;
- let mut result = BooleanBufferBuilder::new($left.len());
+ let byte_capacity = bit_util::ceil($left.len(), 8);
+ let actual_capacity =
bit_util::round_upto_multiple_of_64(byte_capacity);
+ let mut buffer = MutableBuffer::new(actual_capacity);
+ buffer.resize(byte_capacity);
+ let data = buffer.raw_data_mut();
+
for i in 0..$left.len() {
- result.append($op($left.value(i), $right.value(i)))?;
+ if $op($left.value(i), $right.value(i)) {
+ unsafe {
Review comment:
I will revert the change with the asserts for now. I think the `value`
functions should be marked unsafe that don't perform bound checks, and other
functions that can trigger UB. The macro's themselves should be "relatively"
safe I think, as long as the `.len()` value is correct.
I think a better solution for the future would be to have a safe iterator
that doesn't do bound checking, so I think it's better to move the particular
change out of this PR for now.
----------------------------------------------------------------
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:
[email protected]
