Jefffrey commented on code in PR #10709:
URL: https://github.com/apache/arrow-rs/pull/10709#discussion_r3793565007
##########
arrow-data/src/data.rs:
##########
@@ -965,7 +965,8 @@ impl ArrayData {
}
let actual_len = nulls.validity().len();
- let needed_len = bit_util::ceil(len_plus_offset, 8);
+ // ArrayData::offset does not apply to the null buffer, which
carries its own offset
+ let needed_len = bit_util::ceil(nulls.offset() + nulls.len(), 8);
if actual_len < needed_len {
Review Comment:
i wonder if we should instead remove this validation; its brought up in the
original issue:
> Furthermore, this validation is currently redundant. There is no way to
create an invalid `BooleanBuffer` today. I don't think it's necessarily a
problem to have the validation (if fixed) since there may be a way to create an
invalid `BooleanBuffer` in the future. I just mention this to point out that
only the "false error" case (and not the "potentially unsafe" case) can be
encountered.
and the way this fix handles it, its just validating that the `NullBuffer`
contains enough bytes for its bits, which sounds like something `NullBuffer`
itself should guarantee/check, and not `ArrayData` here. perhaps its a holdover
from some old code that was refactored
--
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]
