samuelcolvin commented on code in PR #6295:
URL: https://github.com/apache/arrow-rs/pull/6295#discussion_r1729423099
##########
parquet/src/util/bit_util.rs:
##########
@@ -91,15 +85,22 @@ unsafe impl FromBytes for Int96 {
type Buffer = [u8; 12];
fn try_from_le_slice(b: &[u8]) -> Result<Self> {
- Ok(Self::from_le_bytes(array_from_slice(b)?))
+ let bs: [u8; 12] = array_from_slice(b)?;
+ let mut i = Int96::new();
+ i.set_data(
+ u32::try_from_le_slice(&bs[0..4])?,
+ u32::try_from_le_slice(&bs[4..8])?,
+ u32::try_from_le_slice(&bs[8..12])?,
+ );
+ Ok(i)
}
fn from_le_bytes(bs: Self::Buffer) -> Self {
let mut i = Int96::new();
i.set_data(
- from_le_slice(&bs[0..4]),
- from_le_slice(&bs[4..8]),
- from_le_slice(&bs[8..12]),
+ u32::try_from_le_slice(&bs[0..4]).unwrap(),
+ u32::try_from_le_slice(&bs[4..8]).unwrap(),
+ u32::try_from_le_slice(&bs[8..12]).unwrap(),
Review Comment:
This is the one place (outside tests) where we're still `unwrap`ing
`T::try_from_le_slice`, I think this is okay since the trait is marked as
`unsafe`?
##########
parquet/src/file/page_index/index.rs:
##########
@@ -207,7 +206,10 @@ impl<T: ParquetValueType> NativeIndex<T> {
} else {
let min = min.as_slice();
let max = max.as_slice();
- (Some(from_le_slice::<T>(min)),
Some(from_le_slice::<T>(max)))
+ (
+ Some(T::try_from_le_slice(min)?),
+ Some(T::try_from_le_slice(max)?),
Review Comment:
this is where we were getting the panic from:
```
called `Result::unwrap()` on an `Err` value: General("error converting
value, expected 4 bytes got 0")
...
at
/Users/samuel/.cargo/git/checkouts/arrow-rs-3b86e19e889d5acc/2795b94/parquet/src/util/bit_util.rs:29:5
5: parquet::file::page_index::index::NativeIndex<T>::try_new::{{closure}}
at
/Users/samuel/.cargo/git/checkouts/arrow-rs-3b86e19e889d5acc/2795b94/parquet/src/file/page_index/index.rs:210:31
```
##########
parquet/src/util/bit_util.rs:
##########
@@ -438,7 +439,7 @@ impl BitReader {
}
// TODO: better to avoid copying here
- Some(from_le_slice(v.as_bytes()))
+ T::try_from_le_slice(v.as_bytes()).ok()
Review Comment:
I'm not sure if returning `None` here if `try_from_le_slice` is correct? It
appears to roughly match the meaning in the docstring:
```rs
/// Returns `None` if there's not enough data available. `Some` otherwise.
```
--
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]
