chore: use new OffsetBuffer::subtract helper#23424
Conversation
github-actions
Bot
added
common
| if let (Some(first), Some(last)) = (offsets.first(), offsets.last()) | ||
| && (!first.is_zero() || last.as_usize() != list.values().len()) | ||
| { | ||
| let offsets = offsets.iter().map(|offset| *offset - *first).collect(); | ||
|
|
||
| //todo: use unsafe Offset::new_unchecked? | ||
| return OffsetBuffer::new(offsets); | ||
| } | ||
|
|
||
| offsets.clone() |
There was a problem hiding this comment.
Even though this code has last != list.values().len() and the subtract doesn't have that check, that check is not needed as we don't use the last values at all
| } | ||
|
|
||
| offsets.clone() | ||
| offsets.clone().subtract(offsets[0]) |
There was a problem hiding this comment.
For a prefix slice where the first offset is zero but the last visible offset is before the end of the original child values, subtract(0) returns a clone sharing the original allocation. The previous
Implementation created a compact offset buffer in this case.
There was a problem hiding this comment.
Yes, I'm willing to sacrifice that in favor of avoiding copy and because this was not guaranteed
xudong963
left a comment
xudong963
left a comment
There was a problem hiding this comment.
Other places look good to me
| let offsets = string_array | ||
| .offsets() | ||
| .clone() | ||
| .subtract(string_array.offsets()[0]); |
There was a problem hiding this comment.
Nice cleanup — this also replaces the unsafe new_unchecked path with the checked subtract. Since the subtrahend is offsets()[0], the internal self[0] >= rhs assert can never fire, so the safety upgrade is free.
3 participants
Which issue does this PR close?
N/A
Rationale for this change
Replace manually written code with the newly added helper I added to arrow-rs in:
OffsetBuffer::subtractto allow to shift offsets by value arrow-rs#10120What changes are included in this PR?
use
OffsetBuffer::subtractAre these changes tested?
Existing tests
Are there any user-facing changes?
No