Fix: Avoid recursive external error wrapping #14371

getChan · 2025-01-30T13:44:41Z

Which issue does this PR close?

Closes External Error prefix is repeated multiple times #14080

Rationale for this change

cause of the issue

In RepartitionExec, it was necessary to wrap the DatafusionError to send it across multiple partitions. However due to the absence of an appropriate error clone method, ExternalError was used. This resulted in a recursive ExternalError.

datafusion/datafusion/physical-plan/src/repartition/mod.rs

Lines 918 to 920 in 8d006a2

    
           // wrap it because need to send error to all output partitions 
        
           let err = Err(DataFusionError::External(Box::new(Arc::clone(&e)))); 
        
           tx.send(Some(err)).await.ok();

What changes are included in this PR?

new DatafusionError variant SharedError for sharing source DatafusionError
1. I couldn't think of an appropriate way to clone source error. If you have any ideas, please let me know.
Replace ExternalError with SharedError in RepartitionExec/JoinExec
Avoid recursive external error wrapping in type conversion.

Are these changes tested?

yes

Are there any user-facing changes?

error message will be changed.

This reverts commit 127705e.

getChan · 2025-02-01T15:56:46Z

datafusion/sqllogictest/test_files/errors.slt

+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;


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.

alamb

Thanks @getChan !

alamb · 2025-02-01T17:19:03Z

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.


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?)

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.

datafusion/datafusion/physical-plan/src/repartition/mod.rs

Lines 914 to 921 in 8d006a2

Ok(Err(e)) => {

let e = Arc::new(e);

for (_, tx) in txs {

// wrap it because need to send error to all output partitions

let err = Err(DataFusionError::External(Box::new(Arc::clone(&e))));

tx.send(Some(err)).await.ok();

}

To share the error, I considered the following.

Implement Clone for DataFusionError and use err.clone(): This is not possible because nested Error types do not implement theClone trait.

Pass Arc<DatafusionError> : I implemented it this way.

If using Context, the underlying error is only passed as a single Result due to ownership constraints.

I renamed it to SharedError

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

# Conflicts: # datafusion/common/src/error.rs

alamb

Thank you very much @getChan -- this is a nice usability improvement and I think it is ready to go. It was very nicely done and much appreciated.

I took the liberty of merging this PR up from main and updating some documentation so this is ready to merge.

Thanks again!

alamb · 2025-02-05T11:28:20Z

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.


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

alamb · 2025-02-05T13:32:15Z

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:

Update expected error messages after upstream change datafusion-testing#6

alamb · 2025-02-05T17:14:41Z

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)

alamb · 2025-02-07T16:07:15Z

Thanks again @getChan

alamb · 2025-02-07T16:09:00Z

The extended tests are going to fail on main until we merge apache/datafusion-testing#6 and update the testing pin

alamb · 2025-02-07T18:52:05Z

The extended tests are going to fail on main until we merge apache/datafusion-testing#6 and update the testing pin

Filed Extended tests are failing on main #14549

Fix: Avoid recursive external error wrapping in type conversion.

8a895d5

getChan marked this pull request as draft January 30, 2025 13:44

github-actions bot added the common Related to common crate label Jan 30, 2025

getChan added 2 commits January 30, 2025 22:46

cargo fmt

dc9dd5e

test TableProvider

127705e

github-actions bot added the core Core DataFusion crate label Jan 31, 2025

Revert "test TableProvider"

3723ec5

This reverts commit 127705e.

github-actions bot removed the core Core DataFusion crate label Jan 31, 2025

getChan added 4 commits February 2, 2025 00:23

create WrappedError

5d480a4

Replace ExternalError with WrappedError in RepartitionExec

05293c5

Replace ExternalError with WrappedError in Join Exec

4dc6452

sqllogictest

f3f5b14

github-actions bot added physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt) labels Feb 1, 2025

getChan commented Feb 1, 2025

View reviewed changes

getChan marked this pull request as ready for review February 1, 2025 16:04

alamb reviewed Feb 1, 2025

View reviewed changes

Merge branch 'main' into recursive-external-err

f1c34af

# Conflicts: # datafusion/common/src/error.rs

github-actions bot added the optimizer Optimizer rules label Feb 3, 2025

rename SharedError

d4e860f

getChan force-pushed the recursive-external-err branch from 88233aa to d4e860f Compare February 3, 2025 00:46

github-actions bot removed the optimizer Optimizer rules label Feb 3, 2025

alamb added 3 commits February 5, 2025 06:29

Update comments and rename to DataFusionError::Shared

63de9b7

Merge remote-tracking branch 'apache/main' into recursive-external-err

8907e51

Improve API

b671378

alamb approved these changes Feb 5, 2025

View reviewed changes

alamb added 2 commits February 5, 2025 07:34

fix clippy

e390b97

Merge remote-tracking branch 'apache/main' into recursive-external-err

7e60e6c

This was referenced Feb 5, 2025

Script and documentation for regenerating sqlite test files #14290

Merged

Update expected error messages after upstream change apache/datafusion-testing#6

Merged

alamb added the api change Changes the API exposed to users of the crate label Feb 5, 2025

Merge remote-tracking branch 'apache/main' into recursive-external-err

285f61f

alamb merged commit c234833 into apache:main Feb 7, 2025
25 checks passed

alamb mentioned this pull request Feb 7, 2025

Extended tests are failing on main #14549

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Avoid recursive external error wrapping #14371

Fix: Avoid recursive external error wrapping #14371

getChan commented Jan 30, 2025 •

edited by alamb

Loading

getChan Feb 1, 2025 •

edited

Loading

alamb left a comment

alamb Feb 1, 2025

getChan Feb 1, 2025 •

edited

Loading

getChan Feb 1, 2025

getChan Feb 3, 2025

alamb Feb 5, 2025

alamb left a comment

alamb Feb 5, 2025

alamb commented Feb 5, 2025

alamb commented Feb 5, 2025

alamb commented Feb 7, 2025

alamb commented Feb 7, 2025

alamb commented Feb 7, 2025 •

edited

Loading

	// wrap it because need to send error to all output partitions
	let err = Err(DataFusionError::External(Box::new(Arc::clone(&e))));
	tx.send(Some(err)).await.ok();

	Ok(Err(e)) => {
	let e = Arc::new(e);

	for (_, tx) in txs {
	// wrap it because need to send error to all output partitions
	let err = Err(DataFusionError::External(Box::new(Arc::clone(&e))));
	tx.send(Some(err)).await.ok();
	}

Fix: Avoid recursive external error wrapping #14371

Fix: Avoid recursive external error wrapping #14371

Conversation

getChan commented Jan 30, 2025 • edited by alamb Loading

Which issue does this PR close?

Rationale for this change

cause of the issue

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

getChan Feb 1, 2025 • edited Loading

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Feb 1, 2025

Choose a reason for hiding this comment

getChan Feb 1, 2025 • edited Loading

Choose a reason for hiding this comment

getChan Feb 1, 2025

Choose a reason for hiding this comment

getChan Feb 3, 2025

Choose a reason for hiding this comment

alamb Feb 5, 2025

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Feb 5, 2025

Choose a reason for hiding this comment

alamb commented Feb 5, 2025

alamb commented Feb 5, 2025

alamb commented Feb 7, 2025

alamb commented Feb 7, 2025

alamb commented Feb 7, 2025 • edited Loading

getChan commented Jan 30, 2025 •

edited by alamb

Loading

getChan Feb 1, 2025 •

edited

Loading

getChan Feb 1, 2025 •

edited

Loading

alamb commented Feb 7, 2025 •

edited

Loading