Add `DataFusionError::Collection` to return multiple `DataFusionError`s #14439

eliaperantoni · 2025-02-03T13:19:49Z

This PR adds DataFusionError::Collection so that multiple errors can be returned at once, shortening the feedback loop for developers because they would now be able to fix more than error in their SQL query before trying to run it again.

Example:

All of these errors are returned in a single run:

For now, I just implemented this when:

Planning different SelectItems
Planning different SetExprs

Which issue does this PR close?

Closes #13676.

Rationale for this change

See #13676.

What changes are included in this PR?

Add DataFusionError::Collection
Add DataFusionError::iter
Add tests
Collect multiple errors, instead of returning at the first one, when iterating over SelectItems and SetExprs

Are these changes tested?

Yes, see datafusion/sql/tests/cases/collection.rs

Are there any user-facing changes?

There is one opt-in addition: DataFusionError::iter.

eliaperantoni · 2025-02-03T15:19:53Z

@alamb pr is ready for review. I'm getting this weird error that appears to be because of the TTY size being smaller in CI, making the error wrap on a new line? I can't reproduce it locally.

eliaperantoni · 2025-02-04T10:13:08Z

datafusion/sqllogictest/src/engines/datafusion_engine/error.rs

-    #[error("DataFusion error: {0}")]
+    #[error("DataFusion error: {}", .0.strip_backtrace())]


Okay so it turns out that the issue was that the expected error doesn't contain the backtrace but the actual one does. I couldn't reproduce locally at first because I wasn't using the backtrace feature.

I have no idea why running with --complete didn't write the backtrace to the .slt files, I find no call to strip_backtrace and I don't know what in my PR made this necessary. But it works 🤷‍♂️

I don't think we should add the backtrace to the errors in general (definitely not in slt) as the backtraces would include line numbers / call stacks that would change over time.

Absolutely. This PR doesn't change that, the backtraces are in the wrapped SchemaError and PlanError. I had to strip them away, don't know why that wasn't necessary before

eliaperantoni · 2025-02-04T10:14:22Z

datafusion/sqllogictest/test_files/errors.slt

-query error No function matches
+query error DataFusion error: Error during planning: Execution error: User\-defined coercion failed with "Error during planning: The substr function requires 2 or 3 arguments, but got 1\."
 select 1 group by substr('');


I don't know what caused this change honestly.

comphead · 2025-02-04T19:01:55Z

Thanks @eliaperantoni
What comes to my mind is the PR might make the output too cumbersome, and some time back I think we already faced the issue that extra attention to error can slow down the planning time. I checked bunch of engines now, like DuckDB, Trino they return errors 1 by 1 which is not necessarily correct of course

mkarbo · 2025-02-04T21:13:57Z

Thanks @eliaperantoni

What comes to my mind is the PR might make the output too cumbersome, and some time back I think we already faced the issue that extra attention to error can slow down the planning time. I checked bunch of engines now, like DuckDB, Trino they return errors 1 by 1 which is not necessarily correct of course

Just adding some motivation behind this

For instance, if you want to use diagnostics to power applications such as static validators or LSP's, then it will be necessary to report all errors.

Going from 1->n will open up a new set of applications of diagnostics not easily solvable outside of this ecosystem, which I would say is very valuable to the community, if we can make it happen :)

eliaperantoni · 2025-02-04T21:45:04Z

Thanks @comphead for your feedback. We appreciate it 🙏

What comes to my mind is the PR might make the output too cumbersome

The actual error output doesn't change at all. i.e. if you get a DataFusionError and you use any of the methods from std::error::Error it'll still work the same way, exception made for the X errors, first one is: {err} prefix for the Display implementation.

It only changes if you explicitly call .iter, which I think is a very intentional act of extracting additional information from the error. But we took care to not make it cumbersome by default and to not make breaking changes.

extra attention to error can slow down the planning time

I think that's fair criticism. What could we do to solve this in a way that doesn't complicate the code too much? I think it's also important to notice that the performance impact occurs only when there are errors, and it would be greater than without this PR only if the errors are more than 1. This is because the only performance lost is due to the planner continuing after the first error. In performance critical applications that run the same carefully crafted queries, no extra code is executed and no extra allocations are made.

alamb · 2025-02-05T17:12:55Z

Thanks @comphead for your feedback. We appreciate it 🙏

What comes to my mind is the PR might make the output too cumbersome

The actual error output doesn't change at all. i.e. if you get a DataFusionError and you use any of the methods from std::error::Error it'll still work the same way, exception made for the X errors, first one is: {err} prefix for the Display implementation.

