Skip to content

Add overflow detection to IntervalYearMonthOperators #24617

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
Merged
pramodsatya merged 1 commit into from
Mar 18, 2025
Merged

Add overflow detection to IntervalYearMonthOperators #24617

pramodsatya merged 1 commit into from
Mar 18, 2025

Conversation

@pramodsatya
Copy link
Contributor

@pramodsatya pramodsatya commented Feb 24, 2025 •
edited
Loading

Description

Part of #24087. Duplicate of #24089, continuing it here since Tim is OOO and this change is needed for Velox PR: facebookincubator/velox#11612.

Motivation and Context

Fix incorrect data caused by overflow, and correct the type mapping in certain operators.

Impact

Where previously some expressions could have resulted in an overflow and incorrect results, now they will fail with a NUMERIC_VALUE_OUT_OF_RANGE error.

Test Plan

Unit tests

Release Notes

== RELEASE NOTES ==

General Changes
* Fix addition, subtraction, multiplication and division of ``INTERVAL YEAR MONTH`` values.

@pramodsatya pramodsatya requested a review from a team as a code owner February 24, 2025 19:08
@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Feb 24, 2025
@prestodb-ci prestodb-ci requested review from a team, ScrapCodes and psnv03 and removed request for a team February 24, 2025 19:08
@steveburnett
Copy link
Contributor

steveburnett commented Feb 24, 2025

Thanks for the release note!

New release note guidelines. Please remove the manual PR link in the following format from the release note entries for this PR.


:pr:`12345`

I have updated the Release Notes Guidelines to remove the examples of manually adding the PR link.

@pramodsatya
Copy link
Contributor Author

pramodsatya commented Feb 24, 2025

Thanks @steveburnett, removed the manual PR link from release note. Could you please take another look?

@pramodsatya
Copy link
Contributor Author

pramodsatya commented Feb 24, 2025

@hantangwangd, could you please help review/merge this PR? The commit is cherrypicked from #24089 that you had helped review before. Thanks!

@steveburnett
Copy link
Contributor

steveburnett commented Feb 24, 2025

Thanks @steveburnett, removed the manual PR link from release note. Could you please take another look?

LGTM! Thanks!

hantangwangd
hantangwangd approved these changes Feb 25, 2025
View reviewed changes
Copy link
Member

@hantangwangd hantangwangd left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@pramodsatya
Copy link
Contributor Author

pramodsatya commented Feb 25, 2025

@jaystarshot, @yingsu00, could you please help review this PR?

@majetideepak
Copy link
Collaborator

majetideepak commented Feb 27, 2025

@jaystarshot Can you please help with this review? Thanks!

jaystarshot
jaystarshot reviewed Mar 4, 2025
View reviewed changes
presto-main/src/main/java/com/facebook/presto/type/IntervalYearMonthOperators.java
return (long) (left * right);
long result = (long) (left * right);
if (result < Integer.MIN_VALUE || result > Integer.MAX_VALUE) {
throw new PrestoException(NUMERIC_VALUE_OUT_OF_RANGE, "Overflow multiplying double by interval year-month value: " + left + " * " + right);
Copy link
Member

@jaystarshot jaystarshot Mar 4, 2025

Choose a reason for hiding this comment

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

Why are we comparing the result with int_max here? The return type is long

Copy link
Member

@hantangwangd hantangwangd Mar 11, 2025

Choose a reason for hiding this comment

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

As I understand, like IntegerType, the type IntervalYearMonthType in presto specifies long.class as its javaType, but the actual primitive value is int. So the functions here need to declare long as their parameters and return types, but the actual values should be within the range of integers. This PR handles integer overflow scenarios by directly throwing expected exceptions rather than returning unexpected values after overflow.

Copy link
Contributor Author

@pramodsatya pramodsatya Mar 11, 2025

Choose a reason for hiding this comment

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

+1, thanks @hantangwangd.

@pramodsatya pramodsatya merged commit b4a5f8f into prestodb:master Mar 18, 2025
92 checks passed
@pramodsatya pramodsatya deleted the intervalym_1 branch March 18, 2025 18:59
This was referenced Mar 18, 2025
Closed
Closed
@prestodb-ci prestodb-ci mentioned this pull request Mar 28, 2025
Merged
30 tasks
@unidevel unidevel mentioned this pull request Apr 25, 2025
Closed
30 tasks
This was referenced May 6, 2025
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hantangwangd hantangwangd hantangwangd approved these changes

@jaystarshot jaystarshot jaystarshot approved these changes

@ScrapCodes ScrapCodes Awaiting requested review from ScrapCodes ScrapCodes was automatically assigned from prestodb/ibm-reviewers

@psnv03 psnv03 Awaiting requested review from psnv03 psnv03 was automatically assigned from prestodb/ibm-reviewers

@aditi-pandit aditi-pandit Awaiting requested review from aditi-pandit

@majetideepak majetideepak Awaiting requested review from majetideepak

Assignees

No one assigned

Labels

from:IBM PR from IBM

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@pramodsatya @steveburnett @majetideepak @hantangwangd @jaystarshot @prestodb-ci @tdcmeehan