You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "samuelcolvin (via GitHub)" <gi...@apache.org> on 2024/08/23 19:10:41 UTC
[PR] Err on `try_from_le_slice` [arrow-rs]
samuelcolvin opened a new pull request, #6295:
URL: https://github.com/apache/arrow-rs/pull/6295
# Which issue does this PR close?
fix #3577
# Rationale for this change
Our parquet reader is panicing when hitting metadata that is either invalid or unexpected
# What changes are included in this PR?
Remove interval `from_le_slice` method, use `T::try_from_le_slice` instead.
# Are there any user-facing changes?
NA?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Err on `try_from_le_slice` [arrow-rs]
Posted by "samuelcolvin (via GitHub)" <gi...@apache.org>.
samuelcolvin commented on PR #6295:
URL: https://github.com/apache/arrow-rs/pull/6295#issuecomment-2311114748
Okay, I've now added a a pretty good MRE test. @alamb we're really appreciate it if this could be merged.
I'm happy to remove `bad_metadata_err` and the raw data file if you like?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Err on `try_from_le_slice` [arrow-rs]
Posted by "samuelcolvin (via GitHub)" <gi...@apache.org>.
samuelcolvin commented on PR #6295:
URL: https://github.com/apache/arrow-rs/pull/6295#issuecomment-2307708530
if we could get this merged, we can point cargo to use head of `arrow-rs`.
Currently we're getting panics when reading parquet files which (we think) have invalid metadata.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Err on `try_from_le_slice` [arrow-rs]
Posted by "samuelcolvin (via GitHub)" <gi...@apache.org>.
samuelcolvin commented on PR #6295:
URL: https://github.com/apache/arrow-rs/pull/6295#issuecomment-2308521644
@alamb I've added a test, to get it to fail I've included the raw metadata from our problematic file (35KB).
Let me know if you want to keep this or remove or, and if you want me to send you a link to the full 25MB parquet file via email.
> If we make invalid statistics return an error I think it will mean the reader will not be able to read any data from the file
I don't think we should change this, if we run into errors like this we can always call again with
```rs
loader.load_page_index(false, false).await?
```
as shown in the test.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Err on `try_from_le_slice` [arrow-rs]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #6295:
URL: https://github.com/apache/arrow-rs/pull/6295#issuecomment-2307718085
Right the point is that even if we have invalid data it's hard to even debug because by we have no ability to do error handling due the panics.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Err on `try_from_le_slice` [arrow-rs]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #6295:
URL: https://github.com/apache/arrow-rs/pull/6295#issuecomment-2308401864
I have a repro locally but it's a 25MB file that you have to write out then load back again. So not exactly a MRE. I'm happy to share that file if that's helpful. I tried narrowing down to a specific column or value without much luck.
It happens specifically when using MetadataLoader to load metadata dumped by ParquetMetadataWriter.
Our goal by converting this panic to an error is to buy time in figuring out the root cause by falling back to loading the metadata from the file itself instead of the cached version that was written via ParquetMetadataWriter.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Err on `try_from_le_slice` [arrow-rs]
Posted by "samuelcolvin (via GitHub)" <gi...@apache.org>.
samuelcolvin commented on PR #6295:
URL: https://github.com/apache/arrow-rs/pull/6295#issuecomment-2307717560
cc @tustvold who created the original issue.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Err on `try_from_le_slice` [arrow-rs]
Posted by "samuelcolvin (via GitHub)" <gi...@apache.org>.
samuelcolvin commented on code in PR #6295:
URL: https://github.com/apache/arrow-rs/pull/6295#discussion_r1729447472
##########
parquet/src/file/page_index/index.rs:
##########
@@ -192,7 +191,7 @@ impl<T: ParquetValueType> NativeIndex<T> {
let indexes = index
.min_values
.iter()
- .zip(index.max_values.into_iter())
+ .zip(index.max_values.iter())
Review Comment:
change so `max` is borrowed, to match `min`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Err on `try_from_le_slice` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6295:
URL: https://github.com/apache/arrow-rs/pull/6295#discussion_r1729951766
##########
parquet/src/file/statistics.rs:
##########
@@ -186,14 +186,18 @@ pub fn from_thrift(
// INT96 statistics may not be correct, because comparison is signed
// byte-wise, not actual timestamps. It is recommended to ignore
// min/max statistics for INT96 columns.
- let min = min.map(|data| {
+ let min = if let Some(data) = min {
Review Comment:
If we make invalid statistics return an error I think it will mean the reader will not be able to read any data from the file
What do you think about ignoring the error. `.ok()` style instead here -- that way the statistics would come in as `None` but the file could still be read
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Err on `try_from_le_slice` [arrow-rs]
Posted by "samuelcolvin (via GitHub)" <gi...@apache.org>.
samuelcolvin commented on PR #6295:
URL: https://github.com/apache/arrow-rs/pull/6295#issuecomment-2311152232
Thanks so much @alamb, I'll create an issue with a follow up.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Err on `try_from_le_slice` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #6295:
URL: https://github.com/apache/arrow-rs/pull/6295
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Err on `try_from_le_slice` [arrow-rs]
Posted by "samuelcolvin (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org