Yeah, I think this makes sense -- by default show the first error message unless the application wants to show more of them

It only changes if you explicitly call .iter, which I think is a very intentional act of extracting additional information from the error. But we took care to not make it cumbersome by default and to not make breaking changes.

extra attention to error can slow down the planning time

I think that's fair criticism. What could we do to solve this in a way that doesn't complicate the code too much? I think it's also important to notice that the performance impact occurs only when there are errors, and it would be greater than without this PR only if the errors are more than 1.

some time back I think we already faced the issue that extra attention to error can slow down the planning time.

This is an excellent point @comphead and I agree with @eliaperantoni 's analysis that the additional overhead will happen when the query was going to error anyways so I don't expect this to make a large difference

What we were seeing was that when planning created Err(DataFusionError::...) during normal (non erroring queries) it was quite a bit slower

alamb · 2025-02-05T17:13:49Z

Marking as an API change as the new enum will need to be handled by downstream users

alamb

Thanks @eliaperantoni -- this looks great to me -- my only concern is the wrapping of single errors with Collection. Otherwise I think it is good to go 👍

I left some other comments as well

datafusion/common/src/error.rs

alamb · 2025-02-05T17:26:12Z

datafusion/common/src/error.rs

+    pub fn iter(&self) -> impl Iterator<Item = &DataFusionError> {
+        struct ErrorIterator<'a> {
+            queue: VecDeque<&'a DataFusionError>,
+        }
+
+        impl<'a> Iterator for ErrorIterator<'a> {
+            type Item = &'a DataFusionError;
+
+            fn next(&mut self) -> Option<Self::Item> {
+                loop {
+                    let popped = self.queue.pop_front()?;
+                    match popped {
+                        DataFusionError::Collection(errs) => self.queue.extend(errs),
+                        _ => return Some(popped),
+                    }
+                }
+            }
+        }
+
+        let mut queue = VecDeque::new();
+        queue.push_back(self);
+        ErrorIterator { queue }
+    }


I think you can implement the same thing more simply via

pub fn iter(&self) -> impl Iterator<Item = &DataFusionError> { let mut current_err = self; let errors: Vec<_> = match self { DataFusionError::Collection(errs) => errs.iter().collect(), _ => vec![self], }; errors.into_iter() }

That doesn't handle recursive Collections but I think that is ok

That doesn't handle recursive Collections but I think that is ok

But then in a query like:

SELECT bad, bad UNION SELECT bad

You would get one DataFusionError::Collection and one DataFusionError::Plan whereas you could've gotten three DataFusionError::Plan. I think that's worth having this iter that's slightly more complicated.

datafusion/sql/src/select.rs

datafusion/sql/tests/cases/collection.rs

alamb

Thanks @eliaperantoni -- this looks great to me

alamb · 2025-02-07T18:41:39Z

datafusion/common/src/error.rs

