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.

Sign up for GitHub

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 #5648

Merged
merged 7 commits into from
Aug 28, 2024
+77 −40
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
938b00a
Fix an off-by-one error in TimeRange.
chipkent Jun 21, 2024
bfddbdf
Spotless
chipkent Jun 21, 2024
ccc5b4f
Merge remote-tracking branch 'upstream/main' into timerange_off_by_one
chipkent Jun 21, 2024
c0861ac
By default, make the trading day exclusive of the last time in a rang…
chipkent Jun 24, 2024
ada7dd7
Add calendar configurtaions for including or excluding the final time…
chipkent Jun 24, 2024
c6e4fc8
For legacy calendars, set the timerange to be exclusive end so that t…
chipkent Jun 25, 2024
e603d48
Merge remote-tracking branch 'origin/main' into timerange_off_by_one
chipkent Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions engine/time/src/main/java/io/deephaven/time/calendar/BusinessCalendarXMLParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,12 @@
* <description>New York Stock Exchange Calendar</description>
* <timeZone>America/New_York</timeZone>
* <default>
* <businessTime><open>09:30</open><close>16:00</close></businessTime>
* <businessTime>
* <open>09:30</open>
* <close>16:00</close>
* <!-- Optional include the close nanosecond in the business time range. -->
* <includeClose>true</includeClose>
* </businessTime>
* <weekend>Saturday</weekend>
* <weekend>Sunday</weekend>
* </default>
Expand All @@ -51,6 +56,8 @@
* <businessTime>
* <open>09:30</open>
* <close>13:00</close>
* <!-- Optional include the close nanosecond in the business time range. -->
* <includeClose>true</includeClose>
* </businessTime>
* </holiday>
* </calendar>
Expand Down Expand Up @@ -254,6 +261,7 @@ private static TimeRange<LocalTime>[] parseBusinessRanges(final List<Element> bu
for (int i = 0; i < businessRanges.size(); i++) {
final String openTxt = getText(getRequiredChild(businessRanges.get(i), "open"));
final String closeTxt = getText(getRequiredChild(businessRanges.get(i), "close"));
final String includeCloseTxt = getText(businessRanges.get(i).getChild("includeClose"));

if (closeTxt.startsWith("24:00")) {
throw new RuntimeException("Close time (" + closeTxt
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

I seem to recall some discussion involving DHE? Maybe Japan business day, or some other international concerns?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right that there may have been a UTC discussion. I'll look at that case.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

final boolean inclusiveEnd = Boolean.parseBoolean(includeCloseTxt); // defaults to false
Copy link
Member

Choose a reason for hiding this comment

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

Should use true by default to match our previous behavior? (and fix the test cases by setting to false where appropriate...)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

I've asked DHE to take a look at how this effects them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question on the legacy path.

These are the key parts for the legacy parsing of TimeRange:

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

The PR description has been updated to reflect the breaking change.

rst[i] = new TimeRange<>(open, close, inclusiveEnd);
}

return rst;
Expand All @@ -282,7 +291,7 @@ private static TimeRange<LocalTime>[] parseBusinessRangesLegacy(final List<Eleme
final String closeTxt = openClose[1];
final LocalTime open = DateTimeUtils.parseLocalTime(openTxt);
final LocalTime close = DateTimeUtils.parseLocalTime(closeTxt);
rst[i] = new TimeRange<>(open, close, true);
rst[i] = new TimeRange<>(open, close, false);
} else {
throw new IllegalArgumentException("Can not parse business periods; open/close = " + br.getText());
}
Expand Down
2 changes: 1 addition & 1 deletion engine/time/src/main/java/io/deephaven/time/calendar/TimeRange.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public boolean isInclusiveEnd() {
* @return length of the range in nanoseconds
*/
public long nanos() {
return start.until(end, ChronoUnit.NANOS) - (inclusiveEnd ? 0 : 1);
return start.until(end, ChronoUnit.NANOS) + (inclusiveEnd ? 1 : 0);
}

/**
Expand Down
3 changes: 3 additions & 0 deletions engine/time/src/test/java/io/deephaven/time/calendar/TestBusinessCalendarXMLParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ public static void assertParserTestCal(final BusinessCalendar cal) {
cal.calendarDay("2015-04-06").businessStart());
assertEquals(DateTimeUtils.parseInstant("2015-04-06T16:46 Asia/Tokyo"),
cal.calendarDay("2015-04-06").businessEnd());

assertTrue(cal.calendarDay("2015-04-06").isInclusiveEnd());
assertFalse(cal.calendarDay("2015-04-07").isInclusiveEnd());
}

public void testLoad() throws URISyntaxException {
Expand Down
60 changes: 30 additions & 30 deletions engine/time/src/test/java/io/deephaven/time/calendar/TestCalendarDay.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,10 @@ public void testSinglePeriod() {
assertEquals(close1, single.businessEnd());
assertEquals(close1, single.businessEnd());
assertTrue(single.isInclusiveEnd());
assertEquals(DateTimeUtils.HOUR, single.businessNanos());
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR), single.businessDuration());
assertEquals(DateTimeUtils.HOUR, single.businessNanos());
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR), single.businessDuration());
assertEquals(DateTimeUtils.HOUR + 1, single.businessNanos());
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR + 1), single.businessDuration());
assertEquals(DateTimeUtils.HOUR + 1, single.businessNanos());
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR + 1), single.businessDuration());
assertTrue(single.isBusinessDay());
assertTrue(single.isBusinessTime(DateTimeUtils.parseInstant("2017-03-11T10:00:00.000000000 NY")));
assertTrue(single.isBusinessTime(DateTimeUtils.parseInstant("2017-03-11T10:15:00.000000000 NY")));
Expand All @@ -77,13 +77,13 @@ public void testSinglePeriod() {
single.businessNanosElapsed(DateTimeUtils.parseInstant("2017-03-11T10:30:00.000000000 NY")));
assertEquals(Duration.ofNanos(DateTimeUtils.MINUTE * 30),
single.businessDurationElapsed(DateTimeUtils.parseInstant("2017-03-11T10:30:00.000000000 NY")));
assertEquals(DateTimeUtils.HOUR,
assertEquals(DateTimeUtils.HOUR + 1,
single.businessNanosElapsed(DateTimeUtils.parseInstant("2017-03-11T13:00:00.000000000 NY")));
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR),
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR + 1),
single.businessDurationElapsed(DateTimeUtils.parseInstant("2017-03-11T13:00:00.000000000 NY")));
assertEquals(DateTimeUtils.MINUTE * 30,
assertEquals(DateTimeUtils.MINUTE * 30 + 1,
single.businessNanosRemaining(DateTimeUtils.parseInstant("2017-03-11T10:30:00.000000000 NY")));
assertEquals(Duration.ofNanos(DateTimeUtils.MINUTE * 30),
assertEquals(Duration.ofNanos(DateTimeUtils.MINUTE * 30 + 1),
single.businessDurationRemaining(DateTimeUtils.parseInstant("2017-03-11T10:30:00.000000000 NY")));
assertEquals(0L,
single.businessNanosRemaining(DateTimeUtils.parseInstant("2017-03-11T13:00:00.000000000 NY")));
Expand All @@ -105,10 +105,10 @@ public void testMultiPeriod() {
assertEquals(close2, multi.businessEnd());
assertEquals(close2, multi.businessEnd());
assertTrue(multi.isInclusiveEnd());
assertEquals(DateTimeUtils.HOUR * 6, multi.businessNanos());
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR * 6), multi.businessDuration());
assertEquals(DateTimeUtils.HOUR * 6, multi.businessNanos());
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR * 6), multi.businessDuration());
assertEquals(DateTimeUtils.HOUR * 6 + 2, multi.businessNanos());
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR * 6 + 2), multi.businessDuration());
assertEquals(DateTimeUtils.HOUR * 6 + 2, multi.businessNanos());
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR * 6 + 2), multi.businessDuration());
assertTrue(multi.isBusinessDay());
assertTrue(multi.isBusinessTime(DateTimeUtils.parseInstant("2017-03-11T10:00:00.000000000 NY")));
assertTrue(multi.isBusinessTime(DateTimeUtils.parseInstant("2017-03-11T10:15:00.000000000 NY")));
Expand All @@ -122,25 +122,25 @@ public void testMultiPeriod() {
multi.businessNanosElapsed(DateTimeUtils.parseInstant("2017-03-11T10:30:00.000000000 NY")));
assertEquals(Duration.ofNanos(DateTimeUtils.MINUTE * 30),
multi.businessDurationElapsed(DateTimeUtils.parseInstant("2017-03-11T10:30:00.000000000 NY")));
assertEquals(DateTimeUtils.HOUR * 2,
assertEquals(DateTimeUtils.HOUR * 2 + 1,
multi.businessNanosElapsed(DateTimeUtils.parseInstant("2017-03-11T13:00:00.000000000 NY")));
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR * 2),
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR * 2 + 1),
multi.businessDurationElapsed(DateTimeUtils.parseInstant("2017-03-11T13:00:00.000000000 NY")));
assertEquals(DateTimeUtils.HOUR * 2,
assertEquals(DateTimeUtils.HOUR * 2 + 1,
multi.businessNanosElapsed(DateTimeUtils.parseInstant("2017-03-11T13:00:00.000000000 NY")));
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR * 2),
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR * 2 + 1),
multi.businessDurationElapsed(DateTimeUtils.parseInstant("2017-03-11T13:00:00.000000000 NY")));
assertEquals(DateTimeUtils.HOUR * 6,
assertEquals(DateTimeUtils.HOUR * 6 + 2,
multi.businessNanosRemaining(DateTimeUtils.parseInstant("2017-03-11T01:00:00.000000000 NY")));
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR * 6),
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR * 6 + 2),
multi.businessDurationRemaining(DateTimeUtils.parseInstant("2017-03-11T01:00:00.000000000 NY")));
assertEquals(DateTimeUtils.HOUR * 5 + DateTimeUtils.MINUTE * 30,
assertEquals(DateTimeUtils.HOUR * 5 + DateTimeUtils.MINUTE * 30 + 2,
multi.businessNanosRemaining(DateTimeUtils.parseInstant("2017-03-11T10:30:00.000000000 NY")));
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR * 5 + DateTimeUtils.MINUTE * 30),
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR * 5 + DateTimeUtils.MINUTE * 30 + 2),
multi.businessDurationRemaining(DateTimeUtils.parseInstant("2017-03-11T10:30:00.000000000 NY")));
assertEquals(DateTimeUtils.HOUR * 4,
assertEquals(DateTimeUtils.HOUR * 4 + 1,
multi.businessNanosRemaining(DateTimeUtils.parseInstant("2017-03-11T13:00:00.000000000 NY")));
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR * 4),
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR * 4 + 1),
multi.businessDurationRemaining(DateTimeUtils.parseInstant("2017-03-11T13:00:00.000000000 NY")));

assertEquals(NULL_LONG, multi.businessNanosElapsed(null));
Expand All @@ -156,10 +156,10 @@ public void testMultiPeriod() {
assertEquals(close2, multi2.businessEnd());
assertEquals(close2, multi2.businessEnd());
assertTrue(multi2.isInclusiveEnd());
assertEquals(DateTimeUtils.HOUR * 6, multi2.businessNanos());
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR * 6), multi2.businessDuration());
assertEquals(DateTimeUtils.HOUR * 6, multi2.businessNanos());
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR * 6), multi2.businessDuration());
assertEquals(DateTimeUtils.HOUR * 6 + 2, multi2.businessNanos());
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR * 6 + 2), multi2.businessDuration());
assertEquals(DateTimeUtils.HOUR * 6 + 2, multi2.businessNanos());
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR * 6 + 2), multi2.businessDuration());
assertTrue(multi2.isBusinessDay());
assertTrue(multi2.isBusinessTime(DateTimeUtils.parseInstant("2017-03-11T10:00:00.000000000 NY")));
assertTrue(multi2.isBusinessTime(DateTimeUtils.parseInstant("2017-03-11T10:15:00.000000000 NY")));
Expand All @@ -173,13 +173,13 @@ public void testMultiPeriod() {
multi2.businessNanosElapsed(DateTimeUtils.parseInstant("2017-03-11T10:30:00.000000000 NY")));
assertEquals(Duration.ofNanos(DateTimeUtils.MINUTE * 30),
multi2.businessDurationElapsed(DateTimeUtils.parseInstant("2017-03-11T10:30:00.000000000 NY")));
assertEquals(DateTimeUtils.HOUR * 2,
assertEquals(DateTimeUtils.HOUR * 2 + 1,
multi2.businessNanosElapsed(DateTimeUtils.parseInstant("2017-03-11T13:00:00.000000000 NY")));
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR * 2),
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR * 2 + 1),
multi2.businessDurationElapsed(DateTimeUtils.parseInstant("2017-03-11T13:00:00.000000000 NY")));
assertEquals(DateTimeUtils.HOUR * 2,
assertEquals(DateTimeUtils.HOUR * 2 + 1,
multi2.businessNanosElapsed(DateTimeUtils.parseInstant("2017-03-11T13:00:00.000000000 NY")));
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR * 2),
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR * 2 + 1),
multi2.businessDurationElapsed(DateTimeUtils.parseInstant("2017-03-11T13:00:00.000000000 NY")));

assertEquals(NULL_LONG, multi2.businessNanosElapsed(null));
Expand Down
10 changes: 10 additions & 0 deletions engine/time/src/test/java/io/deephaven/time/calendar/TestCalendars.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import io.deephaven.base.testing.BaseArrayTestCase;
import io.deephaven.configuration.Configuration;
import io.deephaven.time.DateTimeUtils;

import java.net.URISyntaxException;
import java.nio.file.Paths;
Expand Down Expand Up @@ -82,6 +83,15 @@ public void testAdd() throws URISyntaxException {
} catch (Exception e) {
// pass
}
}

public void testUTCDayLength() {
final BusinessCalendar cal = Calendars.calendar("UTC");
assertEquals(DateTimeUtils.DAY, cal.standardBusinessDay().businessNanos());
}

public void testNYSEDayLength() {
final BusinessCalendar cal = Calendars.calendar("USNYSE_EXAMPLE");
assertEquals(6 * DateTimeUtils.HOUR + 30 * DateTimeUtils.MINUTE, cal.standardBusinessDay().businessNanos());
}
}
23 changes: 19 additions & 4 deletions engine/time/src/test/java/io/deephaven/time/calendar/TestTimeRange.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ public void testTimeRangeInclusive() {
assertEquals(open1, period.start());
assertEquals(close1, period.end());
assertTrue(period.isInclusiveEnd());
assertEquals(DateTimeUtils.HOUR, period.nanos());
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR), period.duration());
assertEquals(DateTimeUtils.HOUR + 1, period.nanos());
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR + 1), period.duration());

assertTrue(period.contains(open1));
assertTrue(period
Expand Down Expand Up @@ -101,8 +101,8 @@ public void testTimeRangeExclusive() {
assertEquals(open1, period.start());
assertEquals(close1, period.end());
assertFalse(period.isInclusiveEnd());
assertEquals(DateTimeUtils.HOUR - 1, period.nanos());
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR - 1), period.duration());
assertEquals(DateTimeUtils.HOUR, period.nanos());
assertEquals(Duration.ofNanos(DateTimeUtils.HOUR), period.duration());

assertTrue(period.contains(open1));
assertTrue(period
Expand Down Expand Up @@ -192,4 +192,19 @@ public void testToStringExclusive() {
final TimeRange<LocalTime> p1 = new TimeRange<>(start, end, false);
assertEquals("TimeRange{start=01:02:03, end=07:08:09, inclusiveEnd=false}", p1.toString());
}

public void testNanos() {
final Instant t1 = DateTimeUtils.epochMillisToInstant(0);
final Instant t2 = DateTimeUtils.epochMillisToInstant(1);

final TimeRange<Instant> pInclusive = new TimeRange<>(t1, t2, true);
final TimeRange<Instant> pExclusive = new TimeRange<>(t1, t2, false);

final long nInclusive = pInclusive.nanos();
final long nExclusive = pExclusive.nanos();

assertNotEquals(nInclusive, nExclusive);
assertEquals(DateTimeUtils.MILLI, nExclusive);
assertEquals(DateTimeUtils.MILLI + 1, nInclusive);
}
}
2 changes: 1 addition & 1 deletion engine/time/src/test/resources/PARSER-TEST.calendar
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,6 @@
</holiday>
<holiday>
<date>2015-04-06</date>
<businessTime><open>14:15</open><close>16:46</close></businessTime>
<businessTime><open>14:15</open><close>16:46</close><includeClose>true</includeClose></businessTime>
</holiday>
</calendar>
2 changes: 1 addition & 1 deletion props/configs/src/main/resources/calendar/UTC.calendar
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@
<firstValidDate>1900-01-01</firstValidDate>
<lastValidDate>2100-12-31</lastValidDate>
<default>
<businessTime><open>00:00</open><close>23:59:59.999999999</close></businessTime>
<businessTime><open>00:00</open><close>23:59:59.999999999</close><includeClose>true</includeClose></businessTime>
</default>
</calendar>