Adding try_append_value implementation to ByteViewBuilder
#8594
Conversation
samueleresca
force-pushed
the
safer-appendvalue-bytes-view
branch
from
73faf99 to
8859ff7
Compare
| .map(u32::from_le_bytes) | ||
| .ok_or_else(|| { | ||
| ArrowError::InvalidArgumentError( | ||
| "String must be at least 4 bytes for non-inline view".to_string(), |
There was a problem hiding this comment.
This error is unreachable as we checked that the value is longer than MAX_INLINE_VIEW_LEN (12 bytes) above.
| let offset = self.in_progress.len() as u32; | ||
| let offset: u32 = self.in_progress.len().try_into().map_err(|_| { | ||
| ArrowError::InvalidArgumentError(format!( | ||
| "In-progress buffer length {} exceeds u32::MAX", |
There was a problem hiding this comment.
-
I think the method can recover by starting a new in-progress buffer instead of returning an error here.
-
I am unsure if this error is even reachable.
There was a problem hiding this comment.
I think a new buffer would be allocated in the line immediately above this. Maybe we should do a checked add in let required_cap = self.in_progress.len() + v.len(); 🤔
To error here, we would need a usize that doesn't fit into a u32.. I think all platforms we care about have usize that is at least u32 (aka 32-bit architectures)
| let offset = self.in_progress.len() as u32; | ||
| let offset: u32 = self.in_progress.len().try_into().map_err(|_| { | ||
| ArrowError::InvalidArgumentError(format!( | ||
| "In-progress buffer length {} exceeds u32::MAX", |
There was a problem hiding this comment.
I think a new buffer would be allocated in the line immediately above this. Maybe we should do a checked add in let required_cap = self.in_progress.len() + v.len(); 🤔
To error here, we would need a usize that doesn't fit into a u32.. I think all platforms we care about have usize that is at least u32 (aka 32-bit architectures)
3 participants
Which issue does this PR close?
concat_elements_utf8viewpanics with large buffer on 64bit machines datafusion#17857Rationale for this change
These changes add a safer version of
append_valueinByteViewBuilderthat handles panics calledtry_append_value. Datafusions will consume the API and handle the Result coming back from the function.What changes are included in this PR?
Are these changes tested?
The method is already covered by existing tests.
Are there any user-facing changes?
No breaking changes, as the original
append_valuemethod hasn't changed.