+    fn test_iter() {
+        let err = DataFusionError::Collection(vec![
+            DataFusionError::Plan("a".to_string()),
+            DataFusionError::Collection(vec![


💯 for the recursive check

alamb · 2025-02-07T18:48:09Z

I merged up from main to resolve a conflict

alamb · 2025-02-07T21:01:41Z

datafusion/common/src/error.rs

+    }
+}
+
+pub struct DataFusionErrorBuilder(Vec<DataFusionError>);


I made a follow on PR to add docs and examples:

Minor: Add docs and examples for DataFusionErrorBuilder #14551

alamb · 2025-02-07T21:22:44Z

I am really sorry -- somehow I have messed up the tests on this PR. I will monkey around with them to get them passing 🐒 🤔

alamb · 2025-02-07T22:16:27Z

Thanks again @eliaperantoni

eliaperantoni · 2025-02-07T22:28:40Z

Thanks again @eliaperantoni

Thank you @alamb! It was very kind of you to fix the test 😊

alamb · 2025-02-07T22:36:30Z

Thanks again @eliaperantoni

Thank you @alamb! It was very kind of you to fix the test 😊

I don't really know why it took so much finagling to be honest 🤷

eliaperantoni added 5 commits February 3, 2025 13:51

feat: collect multiple errors when possible

184f7e6

chore: format

ccf36af

doc: explain implementation of DataFusionError::{message,source}

1b287ed

feat: update cli lockfile

ef71a85

doc: explain DataFusionError::iter

8141f23

github-actions bot added sql SQL Planner common Related to common crate labels Feb 3, 2025

eliaperantoni added 2 commits February 3, 2025 14:21

chore: cargo fmt --all

6ab8b36

fix: test

4eae829

github-actions bot added the core Core DataFusion crate label Feb 3, 2025

chore: update slt files

f642b82

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Feb 3, 2025

fix: tests

9cd2e12

fix: strip backtrace in slt tests

e2de823

github-actions bot added the logical-expr Logical plan and expressions label Feb 4, 2025

eliaperantoni commented Feb 4, 2025

View reviewed changes

eliaperantoni mentioned this pull request Feb 4, 2025

Project Ideas for GSoC 2025 (Google Summer of Code) #14478

Open

alamb changed the title ~~Collection of errors~~ Support returning multiple DataFusionErrors Feb 5, 2025

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

alamb changed the title ~~Support returning multiple DataFusionErrors~~ Add DataFusionError::Collection to return multiple DataFusionErrors Feb 5, 2025

alamb reviewed Feb 5, 2025

View reviewed changes

eliaperantoni added 2 commits February 6, 2025 08:34

feat: pr feedback

fbe8b5d

Merge branch 'main' into eper/error-collection

5812715

github-actions bot removed the core Core DataFusion crate label Feb 6, 2025

eliaperantoni requested a review from alamb February 6, 2025 07:50

eliaperantoni added 4 commits February 7, 2025 09:37

chore: fix clippy

eebf9e8

Merge branch 'main' into eper/error-collection

7b0e7f4

fix: test

337ea50

Merge branch 'main' into eper/error-collection

28f351a

alamb approved these changes Feb 7, 2025

View reviewed changes

Merge remote-tracking branch 'apache/main' into eper/error-collection

e3affd7

alamb added 4 commits February 7, 2025 13:52

update expected

984d533

more update

451bd71

revert test in errors

d10c77b

Merge remote-tracking branch 'apache/main' into eper/error-collection

415029f

alamb mentioned this pull request Feb 7, 2025

Minor: Add docs and examples for DataFusionErrorBuilder #14551

Open

alamb reviewed Feb 7, 2025

View reviewed changes

try again

b8cf6c9

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `DataFusionError::Collection` to return multiple `DataFusionError`s #14439

Add `DataFusionError::Collection` to return multiple `DataFusionError`s #14439

eliaperantoni commented Feb 3, 2025 •

edited

Loading

eliaperantoni commented Feb 3, 2025

eliaperantoni Feb 4, 2025

alamb Feb 5, 2025

eliaperantoni Feb 5, 2025

eliaperantoni Feb 4, 2025

comphead commented Feb 4, 2025

mkarbo commented Feb 4, 2025

eliaperantoni commented Feb 4, 2025

alamb commented Feb 5, 2025

alamb commented Feb 5, 2025

alamb left a comment

alamb Feb 5, 2025

eliaperantoni Feb 5, 2025 •

edited

Loading

alamb left a comment

alamb Feb 7, 2025

alamb commented Feb 7, 2025

alamb Feb 7, 2025

alamb commented Feb 7, 2025

alamb commented Feb 7, 2025

eliaperantoni commented Feb 7, 2025

alamb commented Feb 7, 2025

		#[error("DataFusion error: {0}")]
		#[error("DataFusion error: {}", .0.strip_backtrace())]

Add DataFusionError::Collection to return multiple DataFusionErrors #14439

Add DataFusionError::Collection to return multiple DataFusionErrors #14439

Conversation

eliaperantoni commented Feb 3, 2025 • edited Loading

Example:

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

eliaperantoni commented Feb 3, 2025

eliaperantoni Feb 4, 2025

Choose a reason for hiding this comment

alamb Feb 5, 2025

Choose a reason for hiding this comment

eliaperantoni Feb 5, 2025

Choose a reason for hiding this comment

eliaperantoni Feb 4, 2025

Choose a reason for hiding this comment

comphead commented Feb 4, 2025

mkarbo commented Feb 4, 2025

eliaperantoni commented Feb 4, 2025

alamb commented Feb 5, 2025

alamb commented Feb 5, 2025

alamb left a comment

Choose a reason for hiding this comment

alamb Feb 5, 2025

Choose a reason for hiding this comment

eliaperantoni Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Feb 7, 2025

Choose a reason for hiding this comment

alamb commented Feb 7, 2025

alamb Feb 7, 2025

Choose a reason for hiding this comment

alamb commented Feb 7, 2025

alamb commented Feb 7, 2025

eliaperantoni commented Feb 7, 2025

alamb commented Feb 7, 2025

Add `DataFusionError::Collection` to return multiple `DataFusionError`s #14439

Add `DataFusionError::Collection` to return multiple `DataFusionError`s #14439

eliaperantoni commented Feb 3, 2025 •

edited

Loading

eliaperantoni Feb 5, 2025 •

edited

Loading