Skip to content

Conversation

@Omega359
Copy link
Contributor

@Omega359 Omega359 commented Feb 5, 2025

Which issue does this PR close?

  • Closes #.

Rationale for this change

Function signature error messages do not include the function name making for debugging issues very difficult in a system with dataframes with hundreds of function calls.

What changes are included in this PR?

Updated error messages and tests.

Are these changes tested?

Yes

Are there any user-facing changes?

Unless you count updated error messages, no.

@github-actions github-actions bot added sql SQL Planner logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) labels Feb 5, 2025
@Omega359 Omega359 marked this pull request as ready for review February 5, 2025 13:54
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.

Thank you 💯 so much @Omega359 -- this is much better.

I made some suggestions to improve the wording of the errors but we can do that as a follow on PR (or never). This PR is already much better than it was

Words better this PR has.

Co-authored-by: Andrew Lamb <[email protected]>
@github-actions github-actions bot added the optimizer Optimizer rules label Feb 5, 2025
Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

I would prefer the function name to be highlighted some how, it can uppercase or square brackets, otherwise the function name got lost in the message

@Omega359
Copy link
Contributor Author

Omega359 commented Feb 5, 2025

I would prefer the function name to be highlighted some how, it can uppercase or square brackets, otherwise the function name got lost in the message

  1. Function test expects NativeType::Numeric but received NativeType::String
  2. Function [test] expects NativeType::Numeric but received NativeType::String
  3. Function TEST expects NativeType::Numeric but received NativeType::String
  4. Function 'TEST' expects NativeType::Numeric but received NativeType::String
  5. Function 'test' expects NativeType::Numeric but received NativeType::String

my preference is the last version.

@comphead
Copy link
Contributor

comphead commented Feb 5, 2025

I would prefer the function name to be highlighted some how, it can uppercase or square brackets, otherwise the function name got lost in the message

  1. Function test expects NativeType::Numeric but received NativeType::String
  2. Function [test] expects NativeType::Numeric but received NativeType::String
  3. Function TEST expects NativeType::Numeric but received NativeType::String
  4. Function 'TEST' expects NativeType::Numeric but received NativeType::String
  5. Function 'test' expects NativeType::Numeric but received NativeType::String

my preference is the last version.

Yeah, love it. Function name is not allowed to include special characters so it should be good

@Omega359
Copy link
Contributor Author

Omega359 commented Feb 5, 2025

Any other opinions on the function name highlight version? I'll wait to do the work till eod today in case there is a good reason not to go with # 5

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

lgtm thanks @Omega359

Copy link
Member

@davisp davisp left a comment

Choose a reason for hiding this comment

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

Just a +1 for going with option #5 on adding single quotes around the function name. Otherwise I'm +1.

@Omega359
Copy link
Contributor Author

Omega359 commented Feb 6, 2025

Updates applied.

@comphead comphead merged commit ad60ffc into apache:main Feb 6, 2025
25 checks passed
@comphead
Copy link
Contributor

comphead commented Feb 6, 2025

Thanks everyone!

@alamb
Copy link
Contributor

alamb commented Feb 6, 2025

Along with this PR from @Lordworms DataFusion error messages are getting downright friendly!

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.

5 participants