Jefffrey commented on code in PR #10656: URL: https://github.com/apache/arrow-rs/pull/10656#discussion_r3771712125
########## arrow-array/src/array/byte_array.rs: ########## Review Comment: this is interesting; it might actually be a bug. i dont see why we should restrict to requiring an upper bound especially as its not guaranteed an iter will yield only up to upper bound > It is not enforced that an iterator implementation yields the declared number of elements. A buggy iterator may yield less than the lower bound or more than the upper bound of elements. - https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.size_hint perhaps in a followup/separate PR we can remove this expect, to have one less panic in our codebase ########## arrow-array/src/cast.rs: ########## Review Comment: can remove this note about panicking here since we add it below now same for all other methods in this file ########## arrow-array/src/builder/generic_bytes_view_builder.rs: ########## @@ -296,8 +300,12 @@ impl<T: ByteViewType + ?Sized> GenericByteViewBuilder<T> { } /// Returns the value at the given index + /// /// Useful if we want to know what value has been inserted to the builder - /// The index has to be smaller than `self.len()`, otherwise it will panic + /// + /// # Panics + /// + /// Panics unless `index < self.len()` Review Comment: ```suggestion /// Panics if `index >= self.len()` ``` -- 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]
