Skip to content

Fix NULL Arithmetic Handling for Numerical Operators in Type Coercion #17418

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Fix NULL Arithmetic Handling for Numerical Operators in Type Coercion #17418

wants to merge 7 commits into from
+22 −6

Conversation

etolbakov
Copy link
Contributor

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?

@github-actions github-actions bot added the logical-expr Logical plan and expressions label Sep 4, 2025
@etolbakov
Copy link
Contributor Author

etolbakov commented Sep 4, 2025 •
edited
Loading

Hey @2010YOUY01
I was curious to investigate this issue, and this tweak appears to resolve it (tried locally).
Could you please tell me if the approach looks fine?

This is my local testing:
image

@2010YOUY01
Copy link
Contributor

2010YOUY01 commented Sep 5, 2025 •
edited
Loading

I'm not familiar with the related code at this moment, but

  1. If this code path is only used for null literals like NULL % NULL
  2. No more other hack changes to pass the tests

I think this change would be good.

@etolbakov etolbakov marked this pull request as ready for review September 7, 2025 12:49
@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Sep 7, 2025
@etolbakov
Copy link
Contributor Author

After looking at the code more and some attempts to fix failing tests I came with slightly different approach.

@kosiew could you please take a look if the fix seems reasonable?

kosiew
kosiew reviewed Sep 8, 2025
View reviewed changes
Copy link
Contributor

@kosiew kosiew left a comment

Choose a reason for hiding this comment

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

hi @etolbakov

Thanks for working on this.
Looks generally good to me.
I noticed and suggested a few small areas for improvement.

@etolbakov
Copy link
Contributor Author

Hi @kosiew!
Thanks a lot for the review! I've addressed all points!
Please let me know what do you think?

kosiew
kosiew approved these changes Sep 9, 2025
View reviewed changes
Copy link
Contributor

@kosiew kosiew left a comment

Choose a reason for hiding this comment

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

LGTM!

@kosiew kosiew changed the title fix: modify the type coercion logic to avoid planning error Fix NULL Arithmetic Handling for Numerical Operators in Type Coercion Sep 9, 2025
@etolbakov
Copy link
Contributor Author

@2010YOUY01 could you please glance over this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kosiew kosiew kosiew approved these changes

Assignees
No one assigned
Labels
logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@etolbakov @2010YOUY01 @kosiew