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

fix!: Fix a TimeRange off-by-one bug in nanosecond calculation #302

Closed
deephaven-internal opened this issue Aug 28, 2024 · 4 comments
Closed

Comments

@deephaven-internal
Copy link

This issue was auto-generated

PR: deephaven/deephaven-core#5648
Author: chipkent

Original PR Body

Fix a TimeRange off-by-one bug in nanosecond calculation.

Resolves #5646

BREAKING CHANGE: The length of a business day was being incorrectly computed by 1 ns. To keep the business day length the same for existing calendars, the close of a business period has been changed from inclusive to exclusive. This default behavior can be overridden by using <includeClose>true</includeClose> when specifying time ranges. This will include the closing time for the range and will result in a business period that is 1 ns longer. Business calendars using the legacy format must be upgraded to the current format to use <includeClose>true</includeClose>.

@alexpeters1208
Copy link

alexpeters1208 commented Sep 9, 2024

It's not obvious that anything needs to change in the Pydocs, since business day length is not explicitly mentioned anywhere.

It is also not obvious that anything needs to change in the Calendars guide.

However, something may need to change here. In particular, some methods like businessDates have parameter names startInclusive and endInclusive. Have the "defaults" on the second parameter implicitly changed?
Many other methods like businessDates have start and end parameters, both called "inclusive". Is it now the case that start is inclusive and end is exclusive across the board?

@chipkent
Copy link
Member

chipkent commented Sep 9, 2024

The BusinessCalendar methods you mention do not need to change. They do exactly what they say. They have not changed.

The bug was in how TimeRange was being computed. Off-by-one. See https://github.com/deephaven/deephaven-core/pull/5648/files#diff-54ece697448ad466b3a5b659e70da856bbe21c18973c787e62c2506ba8b68fc5.

The only other change of note -- which may require documentation --, is some additional configurability in the calendar XML files. Also see https://github.com/deephaven/deephaven-core/pull/5648/files#diff-54ece697448ad466b3a5b659e70da856bbe21c18973c787e62c2506ba8b68fc5.

@alexpeters1208
Copy link

This is the only real place I can find config details on these XML files: https://deephaven.io/core/javadoc/io/deephaven/time/calendar/BusinessCalendarXMLParser.html
The currently released Javadocs do not include this change, but Chip included the Javadoc change in his PR, and it will be in the next release.

The other "docs" is this example, linked to from the Calendars guide: https://github.com/deephaven/deephaven-core/blob/main/props/configs/src/main/resources/calendar/UTC.calendar
It is already updated to include the new config stuff.

I can't find anywhere else where we document config details. The calendars guide just points to the examples in GH, and if you're looking, you can find the Javadoc. If we want docsite docs for these config details, that is a bigger PR than this ticket warrants.

@chipkent
Copy link
Member

Looks like there is no work to do for this PR.

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

No branches or pull requests

5 participants