Skip to content
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

Implement std::error::Error::source for ArrowError and FlightError #3567

Merged
merged 4 commits into from
Jan 19, 2023

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Jan 19, 2023

Which issue does this PR close?

Close #3566

Rationale for this change

In IOx (and in DataFusion) we often want to know what the root cause of an error is (e.g was it a bug or was it a resources exhausted). ArrowError can wrap other errors with Arrow::External (and DataFusion has something similar) but there is no easy way to get at the source of the error to walk the chain without knowing all the possible types that may be present

I believe this is what https://doc.rust-lang.org/std/error/trait.Error.html#method.source is for

I would like to be able to write something like this to walk the chain

        let mut root_error: &dyn Error = &e3;
        loop {
            match root_error.source() {
                // walk the next level
                Some(source) => root_error = source,
                // at root (as much as we know)
                None => break,
            }
        }

However, ArrowError does not implement source yet

Of course, all the errors in the chain need to implement this

What changes are included in this PR?

  1. Implement std::error::Error::source for ArrowError and FlightError
  2. tests for same

Are there any user-facing changes?

source() sometimes now returns something useful

@github-actions github-actions bot added arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate labels Jan 19, 2023
@@ -52,7 +54,15 @@ impl std::fmt::Display for FlightError {
}
}

impl std::error::Error for FlightError {}
impl Error for FlightError {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the change (and the same for ArrowError) -- the rest of the PR is tests

impl std::error::Error for FlightError {}
impl Error for FlightError {
fn source(&self) -> Option<&(dyn Error + 'static)> {
if let Self::ExternalError(e) = self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope this never happens, but This should probably traverse further if this is an ExternalError again. Same goes for the arrow-schema impl.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reading of https://doc.rust-lang.org/std/error/trait.Error.html#method.source

The lower-level source of this error, if any.

Suggests that the iteration all the way down a chain of errors should be done outside the Error::source. For example I believe this iteration is what https://docs.rs/snafu/0.7.4/snafu/trait.ErrorCompat.html#method.iter_chain is for.

So in other words, I don't think the source() method should have the traversal (even if ArrowError is being silly and boxing itself)

arrow-flight/src/error.rs Outdated Show resolved Hide resolved
Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com>
@alamb alamb merged commit 9bb6aaf into apache:master Jan 19, 2023
@ursabot
Copy link

ursabot commented Jan 19, 2023

Benchmark runs are scheduled for baseline = 4fa0ee8 and contender = 9bb6aaf. 9bb6aaf is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Error::Source for ArrowError and FlightError
5 participants