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

fix: Avoid re-wrapping planning errors Err(DataFusionError::Plan) for use in plan_datafusion_err #14000

Merged

Conversation

avkirilishin
Copy link
Contributor

@avkirilishin avkirilishin commented Jan 3, 2025

Which issue does this PR close?

Closes #13979.

Are these changes tested?

I added several new unit tests in sql_integrations, despite the fact that some cases are already covered in sqllogictest. It was also manually tested (removed the duplicated 'Error during planning'; no changes made to 'Error during planning: Execution error:' in the third case):

BeforeAfter
> select version(1);
Error during planning: Error during planning:
Failed to coerce arguments to satisfy a call
to version function: coercion from [Int64] to
the signature Exact([]) failed. No function
matches the given name and argument
types 'version(Int64)'. You might need to
add explicit type casts.
> select version(1);
Error during planning: Failed to coerce
arguments to satisfy a call to version
function: coercion from [Int64] to the
signature Exact([]) failed. No function
matches the given name and argument
types 'version(Int64)'. You might need
to add explicit type casts.
> select rank(1) over();
Error during planning: Error during planning:
The function expected zero argument but
received 1 No function matches the given
name and argument types 'rank(Int64)'. You
might need to add explicit type casts.
> select rank(1) over();
Error during planning: The function expected
zero argument but received 1 No function
matches the given name and argument types
'rank(Int64)'. You might need to add explicit
type casts.
> select max(1,2);
Error during planning: Execution error:
User-defined coercion failed with
Execution("min/max was called with 2
arguments. It requires only 1.") No
function matches the given name and
argument types 'max(Int64, Int64)'. You
might need to add explicit type casts.
> select max(1,2);
Error during planning: Execution error:
User-defined coercion failed with
Execution("min/max was called with 2
arguments. It requires only 1.") No
function matches the given name and
argument types 'max(Int64, Int64)'. You
might need to add explicit type casts.

Are there any user-facing changes?

Although the output of some errors will change (improve), changes in the documentation are probably not necessary.

@github-actions github-actions bot added the logical-expr Logical plan and expressions label Jan 3, 2025
@avkirilishin avkirilishin changed the title fix: unwrapping Err(DataFusionError::Plan) for use in plan_datafusion_err [WIP] fix: unwrapping Err(DataFusionError::Plan) for use in plan_datafusion_err Jan 4, 2025
@avkirilishin avkirilishin force-pushed the fix-double-display-of-plan-error-prefix-13979 branch from 057943d to d8193e6 Compare January 4, 2025 13:38
@avkirilishin avkirilishin force-pushed the fix-double-display-of-plan-error-prefix-13979 branch from d8193e6 to a730194 Compare January 4, 2025 13:46
@github-actions github-actions bot added sql SQL Planner optimizer Optimizer rules labels Jan 4, 2025
@avkirilishin avkirilishin force-pushed the fix-double-display-of-plan-error-prefix-13979 branch from a730194 to b41ba4c Compare January 4, 2025 13:53
@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jan 4, 2025
@avkirilishin avkirilishin force-pushed the fix-double-display-of-plan-error-prefix-13979 branch from b41ba4c to 4e625e6 Compare January 4, 2025 14:01
@avkirilishin avkirilishin reopened this Jan 4, 2025
@avkirilishin avkirilishin changed the title [WIP] fix: unwrapping Err(DataFusionError::Plan) for use in plan_datafusion_err fix: unwrapping Err(DataFusionError::Plan) for use in plan_datafusion_err Jan 4, 2025
@alamb alamb changed the title fix: unwrapping Err(DataFusionError::Plan) for use in plan_datafusion_err fix: Avoid re-wrapping planning errors Err(DataFusionError::Plan) for use in plan_datafusion_err Jan 4, 2025
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @avkirilishin -- this looks like a very nice improvement to me to avoid double wrapping the errors

@alamb alamb merged commit 42dce6e into apache:main Jan 5, 2025
28 checks passed
@alamb
Copy link
Contributor

alamb commented Jan 5, 2025

Thanks again @avkirilishin

@niebayes
Copy link
Contributor

niebayes commented Jan 6, 2025

Great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logical-expr Logical plan and expressions optimizer Optimizer rules sql SQL Planner sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

datafusion-cli displays error prefix twice
3 participants