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

Provide upperBin and lowerBin signatures that accept Durations #5148

Merged
merged 3 commits into from
Feb 27, 2024

Conversation

chipkent
Copy link
Member

Provide upperBin and lowerBin signatures that accept Durations.

Resolves #5147

@@ -3170,6 +3170,26 @@ public static Instant lowerBin(@Nullable final Instant instant, long intervalNan
return epochNanosToInstant(Numeric.lowerBin(epochNanos(instant), intervalNanos));
}

/**
* Returns an {@link Instant} value, which is at the starting (lower) end of a time range defined by the interval
* nanoseconds. For example, a 5*MINUTE intervalNanos value would return the instant value for the start of the
Copy link
Member

Choose a reason for hiding this comment

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

This is also a criticism of the existing javadoc - we're considering epoch nanoseconds as the start of the interval. I do worry a bit generally that there may be some sloppiness around what the user wants and what we calculate, potentially as it relates to leap seconds and such.

For example, are we guaranteed that lowerBin(x, Duration.ofSeconds(1)).equals(x.truncatedTo(ChronoUnit.SECONDS))? Ditto wrt MICROS, MILLIS, MINUTES, HOURS, HALF_DAYS, DAYS.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess there is still stuff about time that I didn't know.

  1. Java time does not use UTC, they have their own "Java Time-Scale".
  2. "Java Time Scale" divides the day into exactly 86400 subdivisions known as seconds. These seconds differ from SI seconds but closely match the "international civil time scale".
  3. From 1972-11-03 to now, Java Time Scale exactly matches UTC-SLS on days that do not have leap seconds.
  4. On days that do have leap seconds, the leap second is spread equally over the last 1000 seconds of the day.

Details can be found at: https://docs.oracle.com/javase/8/docs/api/java/time/Instant.html

Given this weirdness, I think the upperBin/lowerBin return what is expected.

@devinrsmith devinrsmith self-requested a review February 27, 2024 15:30
@chipkent chipkent merged commit c2ab633 into deephaven:main Feb 27, 2024
19 checks passed
@chipkent chipkent deleted the binning_duration branch February 27, 2024 21:54
@github-actions github-actions bot locked and limited conversation to collaborators Feb 27, 2024
@deephaven-internal
Copy link
Contributor

Labels indicate documentation is required. Issues for documentation have been opened:

Community: https://github.com/deephaven/deephaven.io/issues/3740

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

upperBin and lowerBin should have methods that accept Durations
4 participants