Skip to content

Rename DateTimeDescriptor epochMillis and epochNanos #966

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

Closed
3 tasks
cdavernas opened this issue Aug 8, 2024 · 8 comments · Fixed by #974
Closed
3 tasks

Rename DateTimeDescriptor epochMillis and epochNanos #966

cdavernas opened this issue Aug 8, 2024 · 8 comments · Fixed by #974
Assignees
Labels
area: spec Changes in the Specification change: breaking A breaking change that will impact in a major version change. change: documentation Improvements or additions to documentation. It won't impact a version change. change: fix Something isn't working. Impacts in a minor version change.
Milestone

Comments

@cdavernas
Copy link
Member

cdavernas commented Aug 8, 2024

What would you like to be added:

  • Rename DateTimeDescriptor epochMillis and epochNanos.
  • Move iso8601, epochMillis and epochNanos properties to a DateTimeDescriptor object, so we avoid duplication (in both WorkflowDescriptor and TaskDescriptor).
  • Move epochMillis and epochNanos to a new epoch DateTimeDescriptor property, for a cleaner aspect.

Why is this needed:

  • The properties use abbreviations, which should always be avoided, If we decide to abbreviate, we should use the standard format, and not an opiniated one (ms and ns).
  • Cleans up definitions
  • Increases maintainability
  • Increases readability

The proposal:

DateTime Descriptor

Name Type Description Example
iso8601 string The start time of the execution as a ISO 8601 date time string. It uses T as the date-time delimiter, either UTC (Z) or a time zone offset (+01:00). The precision can be either seconds, milliseconds or nanoseconds 2022-01-01T12:00:00Z, 2022-01-01T12:00:00.123456Z, 2022-01-01T12:00:00.123+01:00
epoch.ms integer The start time of the execution as a integer value of milliseconds since midnight of 1970-01-01 UTC 1641024000123 (="2022-01-01T08:00:00.123Z")
epoch.ns integer The start time of the execution as a integer value of nanoseconds since midnight of 1970-01-01 UTC 1641024000123456 (="2022-01-01T08:00:00.123456Z")
@cdavernas cdavernas added change: fix Something isn't working. Impacts in a minor version change. change: documentation Improvements or additions to documentation. It won't impact a version change. area: spec Changes in the Specification change: breaking A breaking change that will impact in a major version change. labels Aug 8, 2024
@cdavernas cdavernas added this to the v1.0.0-alpha3 milestone Aug 8, 2024
@cdavernas
Copy link
Member Author

@matthias-pichler-warrify WDYT? Do you want to take care of it?

@matthias-pichler
Copy link
Collaborator

Looks good to me 👍

@matthias-pichler
Copy link
Collaborator

matthias-pichler commented Aug 8, 2024

Regarding nanoseconds: the jq docs say that all numbers are represented as IEEE 754 double precision floating point numbers: https://jqlang.github.io/jq/manual/v1.7/

Numbers in jq are internally represented by their IEEE754 double precision approximation. Any arithmetic operation with numbers, whether they are literals or results of previous filters, will produce a double precision floating point result.

and as it turns out these are too small to safely represent nanoseconds as an integer. (At least according to Number.MAX_SAFE_INTEGER in JS)

So I am afraid we have to drop nanoseconds to ensure we have safe jq expressions

@cdavernas
Copy link
Member Author

cdavernas commented Aug 8, 2024

That's fine to me, I didn't really like that we supported both in the first place anyways.

So what, we should therefore have iso8601 and epoch (expressed in ms)?

@matthias-pichler
Copy link
Collaborator

I think having epoch.ms is still nice because it's more explicit. Before I knew JS the epoch always was seconds to me 😅 and now I always have to check when using a different language/api ... we can even consider adding epoch.sec maybe?

@cdavernas
Copy link
Member Author

Yeah, sure, why not?

However, we need to properly document it so that users don't start to think that seconds and milliseconds are complementary components, but are instead mutually exclusive, don't you agree?

@matthias-pichler
Copy link
Collaborator

However, we need to properly document it so that users don't start to think that seconds and milliseconds are complementary components, but are instead mutually exclusive, don't you agree?

not 100% sure what you mean ... you mean that ms should also contain the whole timestamp right?

I would like something like:

echo:
  sec: 1723190735
  ms: 1723190735645

@cdavernas
Copy link
Member Author

not 100% sure what you mean ... you mean that ms should also contain the whole timestamp right?

No, I mean that some users might wrongly think that sec are the seconds of the epoch, and ms the milliseconds, even if that's obivous to me it isn't. Something like 10 secs AND 10000 milliseconds, instead of just 10 secs OR 10000 milliseconds.

I hope this is clearer 👅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: spec Changes in the Specification change: breaking A breaking change that will impact in a major version change. change: documentation Improvements or additions to documentation. It won't impact a version change. change: fix Something isn't working. Impacts in a minor version change.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants