apache / apache/datafusion

API to match against any error in chain

Open
#15,713 0 comments 0 reactions 0 assignees View on GitHub
enhancement
Dominant language
Rust
Stars
9.3k
Forks
2.4k
Avg merge
3d 7h
Merged PRs (30d)
344

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

```rs
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
```rs
/// 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

1. Using [`DataFusionError::find_root()`](https://github.com/apache/datafusion/blob/63f37a34404391f19114407c2a3965213306bb8e/datafusion/common/src/error.rs#L423): 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.
2. Extending `impl DataFusionError` with different variants of `fn 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](https://github.com/apache/datafusion/pull/15692#discussion_r2041239380) 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`](https://pkg.go.dev/errors#Is) which is the most robust and idiomatic way of checking errors in Go.

Contributor guide

Open the contributing guide

Research direction

Start with datafusion/common/src/error.rs and the DataFusionError::find_root entry point, then compare the proposed error_matches! API with the documented error-chain examples. Done means agreeing on a public way to match any error in the chain and its behavior for the shown variants.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
api
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.