feat: more Results, fewer panics, always have backtraces #761
Conversation
This file contains 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
lwwmanning
changed the title
robert3005
reviewed
Sep 9, 2024
|
What do you mean we wouldn’t get backtraces by default using builtin expect? |
AdamGS
approved these changes
Sep 10, 2024
robert3005
approved these changes
Sep 10, 2024
| @@ -190,7 +190,8 @@ pub fn compress_taxi_data() -> Array { | |||
| let chunks = reader | |||
| .into_iter() | |||
| .map(|batch_result| batch_result.unwrap()) | |||
| .map(Array::from) | |||
| .map(Array::try_from) | |||
| .map(Result::unwrap) | |||
There was a problem hiding this comment.
should collect errors and unwrap at the ends
| @@ -45,7 +45,7 @@ pub fn data_vortex_uncompressed(fname_out: &str, downloaded_data: PathBuf) -> Pa | |||
| let array = ChunkedArray::try_new( | |||
| reader | |||
| .into_iter() | |||
| .map(|batch_result| Array::from(batch_result.unwrap())) | |||
| .map(|batch_result| Array::try_from(batch_result.unwrap()).unwrap()) | |||
There was a problem hiding this comment.
collect errors and unwrap at the end
| @@ -101,7 +102,7 @@ pub fn compress_parquet_to_vortex(parquet_path: &Path) -> VortexResult<ChunkedAr | |||
| let chunks = reader | |||
| .map(|batch_result| batch_result.unwrap()) | |||
| .map(|record_batch| { | |||
| let vortex_array = Array::from(record_batch); | |||
| let vortex_array = Array::try_from(record_batch).unwrap(); | |||
There was a problem hiding this comment.
should collect errors and unwrap at the end
| .map(Array::from) | ||
| .map(|a| a.into_struct().unwrap()) | ||
| .map(Array::try_from) | ||
| .map(|a| a.unwrap().into_struct().unwrap()) |
We're a library. Unless the actual binary sets Which is just not that helpful |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
fixes #564
The goals of this PR is to ensure the following:
VortexResultwith a well-formed error from most fallible functionsassert)To accomplish this, this PR does several things.
First, it adds much stricter clippy lints (e.g., deny on
unwrap,expect,panic; forbids fallibleFromimpls; etc.).Second, I did an audit to make functions that are truly fallible return
VortexResultinstead of panicking. The remaining ones that I saw but didn't fix are tracked under #765Third, for places where we truly want to panic (specifically because an internal invariant has been violated, typically indicating programmer error), this PR adds:
vortex_panic!)VortexUnwraptrait, which is implemented only onVortexResultand adds avortex_unwrap()functionVortexExpecttrait, which is implemented onVortexResultandOption<T>and adds avortex_expectfunction that takes a string literalTaken together, the main downsides are that we have a special snowflake unwrap/expect instead of the traditional ones, and we don't have an effective lint rule to prevent calling those in functions that return Result... but I think the latter at least is acceptable (I know @AdamGS finds those lints overly restrictive anyway).
ALTERNATIVELY, a middle ground would be to replace calls to
vortex_unwrapandvortex_expectwithexpectand un-denyexpect_used. This would be more "typical" Rust but with the disadvantage that we wouldn't get backtraces by default in the cases where we have bugs. Or we could dounwrap_or_else(|err| vortex_panic!(err))orok_or_else(|| vortex_panic!(...))everywhere, but that's pretty verbose.