You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "getChan (via GitHub)" <gi...@apache.org> on 2025/01/30 13:44:44 UTC
[PR] Fix: Avoid recursive external error wrapping [datafusion]
getChan opened a new pull request, #14371:
URL: https://github.com/apache/datafusion/pull/14371
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #14080
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
yes
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix: Avoid recursive external error wrapping [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14371:
URL: https://github.com/apache/datafusion/pull/14371#issuecomment-2643347234
Thanks again @getChan
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix: Avoid recursive external error wrapping [datafusion]
Posted by "getChan (via GitHub)" <gi...@apache.org>.
getChan commented on code in PR #14371:
URL: https://github.com/apache/datafusion/pull/14371#discussion_r1938297005
##########
datafusion/sqllogictest/test_files/errors.slt:
##########
@@ -161,3 +161,12 @@ create table records (timestamp timestamp, value float) as values (
'2021-01-01 00:00:00', 1.0,
'2021-01-01 00:00:00', 2.0
);
+
+statement ok
+CREATE TABLE tab0(col0 INTEGER, col1 INTEGER, col2 INTEGER);
+
+statement ok
+INSERT INTO tab0 VALUES(83,0,38);
+
+query error DataFusion error: Arrow error: Divide by zero error
+SELECT DISTINCT - 84 FROM tab0 AS cor0 WHERE NOT + 96 / + col1 <= NULL GROUP BY col1, col0;
Review Comment:
- AS-IS : `query error DataFusion error: External error: External error: External error: Arrow error: Divide by zero error`
- An ExternalError is added to each RepartitionExec.
<img width="861" alt="스크린샷 2025-02-02 오전 12 05 06" src="https://github.com/user-attachments/assets/24541370-e6c1-4e3e-b218-ef27f395efd3" />
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix: Avoid recursive external error wrapping [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14371:
URL: https://github.com/apache/datafusion/pull/14371#discussion_r1942700622
##########
datafusion/common/src/error.rs:
##########
@@ -131,6 +131,10 @@ pub enum DataFusionError {
/// Errors from either mapping LogicalPlans to/from Substrait plans
/// or serializing/deserializing protobytes to Substrait plans
Substrait(String),
+
+ /// Errors for wrapping other errors.
Review Comment:
Thank you @getChan -- As DataFusionError is used all over the place and is a very visible part of the API I was thinking it would be best if we can avoid adding too many variants. However, after playing with some other options (like deep cloning the error) I agree that this solution is the best.
to be consistent with the other variants which don't have a `Error` in their name, I renamed this to `DataFusionError::Shared`
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix: Avoid recursive external error wrapping [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14371:
URL: https://github.com/apache/datafusion/pull/14371#issuecomment-2637543568
Marking as an API change as this will result in downstream users having to handle a new DataFusionError variants (which is fine, I just want to correctly capture the idea)
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix: Avoid recursive external error wrapping [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #14371:
URL: https://github.com/apache/datafusion/pull/14371
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix: Avoid recursive external error wrapping [datafusion]
Posted by "getChan (via GitHub)" <gi...@apache.org>.
getChan commented on code in PR #14371:
URL: https://github.com/apache/datafusion/pull/14371#discussion_r1938313572
##########
datafusion/common/src/error.rs:
##########
@@ -131,6 +131,10 @@ pub enum DataFusionError {
/// Errors from either mapping LogicalPlans to/from Substrait plans
/// or serializing/deserializing protobytes to Substrait plans
Substrait(String),
+
+ /// Errors for wrapping other errors.
Review Comment:
Because if we only pass the underlying error, we can't share the error (due to ownership constraints). For example, `RepartitionExec` needs to send errors to all output partitions. You can see this in the source code.
https://github.com/apache/datafusion/blob/8d006a287c653e8305827ad909c407baa7ed59fd/datafusion/physical-plan/src/repartition/mod.rs#L914-L921
So, how do we share the error?
1. `Implement Clone for DataFusionError` and use `err.clone()`: This is not possible because nested Error types do not implement the`Clone` trait.
2. Pass `Arc<DatafusionError>` : I implemented it this way.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix: Avoid recursive external error wrapping [datafusion]
Posted by "getChan (via GitHub)" <gi...@apache.org>.
getChan commented on code in PR #14371:
URL: https://github.com/apache/datafusion/pull/14371#discussion_r1938313572
##########
datafusion/common/src/error.rs:
##########
@@ -131,6 +131,10 @@ pub enum DataFusionError {
/// Errors from either mapping LogicalPlans to/from Substrait plans
/// or serializing/deserializing protobytes to Substrait plans
Substrait(String),
+
+ /// Errors for wrapping other errors.
Review Comment:
Because if we only pass the underlying error, we can't share the error (due to ownership constraints). For example, `RepartitionExec` needs to send errors to all output partitions. You can see this in the source code.
https://github.com/apache/datafusion/blob/8d006a287c653e8305827ad909c407baa7ed59fd/datafusion/physical-plan/src/repartition/mod.rs#L914-L921
To share the error, I considered the following.
1. `Implement Clone for DataFusionError` and use `err.clone()`: This is not possible because nested Error types do not implement the`Clone` trait.
2. Pass `Arc<DatafusionError>` : I implemented it this way.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix: Avoid recursive external error wrapping [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14371:
URL: https://github.com/apache/datafusion/pull/14371#issuecomment-2643351218
The extended tests are going to fail on main until we merge https://github.com/apache/datafusion-testing/pull/6 and update the testing pin
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix: Avoid recursive external error wrapping [datafusion]
Posted by "getChan (via GitHub)" <gi...@apache.org>.
getChan commented on code in PR #14371:
URL: https://github.com/apache/datafusion/pull/14371#discussion_r1938313572
##########
datafusion/common/src/error.rs:
##########
@@ -131,6 +131,10 @@ pub enum DataFusionError {
/// Errors from either mapping LogicalPlans to/from Substrait plans
/// or serializing/deserializing protobytes to Substrait plans
Substrait(String),
+
+ /// Errors for wrapping other errors.
Review Comment:
Because if we only pass the underlying error, we can't share the error (due to ownership constraints). For example, `RepartitionExec` needs to send errors to all output partitions. You can see this in the source code.
https://github.com/apache/datafusion/blob/8d006a287c653e8305827ad909c407baa7ed59fd/datafusion/physical-plan/src/repartition/mod.rs#L914-L921
So, how do we share the error?
1. `Implement Clone for DataFusionError`: This is not possible because nested Error types do not implement the`Clone` trait.
2. Pass `Arc<DatafusionError>` : I implemented it this way.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix: Avoid recursive external error wrapping [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14371:
URL: https://github.com/apache/datafusion/pull/14371#issuecomment-2636865489
I double checked and we will have to update some expected error messages to get the extended tests passing too.
I made a PR here:
- https://github.com/apache/datafusion-testing/pull/6
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix: Avoid recursive external error wrapping [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14371:
URL: https://github.com/apache/datafusion/pull/14371#discussion_r1938310453
##########
datafusion/common/src/error.rs:
##########
@@ -131,6 +131,10 @@ pub enum DataFusionError {
/// Errors from either mapping LogicalPlans to/from Substrait plans
/// or serializing/deserializing protobytes to Substrait plans
Substrait(String),
+
+ /// Errors for wrapping other errors.
Review Comment:
Why is this needed (compared to `Context` for example?) I don't understand why we need to keep the data wrappeed (why don't we just pass the underlying error back?)
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix: Avoid recursive external error wrapping [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14371:
URL: https://github.com/apache/datafusion/pull/14371#issuecomment-2643730192
> The extended tests are going to fail on main until we merge [apache/datafusion-testing#6](https://github.com/apache/datafusion-testing/pull/6) and update the testing pin
Filed https://github.com/apache/datafusion/issues/14549
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix: Avoid recursive external error wrapping [datafusion]
Posted by "getChan (via GitHub)" <gi...@apache.org>.
getChan commented on code in PR #14371:
URL: https://github.com/apache/datafusion/pull/14371#discussion_r1938297005
##########
datafusion/sqllogictest/test_files/errors.slt:
##########
@@ -161,3 +161,12 @@ create table records (timestamp timestamp, value float) as values (
'2021-01-01 00:00:00', 1.0,
'2021-01-01 00:00:00', 2.0
);
+
+statement ok
+CREATE TABLE tab0(col0 INTEGER, col1 INTEGER, col2 INTEGER);
+
+statement ok
+INSERT INTO tab0 VALUES(83,0,38);
+
+query error DataFusion error: Arrow error: Divide by zero error
+SELECT DISTINCT - 84 FROM tab0 AS cor0 WHERE NOT + 96 / + col1 <= NULL GROUP BY col1, col0;
Review Comment:
AS-IS
- `query error DataFusion error: External error: External error: External error: Arrow error: Divide by zero error`
- An ExternalError is added to each RepartitionExec.
<img width="861" alt="스크린샷 2025-02-02 오전 12 05 06" src="https://github.com/user-attachments/assets/24541370-e6c1-4e3e-b218-ef27f395efd3" />
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix: Avoid recursive external error wrapping [datafusion]
Posted by "getChan (via GitHub)" <gi...@apache.org>.
getChan commented on code in PR #14371:
URL: https://github.com/apache/datafusion/pull/14371#discussion_r1938315198
##########
datafusion/common/src/error.rs:
##########
@@ -131,6 +131,10 @@ pub enum DataFusionError {
/// Errors from either mapping LogicalPlans to/from Substrait plans
/// or serializing/deserializing protobytes to Substrait plans
Substrait(String),
+
+ /// Errors for wrapping other errors.
Review Comment:
If using `Context`, the underlying error is only passed as a single `Result` due to ownership constraints.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix: Avoid recursive external error wrapping [datafusion]
Posted by "getChan (via GitHub)" <gi...@apache.org>.
getChan commented on code in PR #14371:
URL: https://github.com/apache/datafusion/pull/14371#discussion_r1938636466
##########
datafusion/common/src/error.rs:
##########
@@ -131,6 +131,10 @@ pub enum DataFusionError {
/// Errors from either mapping LogicalPlans to/from Substrait plans
/// or serializing/deserializing protobytes to Substrait plans
Substrait(String),
+
+ /// Errors for wrapping other errors.
Review Comment:
I renamed it to `SharedError`
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org