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

Replace usages of internal_err with exec_err where appropriate #9241

Merged
merged 8 commits into from
Feb 27, 2024

Conversation

Omega359
Copy link
Contributor

Which issue does this PR close?

Closes #9164

Rationale for this change

Code cleanup.

What changes are included in this PR?

Only internal_err! for incorrect arg count or data type was migrated to exec_err, all other cases were left alone.

Are these changes tested?

Yes,

Are there any user-facing changes?

No.

@github-actions github-actions bot added the physical-expr Physical Expressions label Feb 15, 2024
@Omega359 Omega359 changed the title Feature/internal err 9164 Replace usages of internal err with exec_err where appropriate Feb 15, 2024
@Omega359 Omega359 changed the title Replace usages of internal err with exec_err where appropriate Replace usages of internal_err with exec_err where appropriate Feb 16, 2024
@Omega359 Omega359 marked this pull request as ready for review February 16, 2024 16:17
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.

Thanks @Omega359 for taking this, it looks good for me, the only tiny improvement is to use not_impl_err! for Not Supported Errors

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Feb 20, 2024
@Omega359 Omega359 mentioned this pull request Feb 21, 2024
@github-actions github-actions bot removed the sqllogictest SQL Logic Tests (.slt) label Feb 21, 2024
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 @Omega359 -- this looks great and I think we should get this asap as it will conflict with many of the function extraction PRs.

I took the liberty of merging up from main and resolving one such conflict and plan to merge this when the CI checks have passed

@@ -58,7 +58,7 @@ impl NullIfFunc {
Self {
signature:
Signature::uniform(2, SUPPORTED_NULLIF_TYPES.to_vec(),
Volatility::Immutable,
Volatility::Immutable,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a strange change -- I wonder if cargo fmt doesn't see this file for some reason 🤔

Copy link
Contributor

@alamb alamb Feb 27, 2024

Choose a reason for hiding this comment

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

Ah, I see now #9281. I made a PR #9367 to fix the issue

@alamb alamb merged commit c439bc7 into apache:main Feb 27, 2024
23 checks passed
@Omega359 Omega359 deleted the feature/internal_err_9164 branch February 27, 2024 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
physical-expr Physical Expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Internal_err! is over used in core code and should be replaced with exec_err! or plan_err! in many places
4 participants