Skip to content

Comments

[Iceberg]Support timestamp without timezone in time travel expressions#23714

Merged
tdcmeehan merged 1 commit intoprestodb:masterfrom
hantangwangd:support_timestamp_in_time_travel
Sep 26, 2024
Merged

[Iceberg]Support timestamp without timezone in time travel expressions#23714
tdcmeehan merged 1 commit intoprestodb:masterfrom
hantangwangd:support_timestamp_in_time_travel

Conversation

@hantangwangd
Copy link
Member

@hantangwangd hantangwangd commented Sep 24, 2024

Description

We have supported time travel query on Iceberg table in PR #21425, however currently we only support timestamp with timezone expressions in FOR TIMESTAMP AS OF as follows:

select * from tab1 FOR TIMESTAMP AS OF TIMESTAMP '2023-08-17 13:29:46.822 America/Los_Angeles';

In many scenarios, we do not want to explicitly indicate the time zone, but just default to using the time zone specified in the current session, since it's natural for users to execute queries based on the timestamp string in their own time zone.

This PR support timestamp without timezone in time travel expressions, so users can execute time travel query as follows:

select * from tab1 FOR TIMESTAMP AS OF TIMESTAMP '2023-08-17 13:29:46.822';

Timestamp without timezone will be parsed and rendered in the session time zone, it's consistent with data with type of TimestampType in Presto.

Motivation and Context

Support timestamp without timezone in time travel expressions

Impact

N/A

Test Plan

  • Newly added test cases to show the behavior of timestamp without tz expressions under sessions with different time zone

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 Connector Changes
* Support timestamp without timezone in time travel expressions :pr:`23714`

@hantangwangd hantangwangd marked this pull request as ready for review September 24, 2024 13:46
@tdcmeehan tdcmeehan self-assigned this Sep 24, 2024
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

Just a nit suggesting a change in the link.

@hantangwangd hantangwangd force-pushed the support_timestamp_in_time_travel branch from f4bfebd to 7d4129a Compare September 24, 2024 15:01
steveburnett
steveburnett previously approved these changes Sep 24, 2024
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)

Pull updated branch, new local doc build, looks great!

ZacBlanco
ZacBlanco previously approved these changes Sep 24, 2024
Timestamp without timezone will be parsed and rendered in the session time zone. See `TIMESTAMP <https://prestodb.io/docs/current/language/types.html#timestamp>`_.

The option following FOR TIMESTAMP AS OF can accept any expression that returns a timestamp or timestamp with time zone value.
For example, `TIMESTAMP '2023-10-17 13:29:46.822 America/Los_Angeles'` and `TIMESTAMP '2023-10-17 13:29:46.822'` are all constant strings for the expression.
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence is a little confusing to me. I think it's fair to specify that they are both valid -- but I think it would be good to explain what distinguishes them. Here's a suggestion, but feel free to modify:

Suggested change
For example, `TIMESTAMP '2023-10-17 13:29:46.822 America/Los_Angeles'` and `TIMESTAMP '2023-10-17 13:29:46.822'` are all constant strings for the expression.
For example, `TIMESTAMP '2023-10-17 13:29:46.822 America/Los_Angeles'` and `TIMESTAMP '2023-10-17 13:29:46.822'` are both valid timestamps. The first specifies the timestamp within the timezone `America/Los_Angeles`. The second will use the timestamp based on the user's session timezone..

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your suggestion, modified! This change makes it more clear for readers to understand, please take a look when available.

assertTrue(snapshotTimeMillis - currentTimeMillis <= 10,
format("Snapshot time %s is greater than the current time %s by more than 10ms", snapshotTimeMillis, currentTimeMillis));

while (currentTimeMillis <= snapshotTimeMillis) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a huge fan of the busy loop but I know we do this elsewhere, so I won't block on this but it wastes a lot of resources especially for concurrent tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see your point, but sometimes we do need to make sure that several consecutive actions happen in different milliseconds. I'd like to to make a detailed investigation into the time frame library later to see if there is a more suitable non busy loop waiting solution, and open for any better thoughts.

@hantangwangd hantangwangd dismissed stale reviews from ZacBlanco and steveburnett via bfb6dad September 25, 2024 01:51
@hantangwangd hantangwangd force-pushed the support_timestamp_in_time_travel branch from 7d4129a to bfb6dad Compare September 25, 2024 01:51
@hantangwangd hantangwangd force-pushed the support_timestamp_in_time_travel branch from bfb6dad to 2a62853 Compare September 26, 2024 00:33
@tdcmeehan tdcmeehan merged commit 6c57a3e into prestodb:master Sep 26, 2024
@jaystarshot jaystarshot mentioned this pull request Nov 1, 2024
25 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