Change some panics to errors in parquet decoder #8602
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rambleraptor
force-pushed
the
fix-some-panics
branch
from
17d5287 to
4d193b3
Compare
|
I guess we can lump this in with #7806 |
|
I'm happy to split this up into some separate PRs. I know it's a lot of random things as-is. |
The pedant in me wants to take you up on your offer, but there's not so much going on here that I think that's necessary. Maybe just change the title to something that sounds better 😉. ("Address panics found in external testing"). |
etseidl
approved these changes
Oct 14, 2025
There was a problem hiding this comment.
Thanks @rambleraptor, these all look sensible to me.
Would it be possible to gin up some tests for at least some of them?
scovich
reviewed
Oct 14, 2025
| return Ok((end, buf.slice(i32_size..end))); | ||
| } | ||
| } | ||
| Err(general_err!("not enough data to read levels")) |
There was a problem hiding this comment.
This is definitely an improvement over the existing code, but it opens a question:
Given that we're reading bytes from a byte buffer, it seems like we must expect to hit this situation at least occasionally? And the correct response is to fetch more bytes, not fail? Is there some mechanism for handling that higher up in the call stack? Or is there some reason it should be impossible for this code to run off the end of the buffer?
There was a problem hiding this comment.
Also -- it seems like read_num_bytes should do bounds checking internally and return Option<T>, so buffer overrun is obvious at the call site instead of a hidden panic footgun? The method has a half dozen other callers, and they all need to do manual bounds checking, in various ways and with varying degrees of safety. In particular, parquet/src/data_type.rs has two call sites that lack any visible bounds checks.
There was a problem hiding this comment.
In this particular instance we're reading a buffer that should contain an entire page of data. If it doesn't, that likely points to a problem with the metadata.
Changes to read_num_bytes would likely need more careful consideration as I suspect it might be used in some performance critical sections.
There was a problem hiding this comment.
I think changing read_num_bytes to return Option would be a good idea, as that would essentially replace the assert in that method. That assert is currently redundant if the caller already checks the bounds, so those two checks would be replaced by one against the option. In the BitReader that might actually improve performance.
| } else if !is_root_node { | ||
| return Err(general_err!("Repetition level must be defined for non-root types")); | ||
| } | ||
| Ok((next_index, Arc::new(builder.build().unwrap()))) |
There was a problem hiding this comment.
How do we know the unwrap is safe?
There was a problem hiding this comment.
build never returns an Err 😉. But good point, could replace unwrap with ?.
rambleraptor
changed the title
Co-authored-by: Ed Seidl <[email protected]>
Co-authored-by: Ryan Johnson <[email protected]>
alamb
changed the title
|
I took the liberty of merging up from main and fixing |
rambleraptor
force-pushed
the
fix-some-panics
branch
from
5176ace to
cdd9f0a
Compare
etseidl
reviewed
Oct 17, 2025
Comment on lines
+1149
to
+1159
| let mut page_header = PageHeader::default(); | ||
| page_header.r#type = PageType::DATA_PAGE_V2; | ||
| page_header.uncompressed_page_size = 10; | ||
| page_header.compressed_page_size = 10; | ||
| let mut data_page_header_v2 = DataPageHeaderV2::default(); | ||
| data_page_header_v2.definition_levels_byte_length = 11; // offset > uncompressed_page_size | ||
| page_header.data_page_header_v2 = Some(data_page_header_v2); | ||
|
|
||
| let buffer = Bytes::new(); | ||
| let err = decode_page(page_header, buffer, Type::INT32, None).unwrap_err(); | ||
| assert_eq!(err.to_string(), "Parquet error: Invalid page header"); |
There was a problem hiding this comment.
We don't implement Default for the thrift structs nor Debug for Page. Instead you'll have to explicitly instantiate the page header. Something like
let page_header = PageHeader {
r#type: PageType::DATA_PAGE_V2,
uncompressed_page_size: 10,
compressed_page_size: 10,
data_page_header: None,
index_page_header: None,
dictionary_page_header: None,
crc: None,
data_page_header_v2: Some(DataPageHeaderV2 {
num_nulls: 0,
num_rows: 0,
num_values: 0,
encoding: Encoding::PLAIN,
definition_levels_byte_length: 11,
repetition_levels_byte_length: 0,
is_compressed: None,
statistics: None,
}),
};
let buffer = Bytes::new();
match decode_page(page_header, buffer, Type::INT32, None) {
Err(e) => assert_eq!(e.to_string(), "Parquet error: Invalid page header"),
_ => panic!("should have failed"),
}
|
Thanks @rambleraptor. Your force push seems to have done away with the merge from head. Could you please merge in origin/main after fixing the failing tests? |
|
🤖 |
|
🤖: Benchmark completed Details
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rationale for this change
We've caused some unexpected panics from our internal testing. We've put in error checks for all of these so that they don't affect other users.
What changes are included in this PR?
Various error checks to ensure panics don't occur.
Are these changes tested?
Tests should continue to pass.
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
Existing tests should cover these changes.
Are there any user-facing changes?
None.