feat(arrow-buffer): add OverflowError and fallible offset constructors - #10736
feat(arrow-buffer): add OverflowError and fallible offset constructors#10736emilk wants to merge 4 commits into
OverflowError and fallible offset constructors#10736Conversation
`arrow-buffer` has no `ArrowError`, so following the `MutableBufferError` precedent from apache#10317, add a small `OverflowError` for the functions that panic on arithmetic overflow, and give each of them a `try_` variant: * `OffsetBuffer::{try_new_zeroed, try_from_lengths, try_from_repeated_length}` * `OffsetBufferBuilder::{try_push_length, try_finish, try_finish_cloned}` * `NullBuffer::try_expand` The panicking versions now delegate to the fallible ones, and panic with the error's `Display`, so their messages are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`offset overflow` did not say what the offsets did not fit in. It now reads `offset overflow: 4294967296 does not fit in i32`, which points at the 2 GiB limit of 32 bit offsets and at `i64` as the fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`with_type` was easy to forget, which left errors that did not say what the value failed to fit in. The type is now a parameter of `new`, so every `OverflowError` names it: `total length overflow: does not fit in usize`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`OffsetBuffer::from_repeated_length` now panics with `total length overflow: does not fit in usize`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rich-T-kid
left a comment
Rich-T-kid
left a comment
There was a problem hiding this comment.
@emilk LGTM , A follow up PR will re-wire arrow-rs call sites right?
| #[derive(Clone, Copy, Debug, PartialEq, Eq)] | ||
| pub struct OverflowError { | ||
| what: &'static str, | ||
| value: Option<usize>, | ||
| type_name: &'static str, | ||
| } |
There was a problem hiding this comment.
what are your thoughts on consildating this error types with the enum introduced in https://github.com/apache/arrow-rs/pull/10317/changes#diff-371342744df1b634b0bd9d90f4fe38c1eb0096df322fd3cc2fbc513f3428046cR38, I.E adding another variant.
I think this is different enough to warrent a seperate type but I thought id throw the idea out there
There was a problem hiding this comment.
At some point we need an enum that contains OverflowError as one of its kinds. I'm not sure MutableBufferError is the right name for it, but I do think we should only have one such error enum per crate.
| .checked_mul(n) | ||
| .ok_or_else(|| OverflowError::new::<usize>("total length"))?; | ||
|
|
||
| // Check for overflow |
There was a problem hiding this comment.
nit: we should try and keep the comments from main
| /// # Errors | ||
| /// | ||
| /// Errors if `self.len() * count` overflows `usize` | ||
| pub fn try_expand(&self, count: usize) -> Result<Self, OverflowError> { |
There was a problem hiding this comment.
I wonder if it makes sense to add a new variant to ArrowError / use an existing one rather than a whole new type. It might be strange to have entirely new types for errrs when the rest of the crates use the same unified error type
There was a problem hiding this comment.
On the other hand I see now there is a special type for mutable buffer https://github.com/apache/arrow-rs/pull/10317/changes#diff-371342744df1b634b0bd9d90f4fe38c1eb0096df322fd3cc2fbc513f3428046cR38 🤔
There was a problem hiding this comment.
We can't use ArrowError without adding a dependency on arrow-schema. But in a future PR I want to add a variant to ArrowError that just wraps OverflowError
Yes exactly. There are also some out-of-bounds-errors I need to add try_-methods for in arrow-buffer |
3 participants
Which issue does this PR close?
MutableBufferErrorprecedent from feat/chore: introduce fallible alternatives for MutableBuffer #10317Rationale for this change
OffsetBuffer::<i32>::from_lengthspanics once the lengths add up to more than 2 GiB. The other overflow panics inarrow-bufferare the same story.What changes are included in this PR?
A small
OverflowErrorand atry_variant for functions that panics on overflow:OffsetBuffer::{try_new_zeroed, try_from_lengths, try_from_repeated_length}OffsetBufferBuilder::{try_push_length, try_finish, try_finish_cloned}NullBuffer::try_expandThe panicking versions delegate to the fallible ones and re-panic with the error's
Display, so their panic messages are unchanged.try_push_lengthleaves the builder unchanged when it fails.Are these changes tested?
Yes, new tests for the error paths. The existing
should_panictests are untouched and still pass, which is what pins the panic messages.Are there any user-facing changes?
New public API only, no breaking changes.