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 Presto's date_add UDF with TimestampAndTimeZone and DST #11353

Closed

Conversation

kevinwilfong
Copy link
Contributor

Summary:
Presto Java's date_add UDF treats the day the clocks move forward as a 23 hour day,
and the day the clocks move back as a 25 hour day.

This means for units greater than or equal to a day date_add with
TimestampWithTimeZone cannot simply use the addition on GMT. It needs to check if
the time zone has changed between the original timestamp and the timestamp after
the addition and apply the change in time zone offset to the mills since epoch to
account for this long or short day.

Note that the for units less than a day, this does not apply. E.g. if you add 24 hours
to a TimestampWithTimeZone and it cross a DST boundary, we do not need to apply
the change in offset like you would if you had added 1 day.

Differential Revision: D64982873

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 25, 2024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D64982873

Copy link

netlify bot commented Oct 25, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 3ddb9e0
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/6722677506837e00083b477c

kevinwilfong pushed a commit to kevinwilfong/velox that referenced this pull request Oct 29, 2024
…incubator#11353)

Summary:

Presto Java's date_add UDF treats the day the clocks move forward as a 23 hour day,
and the day the clocks move back as a 25 hour day.

This means for units greater than or equal to a day date_add with
TimestampWithTimeZone cannot simply use the addition on GMT.  It needs to check if
the time zone has changed between the original timestamp and the timestamp after
the addition and apply the change in time zone offset to the mills since epoch to
account for this long or short day.

Note that the for units less than a day, this does not apply.  E.g. if you add 24 hours
to a TimestampWithTimeZone and it cross a DST boundary, we do not need to apply
the change in offset like you would if you had added 1 day.

Differential Revision: D64982873
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D64982873

kevinwilfong pushed a commit to kevinwilfong/velox that referenced this pull request Oct 29, 2024
…incubator#11353)

Summary:

Presto Java's date_add UDF treats the day the clocks move forward as a 23 hour day,
and the day the clocks move back as a 25 hour day.

This means for units greater than or equal to a day date_add with
TimestampWithTimeZone cannot simply use the addition on GMT.  It needs to
compute the addition based on the local time to handle these.

Note that the for units less than a day, this does not apply, and will produce incorrect results if we compute it using local time.

Differential Revision: D64982873
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D64982873

kevinwilfong pushed a commit to kevinwilfong/velox that referenced this pull request Oct 29, 2024
…incubator#11353)

Summary:

Presto Java's date_add UDF treats the day the clocks move forward as a 23 hour day,
and the day the clocks move back as a 25 hour day.

This means for units greater than or equal to a day date_add with
TimestampWithTimeZone cannot simply use the addition on GMT.  It needs to
compute the addition based on the local time to handle these.

Note that the for units less than a day, this does not apply, and will produce incorrect results if we compute it using local time.

Differential Revision: D64982873
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D64982873

kevinwilfong pushed a commit to kevinwilfong/velox that referenced this pull request Oct 29, 2024
Summary:
Presto Java's date_diff UDF respects DST for units greater than or equal to a day, but
for units less than a day computes the diff on the system time (GMT).

This means for units less than a day date_diff with TimestampWithTimeZone cannot
compute the difference of the local times.  It needs to use system time to be
consistent.

Note that the for units greater than or equal to a day, this does not apply, and we
should continue to use the local time to compute the difference.

This is analogous to the change made to date_add in
facebookincubator#11353

Differential Revision: D65165953
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D64982873

kevinwilfong pushed a commit to kevinwilfong/velox that referenced this pull request Oct 29, 2024
…incubator#11353)

Summary:

Presto Java's date_add UDF treats the day the clocks move forward as a 23 hour day,
and the day the clocks move back as a 25 hour day.

This means for units greater than or equal to a day date_add with
TimestampWithTimeZone cannot simply use the addition on GMT.  It needs to
compute the addition based on the local time to handle these.

Note that the for units less than a day, this does not apply, and will produce incorrect results if we compute it using local time.

Differential Revision: D64982873
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D64982873

kevinwilfong pushed a commit to kevinwilfong/velox that referenced this pull request Oct 29, 2024
…incubator#11353)

