fix!: Fix a TimeRange off-by-one bug in nanosecond calculation #5648
Conversation
chipkent
changed the title
|
By default calendars are created with inclusive ranges. This fix will make the trading day 1 ns longer, unless we decide to make it exclusive. |
chipkent
added
NoDocumentationNeeded
NoReleaseNotesNeeded
…e. This makes the trading day the expected length instead of being 1 ns longer.
| @@ -262,7 +262,7 @@ private static TimeRange<LocalTime>[] parseBusinessRanges(final List<Element> bu | |||
|
|
|||
| final LocalTime open = DateTimeUtils.parseLocalTime(openTxt); | |||
| final LocalTime close = DateTimeUtils.parseLocalTime(closeTxt); | |||
| rst[i] = new TimeRange<>(open, close, true); | |||
There was a problem hiding this comment.
Wasn't this set to true so that "23:59:59.999999999" would include the last nanosecond in the day?
Right now, it looks like we can't represent a full day?
There was a problem hiding this comment.
I seem to recall some discussion involving DHE? Maybe Japan business day, or some other international concerns?
There was a problem hiding this comment.
You're right that there may have been a UTC discussion. I'll look at that case.
There was a problem hiding this comment.
The fix in there allows UTC to be supported correctly with full 24 hour days. It also gives users control over how to count the close times.
… of a range. This allows UTC calendars to be supported and gives users configuration options.
| @@ -262,7 +270,8 @@ private static TimeRange<LocalTime>[] parseBusinessRanges(final List<Element> bu | |||
|
|
|||
| final LocalTime open = DateTimeUtils.parseLocalTime(openTxt); | |||
| final LocalTime close = DateTimeUtils.parseLocalTime(closeTxt); | |||
| rst[i] = new TimeRange<>(open, close, true); | |||
| final boolean inclusiveEnd = Boolean.parseBoolean(includeCloseTxt); // defaults to false | |||
There was a problem hiding this comment.
Should use true by default to match our previous behavior? (and fix the test cases by setting to false where appropriate...)
There was a problem hiding this comment.
true really doesn't match the prior behavior. It yields a business time that is 1 ns longer. false is closer to the prior behavior.
There was a problem hiding this comment.
The TimeRange#nanos being incorrect doesn't before doesn't change the fact that up until now, callers might be relying on parsing returning io.deephaven.time.calendar.TimeRange#isInclusiveEnd as true.
Also, does this code path get hit when parsing a "legacy" calendar file?
There was a problem hiding this comment.
I've asked DHE to take a look at how this effects them.
There was a problem hiding this comment.
Good question on the legacy path.
These are the key parts for the legacy parsing of TimeRange:
- https://github.com/chipkent/deephaven-core/blob/timerange_off_by_one/engine/time/src/main/java/io/deephaven/time/calendar/BusinessCalendarXMLParser.java#L249
- https://github.com/chipkent/deephaven-core/blob/timerange_off_by_one/engine/time/src/main/java/io/deephaven/time/calendar/BusinessCalendarXMLParser.java#L294
The "legacy" and "new" TimeRange formats are parsed separately, so we could handle them differently if we want to. Currently, legacy has inclusiveEnd=true. This will result in a trading day that is 1ns longer than the current implementation, so in this case, I also advocate for setting inclusiveEnd=false so that the business day length does not change. The legacy format for time ranges is not pure XML, so adding in inclusive controls is a little more messy, and I would prefer to avoid it.
I'm going to commit the inclusiveEnd=false change for legacy, and I'm adding @rbasralian for thoughts on these changes since he deals with calendars.
There was a problem hiding this comment.
The PR description has been updated to reflect the breaking change.
…he business day length doesn't change.
chipkent
changed the title
There was a problem hiding this comment.
Looks good to me.
Not strictly related to this change, but It would be nice to have comments around multi and multi2 in TestCalendarDay.testMultiPeriod; the 1 and 2 ns offsets were a bit tough to follow. Just something along the lines of // multi/multi2 are constructed from periods inclusive of the ends, so each period includes an extra nanosecond
(And I guess the point of multi2 is that it sorts the periods you create it with, so it's equivalent to multi?)
chipkent
added
ReleaseNotesNeeded
|
Labels indicate documentation is required. Issues for documentation have been opened: Community: deephaven/deephaven-docs-community#302 |
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>.