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

Add support casting timestamp of micro/nanosecond #17689

Merged
merged 1 commit into from
May 2, 2022

Conversation

fengpengyuan
Copy link
Contributor

@fengpengyuan fengpengyuan commented Apr 25, 2022

Some tables would have date time formats (columns of VARCHAR type) with microsecond or nanosecond precision. When trying to cast these to TIMESTAMP type, Presto will throw a cast error. We should support casting these higher precision date time formats to timestamp for better usability of Presto. Since timestamp only supports millisecond precision, the behavior of the casting should be to truncate to millisecond precision.

Test plan - Unit tests

General Changes
* Add support for casting microseconds/nanoseconds to Timestamp type, though with precision loss to Milliseconds.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 25, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: fengpengyuan / name: Fengpeng (30d92fa84f35e017e7d9cfb7a8fdb3baecb8dff2)

@prithvip prithvip self-requested a review April 25, 2022 20:08
Copy link
Contributor

@prithvip prithvip left a comment

Choose a reason for hiding this comment

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

  1. Please start the commit message with "Add support for..." and limit the commit title to 50 characters
  2. Squash both commits into one commit
  3. Since this a user-visible feature, add a release note describing the change

@fengpengyuan fengpengyuan requested a review from a team as a code owner April 28, 2022 13:34
@fengpengyuan fengpengyuan changed the title Support casting of microsecond and nanosecond precision date time for… Add support casting timestamp of micro/nanosecond Apr 28, 2022
@highker highker requested a review from arunthirupathi April 29, 2022 04:14
@arunthirupathi
Copy link

The change looks good.

Would this result in a perf regression for existing use cases as it tries new formats ? I can think of error cases, will have more results to try before it fails, but want to confirm your thoughts.

Change the release note to
support casting microseconds/nanoseconds to Timestamp type, though with precision loss to Milliseconds.

@fengpengyuan
Copy link
Contributor Author

The change looks good.

Would this result in a perf regression for existing use cases as it tries new formats ? I can think of error cases, will have more results to try before it fails, but want to confirm your thoughts.

Change the release note to support casting microseconds/nanoseconds to Timestamp type, though with precision loss to Milliseconds.

Thanks for review.
Valid point, but I do not think it will cause much(any) regression. When parse the input timestamp string, the DateTimeFormatter will check each of these parsers (patterns) sequentially, per the doc. If users do not input micro/nanoseconds format, I think there will be no impact(regression). Only when users try the new pattern(micro/nano), it will iterate more parses in the list, but only subtle overhead imo (as very limited no. of new patterns added). If you have concerns, we can move all the new parsers to the end of the list, but that will break the code patterns a little we try to maintain. Thoughts?

@arunthirupathi
Copy link

Today in Presto, all the timestamps are stripped to Milliseconds at the converter layer, so the precision loss is ok.

but after this change, some operators will have weird behavior.

"cast('2001-1-22 03:04:05.321123' as timestamp)" = "cast('2001-1-22 03:04:05.321999' as timestamp)"

Since this is already happening other places at the connector, this precision loss is ok.

@arunthirupathi arunthirupathi requested a review from prithvip April 29, 2022 16:13
@arunthirupathi
Copy link

Let us wait for @prithvip to approve as well, then we can merge this. Give it a couple of days.

@prithvip
Copy link
Contributor

prithvip commented May 2, 2022

@fengpengyuan Thank you very much for contributing this change. The code looks good to me, but could you please edit your initial comment to follow our release notes guidelines? https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines

@prithvip
Copy link
Contributor

prithvip commented May 2, 2022

@fengpengyuan Thank you for updating the release notes. @arunthirupathi Let's go ahead and merge this.

@arunthirupathi arunthirupathi merged commit 237e18d into prestodb:master May 2, 2022
@mshang816 mshang816 mentioned this pull request May 17, 2022
14 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.

3 participants