-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Description
Is your feature request related to a problem or challenge?
Wrapping a DataFusionError in a DataFusionError::Context can break behavior for users if they match on specific error variants. E.g. code that handles a Result like
match do_datafusion_things() {
Ok(_) => Ok(())
Err(DataFusionError::Execution(_)) => {
// retry
}
Err(e) => Err(e)
}will stop working as soon as the DataFusionError::Execution returned by DataFusion would be wrapped in context.
Describe the solution you'd like
The best I came up with so far is a macro like
/// Macro to check if an error matches a pattern, similar to `matches!` but for error checking.
/// This macro will traverse through wrapped errors to find a match.
///
/// It is preferable to use `error_matches` over matching on specific error types directly. This is because
/// matching code could otherwise change behavior after the addition of an indirection, e.g. by wrapping
/// an error in Context.
///
/// # Examples
///
/// For example, given the following error chain:
/// ```text
/// DataFusionError::ArrowError
/// ArrowError::ExternalError
/// Box(DataFusionError::Context)
/// DataFusionError::ResourcesExhausted
/// ```
/// `error_matches` will return true for `DataFusionError::ResourceExhausted`,
/// `DataFusionError::Context` and `DataFusionError::ArrowError`.
///
/// In code, this could look like:
/// ```
/// use datafusion_common::error_matches;
/// use datafusion_common::DataFusionError;
///
/// let err = DataFusionError::ArrowError(
/// ArrowError::ExternalError(Box::new(DataFusionError::Context(
/// "additional context".to_string(),
/// Box::new(DataFusionError::ResourcesExhausted("oom".to_string())),
/// ))),
/// None,
/// );
///
/// assert!(error_matches!(err, DataFusionError::ArrowError(..)));
/// assert!(error_matches!(err, DataFusionError::Context(..)));
/// assert!(error_matches!(err, DataFusionError::ResourcesExhausted(_)));
/// ```
#[macro_export]
macro_rules! error_matches {
...
}While this seems nice to work with, it's not a drop-in replacement of the match syntax above.
I'm still ramping up in Rust, so any suggestions on how to improve this API are very much welcomed!
Describe alternatives you've considered
- Using
DataFusionError::find_root(): this function is the closest solution to the problem we have today. But it only allows to check against the very last error in a chain, any errors in between can't currently be handled. - Extending
impl DataFusionErrorwith different variants offn wraps(&self, other: &DataFusionError) -> bool: even if a macro provides a nicer API to work with, it will ultimately be backed by such a function
Additional context
@rluvaton noticed this issue when I added context to an existing error in a separate PR. It started a discussion about whether or not adding context could be considered a breaking change with the lack of such an API.
Note, this proposal is heavily inspired by Go's errors.Is which is the most robust and idiomatic way of checking errors in Go.