alamb commented on code in PR #7752:
URL: https://github.com/apache/arrow-rs/pull/7752#discussion_r2164850556
##########
parquet-variant/src/decoder.rs:
##########
@@ -262,21 +265,19 @@ pub(crate) fn decode_timestampntz_micros(data: &[u8]) ->
Result<NaiveDateTime, A
/// Decodes a Binary from the value section of a variant.
pub(crate) fn decode_binary(data: &[u8]) -> Result<&[u8], ArrowError> {
let len = u32::from_le_bytes(array_from_slice(data, 0)?) as usize;
- let value = slice_from_slice(data, 4..4 + len)?;
- Ok(value)
+ slice_from_slice_at_offset(data, 4, 0..len)
}
/// Decodes a long string from the value section of a variant.
pub(crate) fn decode_long_string(data: &[u8]) -> Result<&str, ArrowError> {
let len = u32::from_le_bytes(array_from_slice(data, 0)?) as usize;
- let string = string_from_slice(data, 4..4 + len)?;
- Ok(string)
+ string_from_slice(data, 4, 0..len)
}
/// Decodes a short string from the value section of a variant.
pub(crate) fn decode_short_string(metadata: u8, data: &[u8]) ->
Result<ShortString, ArrowError> {
let len = (metadata >> 2) as usize;
- let string = string_from_slice(data, 0..len)?;
+ let string = string_from_slice(data, 0, 0..len)?;
Review Comment:
I don't really understand the need for methods like `string_from_slice` in
the first place
For example, I would find it much easier to read
```suggestion
let string = String::try_from(data)
.map_err(|_| "Invalid Utf8 data in short string")
```
But that is just a personal preference
##########
parquet-variant/src/decoder.rs:
##########
@@ -262,21 +265,19 @@ pub(crate) fn decode_timestampntz_micros(data: &[u8]) ->
Result<NaiveDateTime, A
/// Decodes a Binary from the value section of a variant.
pub(crate) fn decode_binary(data: &[u8]) -> Result<&[u8], ArrowError> {
let len = u32::from_le_bytes(array_from_slice(data, 0)?) as usize;
- let value = slice_from_slice(data, 4..4 + len)?;
- Ok(value)
+ slice_from_slice_at_offset(data, 4, 0..len)
Review Comment:
Maybe another way to handle this is
```rust
let len: usize = len.try_into().map_err(...)
```
And let the compiler put in the correct checks 🤔
--
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]
