fix: convert some panics into errors#5258
Conversation
wjones127
added
the
critical-fix
westonpace
left a comment
westonpace
left a comment
There was a problem hiding this comment.
Approve assuming we aren't deleting potentially useful info (maybe we can log it?)
| let schema = match batch { | ||
| Some(Ok(b)) => b.schema(), | ||
| Some(Err(e)) => panic!("do this better: error reading first batch: {:?}", e), | ||
| Some(Err(e)) => return Err(std::mem::replace(e, Error::Stop)), |
There was a problem hiding this comment.
Does this swallow the underlying error?
There was a problem hiding this comment.
No it returns the underlying error. Since we are peeking into the stream, we only have an &mut Error not an owned Error. So we just use std::mem::replace to get the owned Error and replace the one in the stream with a dummy Error::Stop.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5258 +/- ##
==========================================
+ Coverage 82.21% 82.23% +0.02%
==========================================
Files 344 344
Lines 144879 144879
Branches 144879 144879
==========================================
+ Hits 119108 119140 +32
+ Misses 21836 21803 -33
- Partials 3935 3936 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 participants
Closes #5257