-
Notifications
You must be signed in to change notification settings - Fork 1.9k
chore(cubesql): Improve error handling #10118
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10118 +/- ##
===========================================
+ Coverage 55.17% 78.06% +22.89%
===========================================
Files 213 457 +244
Lines 16904 91078 +74174
Branches 3335 3335
===========================================
+ Hits 9326 71101 +61775
- Misses 7100 19499 +12399
Partials 478 478
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:
|
| #[error("CubeError: {0}")] | ||
| Cube(CubeError, Option<Arc<SpanId>>), | ||
| #[error("DataFusionError: {0}")] | ||
| DataFusion(DataFusionError, Option<Arc<SpanId>>), |
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.
Probably, we need to box DataFusionError, ArrowError. I assume it's a large enum. Can we add 1-liner tests with assert_eq for mem::size_of<ConnectionError>. Thanks
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.
I have tested this and CubeError is the variant of biggest size. DataFusionError and ArrowError actually happen to be the smallest, so I avoided boxing them in the end. I added the mem::size_of test to verify this.
ovr
left a comment
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.
LGTM, minor nit, related to Boxing + check sizeof
9b86e64 to
b5d23ad
Compare
Signed-off-by: Alex Qyoun-ae <[email protected]>
b5d23ad to
5c9d92d
Compare
Check List
Description of Changes Made
This PR improves error handling, unwrapping an onion of nested DataFusion and Arrow errors, and adding information regarding whether the error happened in the database execution or post-processing steps.