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 from_unixtime_nanos operation #5046
Add from_unixtime_nanos operation #5046
Conversation
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@prestosql.io. For more information, see https://github.com/prestosql/cla. |
I signed + emailed the CLA yesterday but I guess it hasn't been processed yet |
@cla-bot check |
The cla-bot has been summoned, and re-checked this pull request! |
6ff6b53
to
da0692f
Compare
presto-main/src/test/java/io/prestosql/operator/scalar/TestDateTimeFunctions.java
Outdated
Show resolved
Hide resolved
presto-main/src/test/java/io/prestosql/operator/scalar/TestDateTimeFunctions.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/DateTimeFunctions.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/DateTimeFunctions.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/DateTimeFunctions.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/DateTimeFunctions.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/DateTimeFunctions.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Would you be fine also adding documentation to datetime.rst
?
Also please update PR message to reference #5038
presto-main/src/main/java/io/prestosql/operator/scalar/DateTimeFunctions.java
Outdated
Show resolved
Hide resolved
presto-main/src/test/java/io/prestosql/operator/scalar/TestDateTimeFunctions.java
Outdated
Show resolved
Hide resolved
presto-main/src/test/java/io/prestosql/operator/scalar/TestDateTimeFunctions.java
Outdated
Show resolved
Hide resolved
presto-main/src/test/java/io/prestosql/operator/scalar/TestDateTimeFunctions.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/testing/DateTimeTestingUtils.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/DateTimeFunctions.java
Outdated
Show resolved
Hide resolved
long epochSeconds = unixTimeNanos / Timestamps.NANOSECONDS_PER_SECOND; | ||
long nanosOfSecond = unixTimeNanos % Timestamps.NANOSECONDS_PER_SECOND; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to use floorDiv
and floorMod
to produce correct results when the input is negative. In particular, nanosOfSecond should never be negative
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah interesting, do we need to consider negative unix timestamps as valid? I would prefer to throw when they are encountered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why not.
From wikipedia (https://en.wikipedia.org/wiki/Unix_time)
The Unix time number is zero at the Unix epoch, and increases by exactly 86400 per day since the epoch. Thus 2004-09-16T00:00:00Z, 12677 days after the epoch, is represented by the Unix time number 12677 × 86400 = 1095292800. This can be extended backwards from the epoch too, using negative numbers;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 ok will do
presto-main/src/main/java/io/prestosql/operator/scalar/DateTimeFunctions.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/DateTimeFunctions.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/DateTimeFunctions.java
Outdated
Show resolved
Hide resolved
presto-main/src/test/java/io/prestosql/operator/scalar/TestDateTimeFunctions.java
Outdated
Show resolved
Hide resolved
presto-main/src/test/java/io/prestosql/operator/scalar/TestDateTimeFunctions.java
Outdated
Show resolved
Hide resolved
presto-main/src/test/java/io/prestosql/operator/scalar/TestDateTimeFunctions.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/DateTimeFunctions.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/DateTimeFunctions.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/DateTimeFunctions.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Can you rebase and squash the commits before I merge it?
4ccf90d
to
7e0ff8d
Compare
Thanks @martint , should be good to go now! |
7e0ff8d
to
c5c6b7d
Compare
This addresses the nanos use case of #5046 and also adds support for converting nanos that don't fit in a BIGINT.
Related to #5038