You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2023/01/19 18:32:31 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #4992: Implement `std::error::Error::source()` for `DataFusionError`, make `DataFusionError::find_root` more generic
alamb opened a new pull request, #4992:
URL: https://github.com/apache/arrow-datafusion/pull/4992
# Which issue does this PR close?
Closes https://github.com/apache/arrow-datafusion/issues/4991
# 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 want a generic way to walk up the error stack
# What changes are included in this PR?
1. Implement `std::error::Error::source()` for `DataFusionError`
2. Make `DataFusionError::find_root` less specific (uses source() rather than so much checking for arrow)
# Are these changes tested?
Yes, covered by (some quite gnarly) existing tests
# Are there any user-facing changes?
Better standard support
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #4992: Implement `std::error::Error::source()` for `DataFusionError`, make `DataFusionError::find_root` more generic
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4992:
URL: https://github.com/apache/arrow-datafusion/pull/4992#discussion_r1090655644
##########
datafusion/common/src/error.rs:
##########
@@ -359,84 +380,49 @@ impl DataFusionError {
///
/// This may be the same as `self`.
pub fn find_root(&self) -> &Self {
- // Note: This is a non-recursive algorithm so we do not run out of stack space, even for long error chains. The
- // algorithm will always terminate because all steps access the next error through "converging" ownership,
- // i.e. there can be a fan-in by multiple parents (e.g. via `Arc`), but never a fan-out by multiple
- // children (e.g. via `Weak` or interior mutability via `Mutex`).
-
- // last error in the chain that was a DataFusionError
- let mut checkpoint: &Self = self;
-
- // current non-DataFusion error
- let mut other_e: Option<OtherErr<'_>> = None;
-
- loop {
- // do we have another error type to explore?
- if let Some(inner) = other_e {
- // `other_e` is now bound to `inner`, so we can clear this path
- other_e = None;
-
- match inner {
- OtherErr::Arrow(inner) => {
- if let ArrowError::ExternalError(inner) = inner {
- other_e = Some(OtherErr::Dyn(inner.as_ref()));
- continue;
- }
- }
- OtherErr::Dyn(inner) => {
- if let Some(inner) = inner.downcast_ref::<Self>() {
- checkpoint = inner;
- continue;
- }
-
- if let Some(inner) = inner.downcast_ref::<ArrowError>() {
- other_e = Some(OtherErr::Arrow(inner));
- continue;
- }
-
- // some errors are wrapped into `Arc`s to share them with multiple receivers
- if let Some(inner) = inner.downcast_ref::<Arc<Self>>() {
- checkpoint = inner.as_ref();
- continue;
- }
-
- if let Some(inner) = inner.downcast_ref::<Arc<ArrowError>>() {
- other_e = Some(OtherErr::Arrow(inner.as_ref()));
- continue;
- }
- }
- }
-
- // dead end?
- break;
- }
-
- // traverse context chain
- if let Self::Context(_msg, inner) = checkpoint {
- checkpoint = inner;
- continue;
+ // Note: This is a non-recursive algorithm so we do not run
+ // out of stack space, even for long error chains.
+
+ let mut last_datafusion_error = self;
+ let mut root_error: &dyn Error = self;
+ while let Some(source) = find_source(root_error) {
+ // walk the next level
+ root_error = source;
+ // remember the lowest datafusion error so far
+ if let Some(e) = root_error.downcast_ref::<DataFusionError>() {
+ last_datafusion_error = e;
}
-
- // The Arrow error may itself contain a datafusion error again
- // See https://github.com/apache/arrow-datafusion/issues/4172
- if let Self::ArrowError(inner) = checkpoint {
- other_e = Some(OtherErr::Arrow(inner));
- continue;
- }
-
- // also try to introspect direct external errors
- if let Self::External(inner) = checkpoint {
- other_e = Some(OtherErr::Dyn(inner.as_ref()));
- continue;
- }
-
- // no more traversal
- break;
}
-
// return last checkpoint (which may be the original error)
- checkpoint
+ last_datafusion_error
+ }
+}
+
+fn find_source<'a>(e: &'a (dyn Error + 'static)) -> Option<&'a (dyn Error + 'static)> {
+ // workaround until https://github.com/apache/arrow-rs/issues/3566 is released
+ if let Some(e) = e.downcast_ref::<ArrowError>() {
+ return if let ArrowError::ExternalError(e) = e {
+ Some(e.as_ref())
+ } else {
+ None
+ };
}
+ // some errors are wrapped into `Arc`s to share them with multiple
Review Comment:
Fixed in https://github.com/apache/arrow-datafusion/pull/5056/commits/f3d2ae644f57cac7fd06f1d1c565ced765e42182
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #4992: Implement `std::error::Error::source()` for `DataFusionError`, make `DataFusionError::find_root` more generic
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #4992:
URL: https://github.com/apache/arrow-datafusion/pull/4992#discussion_r1082362233
##########
datafusion/common/src/error.rs:
##########
@@ -359,84 +380,49 @@ impl DataFusionError {
///
/// This may be the same as `self`.
pub fn find_root(&self) -> &Self {
- // Note: This is a non-recursive algorithm so we do not run out of stack space, even for long error chains. The
- // algorithm will always terminate because all steps access the next error through "converging" ownership,
- // i.e. there can be a fan-in by multiple parents (e.g. via `Arc`), but never a fan-out by multiple
- // children (e.g. via `Weak` or interior mutability via `Mutex`).
-
- // last error in the chain that was a DataFusionError
- let mut checkpoint: &Self = self;
-
- // current non-DataFusion error
- let mut other_e: Option<OtherErr<'_>> = None;
-
- loop {
- // do we have another error type to explore?
- if let Some(inner) = other_e {
- // `other_e` is now bound to `inner`, so we can clear this path
- other_e = None;
-
- match inner {
- OtherErr::Arrow(inner) => {
- if let ArrowError::ExternalError(inner) = inner {
- other_e = Some(OtherErr::Dyn(inner.as_ref()));
- continue;
- }
- }
- OtherErr::Dyn(inner) => {
- if let Some(inner) = inner.downcast_ref::<Self>() {
- checkpoint = inner;
- continue;
- }
-
- if let Some(inner) = inner.downcast_ref::<ArrowError>() {
- other_e = Some(OtherErr::Arrow(inner));
- continue;
- }
-
- // some errors are wrapped into `Arc`s to share them with multiple receivers
- if let Some(inner) = inner.downcast_ref::<Arc<Self>>() {
- checkpoint = inner.as_ref();
- continue;
- }
-
- if let Some(inner) = inner.downcast_ref::<Arc<ArrowError>>() {
- other_e = Some(OtherErr::Arrow(inner.as_ref()));
- continue;
- }
- }
- }
-
- // dead end?
- break;
- }
-
- // traverse context chain
- if let Self::Context(_msg, inner) = checkpoint {
- checkpoint = inner;
- continue;
+ // Note: This is a non-recursive algorithm so we do not run
+ // out of stack space, even for long error chains.
+
+ let mut last_datafusion_error = self;
+ let mut root_error: &dyn Error = self;
+ while let Some(source) = find_source(root_error) {
+ // walk the next level
+ root_error = source;
+ // remember the lowest datafusion error so far
+ if let Some(e) = root_error.downcast_ref::<DataFusionError>() {
+ last_datafusion_error = e;
}
-
- // The Arrow error may itself contain a datafusion error again
- // See https://github.com/apache/arrow-datafusion/issues/4172
- if let Self::ArrowError(inner) = checkpoint {
- other_e = Some(OtherErr::Arrow(inner));
- continue;
- }
-
- // also try to introspect direct external errors
- if let Self::External(inner) = checkpoint {
- other_e = Some(OtherErr::Dyn(inner.as_ref()));
- continue;
- }
-
- // no more traversal
- break;
}
-
// return last checkpoint (which may be the original error)
- checkpoint
+ last_datafusion_error
+ }
+}
+
+fn find_source<'a>(e: &'a (dyn Error + 'static)) -> Option<&'a (dyn Error + 'static)> {
+ // workaround until https://github.com/apache/arrow-rs/issues/3566 is released
+ if let Some(e) = e.downcast_ref::<ArrowError>() {
+ return if let ArrowError::ExternalError(e) = e {
+ Some(e.as_ref())
+ } else {
+ None
+ };
}
+ // some errors are wrapped into `Arc`s to share them with multiple
Review Comment:
Is this necessary? The source implementation of Arc should already call this?
https://doc.rust-lang.org/src/alloc/sync.rs.html#2782
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #4992: Implement `std::error::Error::source()` for `DataFusionError`, make `DataFusionError::find_root` more generic
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4992:
URL: https://github.com/apache/arrow-datafusion/pull/4992#discussion_r1082367358
##########
datafusion/common/src/error.rs:
##########
@@ -359,84 +380,49 @@ impl DataFusionError {
///
/// This may be the same as `self`.
pub fn find_root(&self) -> &Self {
- // Note: This is a non-recursive algorithm so we do not run out of stack space, even for long error chains. The
- // algorithm will always terminate because all steps access the next error through "converging" ownership,
- // i.e. there can be a fan-in by multiple parents (e.g. via `Arc`), but never a fan-out by multiple
- // children (e.g. via `Weak` or interior mutability via `Mutex`).
-
- // last error in the chain that was a DataFusionError
- let mut checkpoint: &Self = self;
-
- // current non-DataFusion error
- let mut other_e: Option<OtherErr<'_>> = None;
-
- loop {
- // do we have another error type to explore?
- if let Some(inner) = other_e {
- // `other_e` is now bound to `inner`, so we can clear this path
- other_e = None;
-
- match inner {
- OtherErr::Arrow(inner) => {
- if let ArrowError::ExternalError(inner) = inner {
- other_e = Some(OtherErr::Dyn(inner.as_ref()));
- continue;
- }
- }
- OtherErr::Dyn(inner) => {
- if let Some(inner) = inner.downcast_ref::<Self>() {
- checkpoint = inner;
- continue;
- }
-
- if let Some(inner) = inner.downcast_ref::<ArrowError>() {
- other_e = Some(OtherErr::Arrow(inner));
- continue;
- }
-
- // some errors are wrapped into `Arc`s to share them with multiple receivers
- if let Some(inner) = inner.downcast_ref::<Arc<Self>>() {
- checkpoint = inner.as_ref();
- continue;
- }
-
- if let Some(inner) = inner.downcast_ref::<Arc<ArrowError>>() {
- other_e = Some(OtherErr::Arrow(inner.as_ref()));
- continue;
- }
- }
- }
-
- // dead end?
- break;
- }
-
- // traverse context chain
- if let Self::Context(_msg, inner) = checkpoint {
- checkpoint = inner;
- continue;
+ // Note: This is a non-recursive algorithm so we do not run
+ // out of stack space, even for long error chains.
+
+ let mut last_datafusion_error = self;
+ let mut root_error: &dyn Error = self;
+ while let Some(source) = find_source(root_error) {
+ // walk the next level
+ root_error = source;
+ // remember the lowest datafusion error so far
+ if let Some(e) = root_error.downcast_ref::<DataFusionError>() {
+ last_datafusion_error = e;
}
-
- // The Arrow error may itself contain a datafusion error again
- // See https://github.com/apache/arrow-datafusion/issues/4172
- if let Self::ArrowError(inner) = checkpoint {
- other_e = Some(OtherErr::Arrow(inner));
- continue;
- }
-
- // also try to introspect direct external errors
- if let Self::External(inner) = checkpoint {
- other_e = Some(OtherErr::Dyn(inner.as_ref()));
- continue;
- }
-
- // no more traversal
- break;
}
-
// return last checkpoint (which may be the original error)
- checkpoint
+ last_datafusion_error
+ }
+}
+
+fn find_source<'a>(e: &'a (dyn Error + 'static)) -> Option<&'a (dyn Error + 'static)> {
+ // workaround until https://github.com/apache/arrow-rs/issues/3566 is released
+ if let Some(e) = e.downcast_ref::<ArrowError>() {
+ return if let ArrowError::ExternalError(e) = e {
+ Some(e.as_ref())
+ } else {
+ None
+ };
}
+ // some errors are wrapped into `Arc`s to share them with multiple
Review Comment:
It is necessary, but I don't know why. The tests fail without it. If you could help figure out how to avoid it it would be great.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] ursabot commented on pull request #4992: Implement `std::error::Error::source()` for `DataFusionError`, make `DataFusionError::find_root` more generic
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #4992:
URL: https://github.com/apache/arrow-datafusion/pull/4992#issuecomment-1399537136
Benchmark runs are scheduled for baseline = 350cb47289a76e579b221fe374e4cf09db332569 and contender = e713547fb346e38738ae354a263e801d7be848c1. e713547fb346e38738ae354a263e801d7be848c1 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/fe18153d2e1a4197a8762f5f307929ed...16665763378a4c4086851a708bce6c48/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/cc89423216594d33ab68f1450dbebe29...0778c83572be4401ab65ea7c1eeb3f8d/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/182894c246824cb6bf86250c9548e0b8...5c3f26f9fbc24bf6983b0c3056d5d889/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/46b6275d172a43f88026068f3d434486...3ca94893214e4ef5a7f36f9bd75e31cb/)
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #4992: Implement `std::error::Error::source()` for `DataFusionError`, make `DataFusionError::find_root` more generic
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4992:
URL: https://github.com/apache/arrow-datafusion/pull/4992#discussion_r1081668860
##########
datafusion/common/src/error.rs:
##########
@@ -359,84 +380,49 @@ impl DataFusionError {
///
/// This may be the same as `self`.
pub fn find_root(&self) -> &Self {
- // Note: This is a non-recursive algorithm so we do not run out of stack space, even for long error chains. The
- // algorithm will always terminate because all steps access the next error through "converging" ownership,
- // i.e. there can be a fan-in by multiple parents (e.g. via `Arc`), but never a fan-out by multiple
- // children (e.g. via `Weak` or interior mutability via `Mutex`).
-
- // last error in the chain that was a DataFusionError
- let mut checkpoint: &Self = self;
-
- // current non-DataFusion error
- let mut other_e: Option<OtherErr<'_>> = None;
-
- loop {
- // do we have another error type to explore?
- if let Some(inner) = other_e {
- // `other_e` is now bound to `inner`, so we can clear this path
- other_e = None;
-
- match inner {
- OtherErr::Arrow(inner) => {
- if let ArrowError::ExternalError(inner) = inner {
- other_e = Some(OtherErr::Dyn(inner.as_ref()));
- continue;
- }
- }
- OtherErr::Dyn(inner) => {
- if let Some(inner) = inner.downcast_ref::<Self>() {
- checkpoint = inner;
- continue;
- }
-
- if let Some(inner) = inner.downcast_ref::<ArrowError>() {
- other_e = Some(OtherErr::Arrow(inner));
- continue;
- }
-
- // some errors are wrapped into `Arc`s to share them with multiple receivers
- if let Some(inner) = inner.downcast_ref::<Arc<Self>>() {
- checkpoint = inner.as_ref();
- continue;
- }
-
- if let Some(inner) = inner.downcast_ref::<Arc<ArrowError>>() {
- other_e = Some(OtherErr::Arrow(inner.as_ref()));
- continue;
- }
- }
- }
-
- // dead end?
- break;
- }
-
- // traverse context chain
- if let Self::Context(_msg, inner) = checkpoint {
- checkpoint = inner;
- continue;
+ // Note: This is a non-recursive algorithm so we do not run
+ // out of stack space, even for long error chains.
+
+ let mut last_datafusion_error = self;
+ let mut root_error: &dyn Error = self;
+ while let Some(source) = find_source(root_error) {
Review Comment:
I am quite pleased with how this works (and it will work for other intervening wrapping error types if they implement `source()` as well)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #4992: Implement `std::error::Error::source()` for `DataFusionError`, make `DataFusionError::find_root` more generic
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #4992:
URL: https://github.com/apache/arrow-datafusion/pull/4992
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org