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

Fix silent overflow in DateTimeEncoding.pack #24674

Merged
merged 1 commit into from
Mar 6, 2025

Conversation

kevinwilfong
Copy link
Contributor

@kevinwilfong kevinwilfong commented Mar 5, 2025

Description

Throw an exception when DateTimeEncoding's pack method will see the millisUtc overflow/underflow when constructing a TimestampWithTimeZone long.

Motivation and Context

Today operations that produce a TimestampWithTimeZone can sliently overflow/underflow when packing the millisUtc and time zone ID into a long. The DateTimeEncoding.pack method accepts the millisUtc as a 64-bit long, and then uses 52 bits of the TimestampWithTimeZone long to hold that value. If the value requires more than 52 bits, it will silently overflow/underflow producing incorrect results.

This change adds a check to see if the millisUtc can fit in 52 bits, and throws an ArithmeticException if it detects the value will overflow/underflow.

As an example

select from_iso8601_timestamp('73326-09-11T20:14:46.000Z');

             _col0
-------------------------------
 -69387-04-22 03:45:15.504 UTC

Impact

Users will see exceptions now where they were previously seeing incorrect results.

Test Plan

Added unit tests for a few UDFs I identified as producing TimestampWithTimeZone values to ensure their behavior with values that just fit in 52 bits, and values that require more than 52 bits.

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
* Fix silently returning incorrect results when trying to construct a TimestampWithTimeZone from a value that has a unix timestamp that is too large/small.

@kevinwilfong kevinwilfong requested review from elharo and a team as code owners March 5, 2025 00:16
@kevinwilfong kevinwilfong requested a review from jaystarshot March 5, 2025 00:16
@kevinwilfong kevinwilfong force-pushed the tswtz_overflow branch 3 times, most recently from 392869c to 8695e25 Compare March 5, 2025 21:59
Copy link
Contributor

@amitkdutta amitkdutta left a comment

Choose a reason for hiding this comment

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

Thanks @kevinwilfong

@amitkdutta amitkdutta merged commit 9d46add into prestodb:master Mar 6, 2025
53 checks passed
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.

3 participants