-
Notifications
You must be signed in to change notification settings - Fork 175
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
feat(rust)!: use BoxedError in Error::IO #2329
Conversation
ACTION NEEDED Lance follows the Conventional Commits specification for release automation. The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. For details on the error please inspect the "PR Title Check" action. |
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 don't really see the point of creating lance-file::Error
(and others) and wrapping it in Error::IO
. The wrapping errors is more for wrapping our dependencies errors like those from Arrow
, ObjectStore
, and DataFusion
.
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.
This is looking cleaner. Got two small suggestions and then I think this is ready to go :)
rust/lance-io/src/object_store.rs
Outdated
// TODO: Check the correct way to construct a object_store::Error::Generic | ||
let err = lance_core::Error::from(object_store::Error::Generic { | ||
store: "", | ||
source: format!("Unsupported URI scheme: {}", unknow_scheme).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.
I'd maybe go with ObjectStore::NotSupported
here instead:
// TODO: Check the correct way to construct a object_store::Error::Generic | |
let err = lance_core::Error::from(object_store::Error::Generic { | |
store: "", | |
source: format!("Unsupported URI scheme: {}", unknow_scheme).into(), | |
}); | |
let err = lance_core::Error::from(object_store::Error::NotSupported { | |
source: format!("Unsupported URI scheme: {}", unknow_scheme).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.
sorry, I forget this two TODO comments, it's removed now, thank you!
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 think you missed my actual change here, which is to use Error::NotSupported
instead of Error::Generic
for this spot. Same with the other 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.
ha, I totally missed your comment, fixed.
rust/lance-table/src/io/commit.rs
Outdated
unknow_scheme => { | ||
// TODO: Check the correct way to construct a object_store::Error::Generic | ||
let err = lance_core::Error::from(object_store::Error::Generic { | ||
store: "", | ||
source: format!("Unsupported URI scheme: {}", unknow_scheme).into(), | ||
}); | ||
Err(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.
Same thing here:
unknow_scheme => { | |
// TODO: Check the correct way to construct a object_store::Error::Generic | |
let err = lance_core::Error::from(object_store::Error::Generic { | |
store: "", | |
source: format!("Unsupported URI scheme: {}", unknow_scheme).into(), | |
}); | |
Err(err) | |
} | |
unknown_scheme => { | |
Err(lance_core::Error::from(object_store::Error::NotSupported { | |
source: format!("Unsupported URI scheme: {}", unknown_scheme).into(), | |
})) | |
} |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2329 +/- ##
=======================================
Coverage 80.79% 80.79%
=======================================
Files 192 192
Lines 56213 56212 -1
Branches 56213 56212 -1
=======================================
+ Hits 45416 45417 +1
- Misses 8128 8137 +9
+ Partials 2669 2658 -11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@broccoliSpicy there seem to be some lint / syntax errors that need to be fixed. |
thank you so much @wjones127 ! fixed lint locally but I will see how github CI goes |
sorry this PR has awfully too many commits, I have issue with my local |
Closes #2328
currently, lots of error in our codebase are wrapped in Error::IO, some are not accurate. like this one:
source example
this PR still wraps this error in Error::IO, as we haven't define more precise Error type for HashJoiner yet.
we may need to define our own Error enum in each of our submodule, lance-encoding::Error, lance-file::Error, lance-index::Error etc.
so we can write error return like this: