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

[Iceberg]Support year/month/day/hour transforms both on legacy and non-legacy TimestampType column #21959

Merged

Conversation

hantangwangd
Copy link
Member

@hantangwangd hantangwangd commented Feb 19, 2024

Description

When we create partitions on TimestampType columns(using year/month/day/hour transform) in Iceberg, we will meet various problems because of the legacy timestamp logic in presto engine do not match the logic used in Iceberg transform calculation. See issue: #7122 .

This PR fix the problems, and correct the behaviors for both legacy and non-legacy timestamp.

Test Plan

  • Test year/month/day/hour transform on TimestampType column for various time zones in legacy timestamp mode
  • Test year/month/day/hour transform on TimestampType column in non-legacy timestamp mode

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • 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

== RELEASE NOTES ==

Iceberg Changes
* Support year/month/day/hour transforms both on legacy and non-legacy TimestampType column
* Fix the bug that `CAST` from non-legacy timestamp to date rounding to future when the timestamp is prior than `1970-01-01 00:00:00.000`

Copy link
Contributor

@ZacBlanco ZacBlanco left a comment

Choose a reason for hiding this comment

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

I think the main logic looks ok. Just a few nits and one question

@tdcmeehan
Copy link
Contributor

Can you add to the release note an explanation of the bug fix on Timestamp to Date conversion?

@hantangwangd
Copy link
Member Author

Can you add to the release note an explanation of the bug fix on Timestamp to Date conversion?

Sure, it has been added! Please take a look, thanks!

@gupteaj
Copy link
Contributor

gupteaj commented Feb 27, 2024

Changes looks good to me. In general, I see many references to the legacy system property, can we create a common function for these cases ?

ZacBlanco
ZacBlanco previously approved these changes Feb 27, 2024
@hantangwangd
Copy link
Member Author

hantangwangd commented Feb 28, 2024

Changes looks good to me. In general, I see many references to the legacy system property, can we create a common function for these cases ?

@gupteaj sure, good suggestion! Fixed!

@tdcmeehan tdcmeehan merged commit 51959cd into prestodb:master Feb 28, 2024
56 checks passed
@hantangwangd hantangwangd deleted the fix_timestamp_type_in_partition branch April 18, 2024 08:39
@wanglinsong wanglinsong mentioned this pull request May 1, 2024
48 tasks
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