Skip to content

Support basic timestamp in the iceberg connector#17190

Merged
beinan merged 1 commit intoprestodb:masterfrom
ChunxuTang:iceberg-timestamp
Feb 9, 2022
Merged

Support basic timestamp in the iceberg connector#17190
beinan merged 1 commit intoprestodb:masterfrom
ChunxuTang:iceberg-timestamp

Conversation

@ChunxuTang
Copy link
Copy Markdown
Member

@ChunxuTang ChunxuTang commented Jan 14, 2022 •
edited
Loading

Cherry-pick iceberg timestamp support from trinodb/trino@e82c2d5

Co-Authored-By: Parth Brahmbhatt pbrahmbhatt@netflix.com

Test plan - Unit test

== RELEASE NOTES ==

Iceberg Changes
* Support basic timestamp in the iceberg connector.

@ChunxuTang ChunxuTang mentioned this pull request Jan 14, 2022
Closed
@ChunxuTang ChunxuTang force-pushed the iceberg-timestamp branch 2 times, most recently from c04839e to 2906e2d Compare January 19, 2022 19:48
@ChunxuTang
Copy link
Copy Markdown
Member Author

ChunxuTang commented Jan 19, 2022

Note that this PR won't completely resolve the timestamp querying issue from the presto-iceberg connector (e.g. the millisecond unit issue mentioned by @beinan). This PR helps create initial features to support timestamps.

@ChunxuTang
Copy link
Copy Markdown
Member Author

ChunxuTang commented Jan 19, 2022

Hi @beinan @zhenxiao, could you help review the PR? Thanks!

@ChunxuTang ChunxuTang changed the title [WIP] Support timestamp in the iceberg connector Support timestamp in the iceberg connector Jan 19, 2022
beinan
beinan reviewed Jan 20, 2022
View reviewed changes
presto-iceberg/src/main/java/com/facebook/presto/iceberg/ExpressionConverter.java Outdated
Copy link
Copy Markdown
Member

@beinan beinan Jan 19, 2022

Choose a reason for hiding this comment

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

non-blocking: I'm not sure if we should user's local timezone or just utc here. what do you think?

Copy link
Copy Markdown
Member Author

@ChunxuTang ChunxuTang Jan 20, 2022

Choose a reason for hiding this comment

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

Per our discussion, I removed the timestamp with time zone feature as it requires extra work in the parquet/ORC reader.

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergPageSource.java Outdated
Comment on lines 218 to 223
Copy link
Copy Markdown
Member

@beinan beinan Jan 20, 2022

Choose a reason for hiding this comment

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

merge the three if for DATE/TIME/TIMESTAMP?

Copy link
Copy Markdown
Member Author

@ChunxuTang ChunxuTang Jan 20, 2022

Choose a reason for hiding this comment

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

Thanks for the comment! I merged them into one if statement.

presto-iceberg/src/test/java/com/facebook/presto/iceberg/TestIcebergSmoke.java Outdated
Copy link
Copy Markdown
Member

@beinan beinan Jan 20, 2022

Choose a reason for hiding this comment

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

any test to cover the time with zone?

Copy link
Copy Markdown
Member Author

@ChunxuTang ChunxuTang Jan 20, 2022

Choose a reason for hiding this comment

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

The timestamp with time zone will require changes in the ORC/parquet readers underneath. So I remove the timestamp with time zone support temporarily for now.

@ChunxuTang ChunxuTang changed the title Support timestamp in the iceberg connector Support basic timestamp in the iceberg connector Jan 20, 2022
@ChunxuTang
Copy link
Copy Markdown
Member Author

ChunxuTang commented Jan 20, 2022 •
edited
Loading

As mentioned by @beinan, the timestamp with time zone feature requires extra changes in the ORC/Parquet readers. As this PR only serves as the first PR to support the basic timestamp feature, I removed the code for the timestamp with time zone.
I'll send follow-up PRs for extra features.

beinan
beinan approved these changes Jan 20, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@beinan beinan left a comment

Choose a reason for hiding this comment

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

lgtm, rerun the test

@Parth-Brahmbhatt
Support basic timestamp in the iceberg connector
5242e77 
Cherry-pick iceberg timestamp support from trinodb/trino@e82c2d5

Co-Authored-By: Parth Brahmbhatt <pbrahmbhatt@netflix.com>
zhenxiao
zhenxiao approved these changes Feb 8, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@zhenxiao zhenxiao left a comment

Choose a reason for hiding this comment

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

looks good, @ChunxuTang
could you please add release note in the PR description:
Support basic timestamp in the iceberg connector

@ChunxuTang
Copy link
Copy Markdown
Member Author

ChunxuTang commented Feb 8, 2022

looks good, @ChunxuTang could you please add release note in the PR description: Support basic timestamp in the iceberg connector

Hi @zhenxiao, thanks for the review!
I've updated the release note.

@beinan beinan merged commit 621d42d into prestodb:master Feb 9, 2022
@ChunxuTang ChunxuTang deleted the iceberg-timestamp branch February 9, 2022 00:42
This was referenced Feb 24, 2022
Closed
Closed
Merged
@agrawalreetika agrawalreetika mentioned this pull request Mar 2, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zhenxiao zhenxiao zhenxiao approved these changes

@beinan beinan beinan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ChunxuTang @zhenxiao @beinan