neilconway commented on code in PR #11770:
URL: https://github.com/apache/arrow-rs/pull/11770#discussion_r4377615936
##########
arrow-array/src/builder/generic_bytes_view_builder.rs:
##########
@@ -224,25 +224,39 @@ impl<T: ByteViewType + ?Sized> GenericByteViewBuilder<T> {
/// Appends an array to the builder.
/// This will flush any in-progress block and append the data buffers
/// and add the (adapted) views.
+ ///
+ /// # Panics
+ ///
+ /// Panics if the builder would hold more than `u32::MAX` data buffers
Review Comment:
I don't think this is a breaking change. Code that produces incorrect
results should not be considered "code that previously worked" -- if that was
the definition of a breaking change, many straightforward bugfixes would not be
allowed in minor releases.
Additional considerations:
* In debug builds, the previous coding resulted in a panic anyway, so there
is no change in behavior.
* In release builds, the previous behavior is actually worse than just
incorrect results: `buffer_index` points at one buffer but the view's `length`
and `offset` were validated against a different buffer. Reader methods like
`GenericByteViewArray::value(k)` do an unchecked read somewhere off into heap
memory, resulting in undefined behavior.
* Overflow requires an array with `u32::MAX` data buffers, which is
extremely unlikely to occur in practice.
--
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]
