alamb commented on code in PR #9816:
URL: https://github.com/apache/arrow-rs/pull/9816#discussion_r3148956092
##########
arrow-data/src/data.rs:
##########
@@ -324,7 +336,8 @@ impl ArrayData {
// because we use this buffer to calculate `null_count`
// in `Self::new_unchecked`.
if let Some(null_bit_buffer) = null_bit_buffer.as_ref() {
- let needed_len = bit_util::ceil(len + offset, 8);
+ let len_plus_offset = checked_len_plus_offset(&data_type, len,
offset)?;
+ let needed_len = bit_util::ceil(len_plus_offset, 8);
Review Comment:
Right --
https://github.com/apache/arrow-rs/blob/4fa8d2ff5f18f2d773f9642631715509f844a062/arrow-buffer/src/util/bit_util.rs#L95-L97
Uses div_ceil,
https://doc.rust-lang.org/std/primitive.usize.html#method.div_ceil
> Calculates the smallest value greater than or equal to self that is a
multiple of rhs.
though maybe we should just remove bit_util::ceil and call `div_ceil`
directly now that it is in stable Rust 🤔 ( as a follow on PR)
##########
arrow-data/src/data.rs:
##########
@@ -1554,15 +1578,15 @@ impl ArrayData {
where
T: ArrowNativeType + TryInto<i64> + num_traits::Num +
std::fmt::Display,
{
- let required_len = self.len + self.offset;
+ let required_len = checked_len_plus_offset(&self.data_type, self.len,
self.offset)?;
let buffer = &self.buffers[0];
// This should have been checked as part of `validate()` prior
// to calling `validate_full()` but double check to be sure
assert!(buffer.len() / mem::size_of::<T>() >= required_len);
Review Comment:
I do think it is a potential panic (though as the code says it "should not
be possible")
making it an internal error of some sort would likely be the more defensive
strategy
I can do that in a follow on PR
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
