alamb commented on a change in pull request #9413:
URL: https://github.com/apache/arrow/pull/9413#discussion_r571603181
##########
File path: rust/arrow/src/compute/kernels/arithmetic.rs
##########
@@ -225,15 +205,7 @@ where
unsafe { Buffer::try_from_trusted_len_iter(values) }
}?;
- let data = ArrayData::new(
Review comment:
FWIW I think the new style of calling `ArrayData::new_primitive` is
much more understandable: Rather than 7 parameters which have some implicit
assumptions about invariants between them, we have 2 parameters that make more
sense.
##########
File path: rust/arrow/src/compute/kernels/boolean.rs
##########
@@ -288,27 +288,25 @@ where
// Align/shift left data on offset as needed, since new bitmaps are
shifted and aligned to 0 already
// NOTE: this probably only works for primitive arrays.
- let data_buffers = if left.offset() == 0 {
- left_data.buffers().to_vec()
+ let buffer = if left.offset() == 0 {
+ left_data.buffers()[0].clone()
} else {
// Shift each data buffer by type's bit_width * offset.
- left_data
- .buffers()
- .iter()
- .map(|buf| buf.slice(left.offset() * T::get_byte_width()))
- .collect::<Vec<_>>()
+ left_data.buffers()[0].slice(left.offset() * T::get_byte_width())
};
+ // UNSOUND: when `offset != 0`, the sliced buffer's `len` will be smaller
than `left.len()`
Review comment:
I don't understand this comment -- is it just pointing out that a bug
still exists in the code? It doesn't look like your change changes the behavior
at this time. Maybe it is worth a JIRA ticket to track it
----------------------------------------------------------------
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]
