-
Notifications
You must be signed in to change notification settings - Fork 179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(connect): better error propagation & handling #3675
chore(connect): better error propagation & handling #3675
Conversation
CodSpeed Performance ReportMerging #3675 will improve performances by 31.1%Comparing Summary
Benchmarks breakdown
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3675 +/- ##
==========================================
+ Coverage 76.75% 76.81% +0.06%
==========================================
Files 728 729 +1
Lines 90944 91164 +220
==========================================
+ Hits 69803 70027 +224
+ Misses 21141 21137 -4
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a couple of comments, but looks good so far. Not super familiar with the inner workings of the service, so I'll take a deeper look into it when I get the chance.
} | ||
plan => bail!("Unsupported relation type: \"{}\"", rel_name(&plan)), | ||
plan => not_yet_implemented!("relation type: \"{}\"", rel_name(&plan))?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit:
Change to not_yet_implemented!(r#"relation type: "{}""#, rel_name(&plan))?
, which will allow you to avoid escaping nested quotes.
let name = table.table_name; | ||
bail!("Tried to write to table {name} but it is not yet implemented. Try to write to a path instead."); | ||
SaveType::Table(_) => { | ||
return not_yet_implemented!("write to table").map_err(|e| e.into()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have not_yet_implemented
act similar to bail
? I.e., the macro expands to a return Err(..)?;
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so I think I'll follow up with this in a separate PR. I'm planning on removing the eyre
crate, and make the error handling similar to the rest of the codebase.
depends on #3666
see here for proper diff
universalmind303/Daft@rust-ray-exec...universalmind303:Daft:error-messages