veluca93 commented on code in PR #6021:
URL: https://github.com/apache/arrow-rs/pull/6021#discussion_r1669387981
##########
parquet/src/arrow/array_reader/byte_view_array.rs:
##########
@@ -315,26 +315,30 @@ impl ByteViewArrayDecoderPlain {
let mut read = 0;
output.views.reserve(to_read);
while self.offset < self.buf.len() && read != to_read {
- if self.offset + 4 > self.buf.len() {
- return Err(ParquetError::EOF("eof decoding byte
array".into()));
- }
- let len_bytes: [u8; 4] = unsafe {
- buf.get_unchecked(self.offset..self.offset + 4)
- .try_into()
- .unwrap()
+ let len_bytes = buf.get(self.offset..self.offset + 4);
+ let len = match len_bytes {
+ None => {
+ return Err(ParquetError::EOF("eof decoding byte
array".into()));
+ }
+ Some(l) => u32::from_le_bytes(l.try_into().unwrap()),
Review Comment:
Yes, I was trying to avoid the version with `let .. else` because I was not
sure on the MSRV policies of arrow-rs :-)
As for being more sound: the original version is unsound if `self.offset +
4` overflows (because it ends up calling .get_unchecked with an empty range,
which is UB). In general, the new version achieves the same result without
unsafe, which is IMO always a good idea (unless there are big performance
issues).
--
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]
