feat: explicit python exceptions #1409
Conversation
|
My overall opinion on Python errors is that we should be mapping to standard Python exceptions in most cases, except:
But most other things could be Python exceptions:
The purpose of these errors should be making it as easy as possible for Python users to write error handling code. If I were making breaking changes to the Python exceptions, I would drop the generic |
Completely agree! Previously had a look into the datafusion python repo, and they already did a lot of work mapping to python exceptions. We should be able to adopt much of that since our stacks overlap quite a bit ... I also like the custom exceptions you proposed, along with being generally cautious of adding additional custom ones. |
|
@wjones127 - converted all the errors and did not find any that would warrant a new custom error type other then the ones you mentioned. At times I may have been too liberal with the Main think I took away from this though is that we have to a) name / use errors more consistently throughout delta-rs, and b) simplify our errors quite a bit, but this goes beyond the scope of this PR ... |
There was a problem hiding this comment.
I like this cleanup, especially the ergonomics of mapping errors to PythonError::from.
Some minor suggestions, but I think once @wjones127 approves this should be landed
| ObjectStoreError::Generic { source, .. } | ||
| if source.to_string().contains("AWS_S3_ALLOW_UNSAFE_RENAME") => | ||
| { | ||
| DeltaProtocolError::new_err(source.to_string()) | ||
| } |
| .map_err(PyDeltaTableError::from_object_store)?; | ||
| .map_err(PythonError::from)?; |
There was a problem hiding this comment.
🤔 I wonder if we implemented:
impl<T: Into<PythonError>> From<T> for PyErr { ... }could we remove the map_err entirely?
There was a problem hiding this comment.
Cool idea, and gave it a quick go, but the compiler is complaining that Into is not our trait, and neither is PyErr, so it won't allow it :(
Co-authored-by: R. Tyler Croy <[email protected]>
Working on an integration, I felt that our error handling in python was a bit opaque and handling errors on the python side would have to rely on inspecting the error message, which can easily silently break, if not covered by tests. The proposed approach would be to introduce a few more python exceptions and - as we already do - rely on existing python exceptions where appropriate. As this would be a bigger breaking change on the python side I guess, looking for some feedback, if you think this is worthwhile. @wjones127 @fvaleye @rtyler <!--- For example: - closes #106 ---> <!--- Share links to useful documentation ---> --------- Co-authored-by: R. Tyler Croy <[email protected]>
# Description While doing #1409 it became evident, that our errors are somewhat organically grown and could do with some pruning. At the same time we are hoping to reorganize (#1136) delta-rs a bit, to make it easier to reason about. This PR is a first attempt to introduce a more explicit approach to how we model our errors. Here I'd like to propose we work towards the approach taken in the object store crate - specifically have very specific errors in submodules, but do not surface those errors to the user. Rather collapse them into eventually a single error type. Since delta-rs does much more things then object store, I believe we will eventually end up with some more top level errors, or have maybe a two level hierarch. As a first step in this PR: - move `checkpoints.rs` into actions module (as proposed in #1136) - move top level errors into new `errors` module - at lest the ones defined in `delta.rs` - try and consolidate `ActionError` and `CheckpointError` which are now part of the `action` module. As I needed to touch a bunch of imports anyhow, I took the liberty to organize them according to what to the best of my knowledge is the [leading contender](https://rust-lang.github.io/rustfmt/?version=v1.4.38&search=#StdExternalCrate%5C%3A) for what rust-fmt might do. @wjones127 @rtyler @Blajda - opening this as a draft to get some feedback, as this is a somewhat intrusive change to the overall crate, in case you want to go another way. # Related Issue(s) part of: #1136 # Documentation <!--- Share links to useful documentation --->
Description
Working on an integration, I felt that our error handling in python was a bit opaque and handling errors on the python side would have to rely on inspecting the error message, which can easily silently break, if not covered by tests.
The proposed approach would be to introduce a few more python exceptions and - as we already do - rely on existing python exceptions where appropriate.
As this would be a bigger breaking change on the python side I guess, looking for some feedback, if you think this is worthwhile.
@wjones127 @fvaleye @rtyler
Related Issue(s)
Documentation