apache / arrow-rs

Official Rust implementation of Apache Arrow
https://arrow.apache.org/
Apache License 2.0
2.62k stars 802 forks source link

Convert some panics that happen on invalid parquet files to error results #6738

Open jp0317 opened 6 days ago

jp0317 commented 6 days ago

Which issue does this PR close?

This solves some of #6737.

Rationale for this change

Some code changes to replace some panics with proper errors

What changes are included in this PR?

Some codes that lead to panic are converted to returning error results.

Are there any user-facing changes?

Behavior change from panics to errors.

jp0317 commented 3 days ago

Thanks @tustvold for the review!

this seems to add a number of untested additional checks...

I think the changes are more about converting panics to errors, rather than actual code logic.

looking for things that might panic, instead going from a failing test to a fix

these panics were triggered in my own fuzzing test with invalid parquet files. Nevertheless, i think it's a similar topic of "how to handle invalid inputs" as discussed in #5323. Reading this doc, imho errors better than panics unless it's really something unrecoverable.

jp0317 commented 2 days ago

Hi @tustvold, I removed some changes based on your comment. PTAL, thanks!

jp0317 commented 1 day ago

Thanks! Just a few nits. However, I wonder if perhaps this should be multiple PRs with rationales given for each change.

Thanks for the review! imho these changes share the same rationale in that they just convert panics to errors