Summary:

Presto Java's date_add UDF treats the day the clocks move forward as a 23 hour day,
and the day the clocks move back as a 25 hour day.

This means for units greater than or equal to a day date_add with
TimestampWithTimeZone cannot simply use the addition on GMT.  It needs to
compute the addition based on the local time to handle these.

Note that the for units less than a day, this does not apply, and will produce incorrect results if we compute it using local time.

Differential Revision: D64982873
kevinwilfong pushed a commit to kevinwilfong/velox that referenced this pull request Oct 29, 2024
…kincubator#11380)

Summary:

Presto Java's date_diff UDF respects DST for units greater than or equal to a day, but
for units less than a day computes the diff on the system time (GMT).

This means for units less than a day date_diff with TimestampWithTimeZone cannot
compute the difference of the local times.  It needs to use system time to be
consistent.

Note that the for units greater than or equal to a day, this does not apply, and we
should continue to use the local time to compute the difference.

This is analogous to the change made to date_add in
facebookincubator#11353

Differential Revision: D65165953
kevinwilfong pushed a commit to kevinwilfong/velox that referenced this pull request Oct 30, 2024
…incubator#11353)

Summary:

Presto Java's date_add UDF treats the day the clocks move forward as a 23 hour day,
and the day the clocks move back as a 25 hour day.

This means for units greater than or equal to a day date_add with
TimestampWithTimeZone cannot simply use the addition on GMT.  It needs to
compute the addition based on the local time to handle these.

Note that the for units less than a day, this does not apply, and will produce incorrect results if we compute it using local time.

Reviewed By: xiaoxmeng

Differential Revision: D64982873
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D64982873

…incubator#11353)

Summary:

Presto Java's date_add UDF treats the day the clocks move forward as a 23 hour day,
and the day the clocks move back as a 25 hour day.

This means for units greater than or equal to a day date_add with
TimestampWithTimeZone cannot simply use the addition on GMT.  It needs to
compute the addition based on the local time to handle these.

Note that the for units less than a day, this does not apply, and will produce incorrect results if we compute it using local time.

Reviewed By: xiaoxmeng

Differential Revision: D64982873
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D64982873

kevinwilfong pushed a commit to kevinwilfong/velox that referenced this pull request Oct 30, 2024
…incubator#11353)

Summary:

Presto Java's date_add UDF treats the day the clocks move forward as a 23 hour day,
and the day the clocks move back as a 25 hour day.

This means for units greater than or equal to a day date_add with
TimestampWithTimeZone cannot simply use the addition on GMT.  It needs to
compute the addition based on the local time to handle these.

Note that the for units less than a day, this does not apply, and will produce incorrect results if we compute it using local time.

Reviewed By: xiaoxmeng

Differential Revision: D64982873
kevinwilfong pushed a commit to kevinwilfong/velox that referenced this pull request Oct 30, 2024
…kincubator#11380)

Summary:

Presto Java's date_diff UDF respects DST for units greater than or equal to a day, but
for units less than a day computes the diff on the system time (GMT).

This means for units less than a day date_diff with TimestampWithTimeZone cannot
compute the difference of the local times.  It needs to use system time to be
consistent.

Note that the for units greater than or equal to a day, this does not apply, and we
should continue to use the local time to compute the difference.

This is analogous to the change made to date_add in
facebookincubator#11353

Reviewed By: xiaoxmeng

Differential Revision: D65165953
facebook-github-bot pushed a commit that referenced this pull request Oct 30, 2024
Summary:
Pull Request resolved: #11380

Presto Java's date_diff UDF respects DST for units greater than or equal to a day, but
for units less than a day computes the diff on the system time (GMT).

This means for units less than a day date_diff with TimestampWithTimeZone cannot
compute the difference of the local times.  It needs to use system time to be
consistent.

Note that the for units greater than or equal to a day, this does not apply, and we
should continue to use the local time to compute the difference.

This is analogous to the change made to date_add in
#11353

Reviewed By: xiaoxmeng

Differential Revision: D65165953

fbshipit-source-id: 7817ffd31c9e078c3078e861fe3813135bf0f2b8
@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 1c0c533.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants