-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat(arrow-buffer): add OverflowError and fallible offset constructors
#10736
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
emilk
wants to merge
4
commits into
apache:main
Choose a base branch
from
emilk:emilk/overflow-error
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
b29820d
feat(arrow-buffer): add `OverflowError` and fallible offset constructors
emilk 2c03ad3
feat(arrow-buffer): name the offset type in `OverflowError`
emilk 95341fa
refactor(arrow-buffer): make the type mandatory in `OverflowError::new`
emilk c13ccc9
fix: update the two `arrow-array` tests that pin the old panic message
emilk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,7 @@ | |
| // under the License. | ||
|
|
||
| use crate::buffer::ScalarBuffer; | ||
| use crate::{ArrowNativeType, MutableBuffer, NullBuffer, OffsetBufferBuilder}; | ||
| use crate::{ArrowNativeType, MutableBuffer, NullBuffer, OffsetBufferBuilder, OverflowError}; | ||
| use std::ops::Deref; | ||
|
|
||
| /// A non-empty buffer of monotonically increasing, positive integers. | ||
|
|
@@ -98,14 +98,24 @@ impl<O: ArrowNativeType> OffsetBuffer<O> { | |
| /// | ||
| /// # Panics | ||
| /// | ||
| /// Panics if `(len + 1) * size_of::<O>()` overflows `usize` | ||
| /// Panics if `(len + 1) * size_of::<O>()` overflows `usize`. | ||
| /// Use [`Self::try_new_zeroed`] for a fallible version. | ||
| pub fn new_zeroed(len: usize) -> Self { | ||
| Self::try_new_zeroed(len).unwrap_or_else(|err| panic!("{err}")) | ||
| } | ||
|
|
||
| /// Create a new [`OffsetBuffer`] containing `len + 1` `0` values | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Errors if `(len + 1) * size_of::<O>()` overflows `usize` | ||
| pub fn try_new_zeroed(len: usize) -> Result<Self, OverflowError> { | ||
| let len_bytes = len | ||
| .checked_add(1) | ||
| .and_then(|o| o.checked_mul(std::mem::size_of::<O>())) | ||
| .expect("overflow"); | ||
| .ok_or_else(|| OverflowError::new::<usize>("buffer length"))?; | ||
| let buffer = MutableBuffer::from_len_zeroed(len_bytes); | ||
| Self(buffer.into_buffer().into()) | ||
| Ok(Self(buffer.into_buffer().into())) | ||
| } | ||
|
|
||
| /// Create a new [`OffsetBuffer`] from the iterator of slice lengths | ||
|
|
@@ -121,8 +131,27 @@ impl<O: ArrowNativeType> OffsetBuffer<O> { | |
| /// | ||
| /// # Panics | ||
| /// | ||
| /// Panics on overflow | ||
| /// Panics on overflow. Use [`Self::try_from_lengths`] for a fallible version. | ||
| pub fn from_lengths<I>(lengths: I) -> Self | ||
| where | ||
| I: IntoIterator<Item = usize>, | ||
| { | ||
| Self::try_from_lengths(lengths).unwrap_or_else(|err| panic!("{err}")) | ||
| } | ||
|
|
||
| /// Create a new [`OffsetBuffer`] from the iterator of slice lengths | ||
| /// | ||
| /// ``` | ||
| /// # use arrow_buffer::OffsetBuffer; | ||
| /// let offsets = OffsetBuffer::<i32>::try_from_lengths([1, 3, 5]).unwrap(); | ||
| /// assert_eq!(offsets.as_ref(), &[0, 1, 4, 9]); | ||
| /// ``` | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Errors if the total length overflows `usize` or `O`, e.g. if the lengths | ||
| /// add up to more than `i32::MAX` for a `OffsetBuffer<i32>`. | ||
| pub fn try_from_lengths<I>(lengths: I) -> Result<Self, OverflowError> | ||
| where | ||
| I: IntoIterator<Item = usize>, | ||
| { | ||
|
|
@@ -132,12 +161,14 @@ impl<O: ArrowNativeType> OffsetBuffer<O> { | |
|
|
||
| let mut acc = 0_usize; | ||
| for length in iter { | ||
| acc = acc.checked_add(length).expect("usize overflow"); | ||
| acc = acc | ||
| .checked_add(length) | ||
| .ok_or_else(|| OverflowError::new::<usize>("total length"))?; | ||
| out.push(O::usize_as(acc)) | ||
| } | ||
| // Check for overflow | ||
| O::from_usize(acc).expect("offset overflow"); | ||
| Self(out.into()) | ||
| O::from_usize(acc).ok_or_else(|| OverflowError::new::<O>("offset").with_value(acc))?; | ||
| Ok(Self(out.into())) | ||
| } | ||
|
|
||
| /// Create a new [`OffsetBuffer`] where each slice has the same length | ||
|
|
@@ -153,28 +184,45 @@ impl<O: ArrowNativeType> OffsetBuffer<O> { | |
| /// | ||
| /// # Panics | ||
| /// | ||
| /// Panics on overflow | ||
| /// Panics on overflow. Use [`Self::try_from_repeated_length`] for a fallible version. | ||
| pub fn from_repeated_length(length: usize, n: usize) -> Self { | ||
| Self::try_from_repeated_length(length, n).unwrap_or_else(|err| panic!("{err}")) | ||
| } | ||
|
|
||
| /// Create a new [`OffsetBuffer`] where each slice has the same length | ||
| /// `length`, repeated `n` times. | ||
| /// | ||
| /// ``` | ||
| /// # use arrow_buffer::OffsetBuffer; | ||
| /// let offsets = OffsetBuffer::<i32>::try_from_repeated_length(4, 3).unwrap(); | ||
| /// assert_eq!(offsets.as_ref(), &[0, 4, 8, 12]); | ||
| /// ``` | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Errors if `length * n` overflows `usize` or `O`. | ||
| pub fn try_from_repeated_length(length: usize, n: usize) -> Result<Self, OverflowError> { | ||
| if n == 0 { | ||
| return Self::new_empty(); | ||
| return Ok(Self::new_empty()); | ||
| } | ||
|
|
||
| if length == 0 { | ||
| return Self::new_zeroed(n); | ||
| return Self::try_new_zeroed(n); | ||
| } | ||
|
|
||
| // Check for overflow | ||
| // Making sure we don't overflow usize or O when calculating the total length | ||
| length.checked_mul(n).expect("usize overflow"); | ||
| let total_length = length | ||
| .checked_mul(n) | ||
| .ok_or_else(|| OverflowError::new::<usize>("total length"))?; | ||
|
|
||
| // Check for overflow | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: we should try and keep the comments from |
||
| O::from_usize(length * n).expect("offset overflow"); | ||
| O::from_usize(total_length) | ||
| .ok_or_else(|| OverflowError::new::<O>("offset").with_value(total_length))?; | ||
|
|
||
| let offsets = (0..=n) | ||
| .map(|index| O::usize_as(index * length)) | ||
| .collect::<Vec<O>>(); | ||
|
|
||
| Self(ScalarBuffer::from(offsets)) | ||
| Ok(Self(ScalarBuffer::from(offsets))) | ||
| } | ||
|
|
||
| /// Get an Iterator over the lengths of this [`OffsetBuffer`] | ||
|
|
@@ -445,6 +493,64 @@ mod tests { | |
| OffsetBuffer::new(vec![-1, 0, 1].into()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn try_from_lengths_overflow() { | ||
| // Fits in an i64, but not in an i32: | ||
| let lengths = [u32::MAX as usize, 1]; | ||
|
|
||
| let err = OffsetBuffer::<i32>::try_from_lengths(lengths).unwrap_err(); | ||
| assert_eq!( | ||
| err.to_string(), | ||
| "offset overflow: 4294967296 does not fit in i32" | ||
| ); | ||
| assert!(OffsetBuffer::<i64>::try_from_lengths(lengths).is_ok()); | ||
|
|
||
| let err = OffsetBuffer::<i32>::try_from_lengths([usize::MAX, 1]).unwrap_err(); | ||
| assert_eq!( | ||
| err.to_string(), | ||
| "total length overflow: does not fit in usize" | ||
| ); | ||
|
|
||
| // The panicking version agrees: | ||
| assert!(std::panic::catch_unwind(|| OffsetBuffer::<i32>::from_lengths(lengths)).is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn try_from_repeated_length_overflow() { | ||
| assert_eq!( | ||
| OffsetBuffer::<i32>::try_from_repeated_length(2, usize::MAX) | ||
| .unwrap_err() | ||
| .to_string(), | ||
| "total length overflow: does not fit in usize" | ||
| ); | ||
| assert_eq!( | ||
| OffsetBuffer::<i32>::try_from_repeated_length(u32::MAX as usize, 2) | ||
| .unwrap_err() | ||
| .to_string(), | ||
| "offset overflow: 8589934590 does not fit in i32" | ||
| ); | ||
| assert_eq!( | ||
| OffsetBuffer::<i32>::try_from_repeated_length(4, 3) | ||
| .unwrap() | ||
| .as_ref(), | ||
| &[0, 4, 8, 12] | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn try_new_zeroed_overflow() { | ||
| assert_eq!( | ||
| OffsetBuffer::<i64>::try_new_zeroed(usize::MAX) | ||
| .unwrap_err() | ||
| .to_string(), | ||
| "buffer length overflow: does not fit in usize" | ||
| ); | ||
| assert_eq!( | ||
| OffsetBuffer::<i32>::try_new_zeroed(3).unwrap().as_ref(), | ||
| &[0; 4] | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn offsets() { | ||
| OffsetBuffer::new(vec![0, 1, 2, 3].into()); | ||
|
|
@@ -485,7 +591,7 @@ mod tests { | |
| } | ||
|
|
||
| #[test] | ||
| #[should_panic(expected = "usize overflow")] | ||
| #[should_panic(expected = "total length overflow: does not fit in usize")] | ||
| fn from_lengths_usize_overflow() { | ||
| OffsetBuffer::<i32>::from_lengths([usize::MAX, 1]); | ||
| } | ||
|
|
@@ -509,7 +615,7 @@ mod tests { | |
| } | ||
|
|
||
| #[test] | ||
| #[should_panic(expected = "usize overflow")] | ||
| #[should_panic(expected = "total length overflow: does not fit in usize")] | ||
| fn from_repeated_lengths_usize_length_usize_overflow() { | ||
| OffsetBuffer::<i32>::from_repeated_length(usize::MAX, 2); | ||
| } | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 typeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't use
ArrowErrorwithout adding a dependency onarrow-schema. But in a future PR I want to add a variant toArrowErrorthat just wrapsOverflowError