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 Time.unix_ns and #to_unix_ns #13359

Merged
merged 3 commits into from
Jun 18, 2023

Conversation

garymardell
Copy link
Contributor

I am currently working on an application that requires nanosecond time precision (telemetry data). I receive the number of nanoseconds since the unix epoch. There are already convenience methods when the value is milliseconds (unix_ms and to_unix_ms) however as far as I can tell there is nothing for nanoseconds despite Time supporting the precision.

In my application I have patched in these methods to Time. These methods might be too niche for being included in the standard library though.

@Blacksmoke16
Copy link
Member

Resolves #11061

spec/std/time/time_spec.cr Outdated Show resolved Hide resolved
@straight-shoota straight-shoota linked an issue Apr 20, 2023 that may be closed by this pull request
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

My main concern is that a 64-bit value for nanoseconds can only represent about 300 years +/- the epoch. So it falls short of representing the full range of Time (years 1..9999).
Probably in practice this isn't very relevant. If you're using the unix nanosecond API it's most likely for contemporary timestamps, not far in the future or history.
It would still be nice and easy to remove the limitation on the constructor and allow bigger number types as well. (drop the .to_i).
We could consider changing the return type of #to_unix_ns to Int128 as well, but I'm not sure that makes sense.

@straight-shoota straight-shoota changed the title Add .to_unix_ns and #unix_ns methods to Time Add Time.unix_ns and #to_unix_ns Apr 20, 2023
src/time.cr Outdated
# time.to_unix_ns # => 1452567845678910123
# ```
def to_unix_ns : Int64
(to_unix * NANOSECONDS_PER_SECOND) + nanosecond
Copy link
Contributor

Choose a reason for hiding this comment

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

This causes an OverflowError for Time.unix_ns(Int64::MIN).to_unix_ns, because nanosecond > 0 implies to_unix * NANOSECONDS_PER_SECOND < Int64::MIN.

This isn't a problem for to_unix_ms because Int64's range far exceeds the representable range of milliseconds (the earliest being Time.unix_ms(-62135596800000)).

@straight-shoota
Copy link
Member

straight-shoota commented May 27, 2023

@garymardell Would you mind adressing the review comments?
If you don't want to continue with this PR, please let us know and someone else will take over.

@garymardell
Copy link
Contributor Author

garymardell commented May 29, 2023

Sorry for the delay, fell off my radar. I have pushed 2 changes:

  1. Removed the conversion of nanoseconds to Int64 in .unix_ns as it was unnecessary. This means that it now supports any Int type including Int128 on input allowing the full time range (1...9999 years). Tests were added for the min/max values as Int128.
  2. #to_unix_ns now returns an Int128 value, again to support the full range of years.

@garymardell garymardell requested a review from HertzDevil May 30, 2023 18:32
Copy link
Member

@straight-shoota straight-shoota 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 this is the first instance of a Int128 return type in a non-numeric API. But it's probably fine?
That would be my only concern for other reviewers to consider.

@straight-shoota straight-shoota added this to the 1.9.0 milestone Jun 15, 2023
@straight-shoota straight-shoota merged commit 6de28c8 into crystal-lang:master Jun 18, 2023
Blacksmoke16 pushed a commit to Blacksmoke16/crystal that referenced this pull request Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nanosecond support for Time.unix constructor
5 participants