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

Incorrect nested error wrapped to ArrowError:External variant for joins #4981

Closed
DDtKey opened this issue Jan 19, 2023 · 2 comments · Fixed by #4983
Closed

Incorrect nested error wrapped to ArrowError:External variant for joins #4981

DDtKey opened this issue Jan 19, 2023 · 2 comments · Fixed by #4983
Labels
bug Something isn't working

Comments

@DDtKey
Copy link
Contributor

DDtKey commented Jan 19, 2023

Describe the bug
This line of code makes impossible to get real reason of error.

.map_err(|e| ArrowError::ExternalError(e.to_string().into())),

It could produce such error: External error: Arrow error: External error: Arrow error: External error: Arrow error: External error: Arrow error: Csv error: ... as a string-message, not a type

That's actually really bad, because under Csv error it could be useful info. But instead of this I receive text error with too many nested External variants.

To Reproduce
Occurrence depends on execution plan and my data that caused this is rather large.
I believe it should be possible to cover with simple test of error type:

    #[tokio::test]
    async fn check_error_nesting() {
        let once_fut = OnceFut::<()>::new(async {
            Err(DataFusionError::ArrowError(ArrowError::CsvError("some error".to_string())))
        });

        struct TestFut(OnceFut<()>);
        impl Future for TestFut {
            type Output = ArrowResult<()>;

            fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
                match ready!((&mut self.0).get(cx)) {
                    Ok(()) => Poll::Ready(Ok(())),
                    Err(e) => Poll::Ready(Err(e)),
                }
            }
        }

        let res = TestFut(once_fut).await;
        let err = res.expect_err("future always return err");
        // ... some checks of error type

    }

Expected behavior
I expect to be able to take error type instead of message in string format.
However, I see that it's reference and need to figure out right way to handle this case. But current one breaks error handling system for datafustion.

Additional context
I would like to fix this behavior

And it probably make sense to request implementing find_root for arrow (like exists for datafusion error) - but that's out of scope

@DDtKey DDtKey added the bug Something isn't working label Jan 19, 2023
@DDtKey DDtKey changed the title Incorrect nested error wrapped to ArrowError:External variant Incorrect nested error wrapped to ArrowError:External variant for joins Jan 19, 2023
@alamb
Copy link
Contributor

alamb commented Jan 19, 2023

Possibly related #4991 and apache/arrow-rs#3567

@DDtKey
Copy link
Contributor Author

DDtKey commented Jan 19, 2023

@alamb don't think so, check #4983, I made a fix. It had to_string() usage, so it was impossible to downcast error to relevant error type

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants