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

[Presto-Main] Add long overflow checks for IntervalDayTime #24353

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

LiBrian415
Copy link
Contributor

@LiBrian415 LiBrian415 commented Jan 13, 2025

Description

Part of #24087.

Motivation and Context

Throwing an error instead of returning overflowed long values

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
  • Local Testing:
Before:
presto> select interval '64' day * 2147483647;
           _col0
---------------------------
 -76065028926 14:25:51.616
(1 row)

After:
presto> select interval '64' day * 2147483647;
Query 20250113_175455_00000_rusmt failed: interval_day_to_second multiply overflow: 5529600000 ms * 2147483647

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Improve error handling of ``INTERVAL DAY``, ``INTERVAL HOUR``, and ``INTERVAL SECOND`` operators when experiencing overflows. :pr:`24353`

@LiBrian415 LiBrian415 requested a review from a team as a code owner January 13, 2025 17:50
@LiBrian415 LiBrian415 requested a review from presto-oss January 13, 2025 17:50
@tdcmeehan tdcmeehan self-assigned this Jan 13, 2025
@steveburnett
Copy link
Contributor

Thanks for the release note! Nit formatting and rephrasing suggestions to follow Release Note Guidelines.

== RELEASE NOTES ==

General Changes
* Improve error handling of ``INTERVAL DAY``, ``INTERVAL HOUR``, and ``INTERVAL SECOND`` operators when experiencing overflows. :pr:`24353`

Copy link
Contributor

@tdcmeehan tdcmeehan left a comment

Choose a reason for hiding this comment

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

Can you add some tests for all changed operators? I see only multiplication and division.

@LiBrian415
Copy link
Contributor Author

Can you add some tests for all changed operators? I see only multiplication and division.

I don't think I can. My understanding is that the overflow happens when the operation on the interval value converted to ms exceeds the min/max value for longs. But since the value of the interval looks to be constrained to ints, the min / max value we can assign to this type is INT_MIN * (days to ms) / INT_MAX * (days to ms) which isn't large enough to cause an overflow when performing add / sub / negate on only types of IntervalDayTime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants