From fcf8b706d8416577abb6c838ba06be96a92a115f Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Thu, 24 Sep 2020 10:10:19 -0400 Subject: [PATCH] Speed up date_histogram by precomputing ranges (#61467) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A few of us were talking about ways to speed up the `date_histogram` using the index for the timestamp rather than the doc values. To do that we'd have to pre-compute all of the "round down" points in the index. It turns out that *just* precomputing those values speeds up rounding fairly significantly: ``` Benchmark (count) (interval) (range) (zone) Mode Cnt Score Error Units before 10000000 calendar month 2000-10-28 to 2000-10-31 UTC avgt 10 96461080.982 ± 616373.011 ns/op before 10000000 calendar month 2000-10-28 to 2000-10-31 America/New_York avgt 10 130598950.850 ± 1249189.867 ns/op after 10000000 calendar month 2000-10-28 to 2000-10-31 UTC avgt 10 52311775.080 ± 107171.092 ns/op after 10000000 calendar month 2000-10-28 to 2000-10-31 America/New_York avgt 10 54800134.968 ± 373844.796 ns/op ``` That's a 46% speed up when there isn't a time zone and a 58% speed up when there is. This doesn't work for every time zone, specifically those that have two midnights in a single day due to daylight savings time will produce wonky results. So they don't get the optimization. Second, this requires a few expensive computation up front to make the transition array. And if the transition array is too large then we give up and use the original mechanism, throwing away all of the work we did to build the array. This seems appropriate for most usages of `round`, but this change uses it for *all* usages of `round`. That seems ok for now, but it might be worth investigating in a follow up. I ran a macrobenchmark as well which showed an 11% preformance improvement. *BUT* the benchmark wasn't tuned for my desktop so it overwhelmed it and might have produced "funny" results. I think it is pretty clear that this is an improvement, but know the measurement is weird: ``` Benchmark (count) (interval) (range) (zone) Mode Cnt Score Error Units before 10000000 calendar month 2000-10-28 to 2000-10-31 UTC avgt 10 96461080.982 ± 616373.011 ns/op before 10000000 calendar month 2000-10-28 to 2000-10-31 America/New_York avgt 10 g± 1249189.867 ns/op after 10000000 calendar month 2000-10-28 to 2000-10-31 UTC avgt 10 52311775.080 ± 107171.092 ns/op after 10000000 calendar month 2000-10-28 to 2000-10-31 America/New_York avgt 10 54800134.968 ± 373844.796 ns/op Before: | Min Throughput | hourly_agg | 0.11 | ops/s | | Median Throughput | hourly_agg | 0.11 | ops/s | | Max Throughput | hourly_agg | 0.11 | ops/s | | 50th percentile latency | hourly_agg | 650623 | ms | | 90th percentile latency | hourly_agg | 821478 | ms | | 99th percentile latency | hourly_agg | 859780 | ms | | 100th percentile latency | hourly_agg | 864030 | ms | | 50th percentile service time | hourly_agg | 9268.71 | ms | | 90th percentile service time | hourly_agg | 9380 | ms | | 99th percentile service time | hourly_agg | 9626.88 | ms | |100th percentile service time | hourly_agg | 9884.27 | ms | | error rate | hourly_agg | 0 | % | After: | Min Throughput | hourly_agg | 0.12 | ops/s | | Median Throughput | hourly_agg | 0.12 | ops/s | | Max Throughput | hourly_agg | 0.12 | ops/s | | 50th percentile latency | hourly_agg | 519254 | ms | | 90th percentile latency | hourly_agg | 653099 | ms | | 99th percentile latency | hourly_agg | 683276 | ms | | 100th percentile latency | hourly_agg | 686611 | ms | | 50th percentile service time | hourly_agg | 8371.41 | ms | | 90th percentile service time | hourly_agg | 8407.02 | ms | | 99th percentile service time | hourly_agg | 8536.64 | ms | |100th percentile service time | hourly_agg | 8538.54 | ms | | error rate | hourly_agg | 0 | % | ``` --- .../elasticsearch/common/LocalTimeOffset.java | 80 +++++++- .../org/elasticsearch/common/Rounding.java | 97 +++++++++- .../common/LocalTimeOffsetTests.java | 34 +++- .../elasticsearch/common/RoundingTests.java | 171 +++++++++++++----- 4 files changed, 327 insertions(+), 55 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/LocalTimeOffset.java b/server/src/main/java/org/elasticsearch/common/LocalTimeOffset.java index dd69244a00b78..78394ec32956c 100644 --- a/server/src/main/java/org/elasticsearch/common/LocalTimeOffset.java +++ b/server/src/main/java/org/elasticsearch/common/LocalTimeOffset.java @@ -21,6 +21,7 @@ import java.time.Instant; import java.time.ZoneId; +import java.time.temporal.ChronoField; import java.time.zone.ZoneOffsetTransition; import java.time.zone.ZoneOffsetTransitionRule; import java.time.zone.ZoneRules; @@ -173,6 +174,12 @@ public interface Strategy { */ protected abstract LocalTimeOffset offsetContaining(long utcMillis); + /** + * Does this transition or any previous transitions move back to the + * previous day? See {@link Lookup#anyMoveBackToPreviousDay()} for rules. + */ + protected abstract boolean anyMoveBackToPreviousDay(); + @Override public String toString() { return toString(millis); @@ -194,6 +201,15 @@ public abstract static class Lookup { */ public abstract LocalTimeOffset fixedInRange(long minUtcMillis, long maxUtcMillis); + /** + * Do any of the transitions move back to the previous day? + *

+ * Note: If an overlap occurs at, say, 1 am and jumps back to + * exactly midnight then it doesn't count because + * midnight is still counted as being in the "next" day. + */ + public abstract boolean anyMoveBackToPreviousDay(); + /** * The number of offsets in the lookup. Package private for testing. */ @@ -224,6 +240,11 @@ protected LocalTimeOffset offsetContaining(long utcMillis) { return this; } + @Override + protected boolean anyMoveBackToPreviousDay() { + return false; + } + @Override protected String toString(long millis) { return Long.toString(millis); @@ -297,6 +318,11 @@ public long firstMissingLocalTime() { return firstMissingLocalTime; } + @Override + protected boolean anyMoveBackToPreviousDay() { + return previous().anyMoveBackToPreviousDay(); + } + @Override protected String toString(long millis) { return "Gap of " + millis + "@" + Instant.ofEpochMilli(startUtcMillis()); @@ -306,13 +332,21 @@ protected String toString(long millis) { public static class Overlap extends Transition { private final long firstOverlappingLocalTime; private final long firstNonOverlappingLocalTime; - - private Overlap(long millis, LocalTimeOffset previous, long startUtcMillis, - long firstOverlappingLocalTime, long firstNonOverlappingLocalTime) { + private final boolean movesBackToPreviousDay; + + private Overlap( + long millis, + LocalTimeOffset previous, + long startUtcMillis, + long firstOverlappingLocalTime, + long firstNonOverlappingLocalTime, + boolean movesBackToPreviousDay + ) { super(millis, previous, startUtcMillis); this.firstOverlappingLocalTime = firstOverlappingLocalTime; this.firstNonOverlappingLocalTime = firstNonOverlappingLocalTime; assert firstOverlappingLocalTime < firstNonOverlappingLocalTime; + this.movesBackToPreviousDay = movesBackToPreviousDay; } @Override @@ -340,6 +374,11 @@ public long firstOverlappingLocalTime() { return firstOverlappingLocalTime; } + @Override + protected boolean anyMoveBackToPreviousDay() { + return movesBackToPreviousDay || previous().anyMoveBackToPreviousDay(); + } + @Override protected String toString(long millis) { return "Overlap of " + millis + "@" + Instant.ofEpochMilli(startUtcMillis()); @@ -374,6 +413,11 @@ int size() { public String toString() { return String.format(Locale.ROOT, "FixedLookup[for %s at %s]", zone, fixed); } + + @Override + public boolean anyMoveBackToPreviousDay() { + return false; + } } /** @@ -405,6 +449,11 @@ public LocalTimeOffset innerLookup(long utcMillis) { int size() { return size; } + + @Override + public boolean anyMoveBackToPreviousDay() { + return lastOffset.anyMoveBackToPreviousDay(); + } } /** @@ -452,6 +501,11 @@ int size() { return offsets.length; } + @Override + public boolean anyMoveBackToPreviousDay() { + return offsets[offsets.length - 1].anyMoveBackToPreviousDay(); + } + @Override public String toString() { return String.format(Locale.ROOT, "TransitionArrayLookup[for %s between %s and %s]", @@ -504,7 +558,25 @@ protected static Transition buildTransition(ZoneOffsetTransition transition, Loc } long firstOverlappingLocalTime = utcStart + offsetAfterMillis; long firstNonOverlappingLocalTime = utcStart + offsetBeforeMillis; - return new Overlap(offsetAfterMillis, previous, utcStart, firstOverlappingLocalTime, firstNonOverlappingLocalTime); + return new Overlap( + offsetAfterMillis, + previous, + utcStart, + firstOverlappingLocalTime, + firstNonOverlappingLocalTime, + movesBackToPreviousDay(transition) + ); + } + + private static boolean movesBackToPreviousDay(ZoneOffsetTransition transition) { + if (transition.getDateTimeBefore().getDayOfMonth() == transition.getDateTimeAfter().getDayOfMonth()) { + return false; + } + if (transition.getDateTimeBefore().getLong(ChronoField.NANO_OF_DAY) == 0L) { + // If we change *at* midnight this is ok. + return false; + } + return true; } } diff --git a/server/src/main/java/org/elasticsearch/common/Rounding.java b/server/src/main/java/org/elasticsearch/common/Rounding.java index 9fa4d2aff9575..e026cbaabe8b5 100644 --- a/server/src/main/java/org/elasticsearch/common/Rounding.java +++ b/server/src/main/java/org/elasticsearch/common/Rounding.java @@ -18,6 +18,7 @@ */ package org.elasticsearch.common; +import org.apache.lucene.util.ArrayUtil; import org.elasticsearch.ElasticsearchException; import org.elasticsearch.Version; import org.elasticsearch.common.LocalTimeOffset.Gap; @@ -44,6 +45,7 @@ import java.time.temporal.TemporalQueries; import java.time.zone.ZoneOffsetTransition; import java.time.zone.ZoneRules; +import java.util.Arrays; import java.util.List; import java.util.Locale; import java.util.Objects; @@ -405,6 +407,34 @@ public Rounding build() { } } + private abstract class PreparedRounding implements Prepared { + /** + * Attempt to build a {@link Prepared} implementation that relies on pre-calcuated + * "round down" points. If there would be more than {@code max} points then return + * the original implementation, otherwise return the new, faster implementation. + */ + protected Prepared maybeUseArray(long minUtcMillis, long maxUtcMillis, int max) { + long[] values = new long[1]; + long rounded = round(minUtcMillis); + int i = 0; + values[i++] = rounded; + while ((rounded = nextRoundingValue(rounded)) <= maxUtcMillis) { + if (i >= max) { + return this; + } + /* + * We expect a time in the last transition (rounded - 1) to round + * to the last value we calculated. If it doesn't then we're + * probably doing something wrong here.... + */ + assert values[i - 1] == round(rounded - 1); + values = ArrayUtil.grow(values, i + 1); + values[i++]= rounded; + } + return new ArrayRounding(values, i, this); + } + } + static class TimeUnitRounding extends Rounding { static final byte ID = 1; @@ -474,6 +504,15 @@ private LocalDateTime truncateLocalDateTime(LocalDateTime localDateTime) { @Override public Prepared prepare(long minUtcMillis, long maxUtcMillis) { + /* + * 128 is a power of two that isn't huge. We might be able to do + * better if the limit was based on the actual type of prepared + * rounding but this'll do for now. + */ + return prepareOffsetOrJavaTimeRounding(minUtcMillis, maxUtcMillis).maybeUseArray(minUtcMillis, maxUtcMillis, 128); + } + + private TimeUnitPreparedRounding prepareOffsetOrJavaTimeRounding(long minUtcMillis, long maxUtcMillis) { long minLookup = minUtcMillis - unit.extraLocalOffsetLookup(); long maxLookup = maxUtcMillis; @@ -492,7 +531,6 @@ public Prepared prepare(long minUtcMillis, long maxUtcMillis) { // Range too long, just use java.time return prepareJavaTime(); } - LocalTimeOffset fixedOffset = lookup.fixedInRange(minLookup, maxLookup); if (fixedOffset != null) { // The time zone is effectively fixed @@ -521,7 +559,7 @@ public Prepared prepareForUnknown() { } @Override - Prepared prepareJavaTime() { + TimeUnitPreparedRounding prepareJavaTime() { if (unitRoundsToMidnight) { return new JavaTimeToMidnightRounding(); } @@ -560,7 +598,7 @@ public String toString() { return "Rounding[" + unit + " in " + timeZone + "]"; } - private abstract class TimeUnitPreparedRounding implements Prepared { + private abstract class TimeUnitPreparedRounding extends PreparedRounding { @Override public double roundingSize(long utcMillis, DateTimeUnit timeUnit) { if (timeUnit.isMillisBased == unit.isMillisBased) { @@ -654,6 +692,14 @@ public long inOverlap(long localMillis, Overlap overlap) { public long beforeOverlap(long localMillis, Overlap overlap) { return overlap.previous().localToUtc(localMillis, this); } + + @Override + protected Prepared maybeUseArray(long minUtcMillis, long maxUtcMillis, int max) { + if (lookup.anyMoveBackToPreviousDay()) { + return this; + } + return super.maybeUseArray(minUtcMillis, maxUtcMillis, max); + } } private class NotToMidnightRounding extends AbstractNotToMidnightRounding implements LocalTimeOffset.Strategy { @@ -713,6 +759,12 @@ public long nextRoundingValue(long utcMillis) { return firstTimeOnDay(localMidnight); } + @Override + protected Prepared maybeUseArray(long minUtcMillis, long maxUtcMillis, int max) { + // We don't have the right information needed to know if this is safe for this time zone so we always use java rounding + return this; + } + private long firstTimeOnDay(LocalDateTime localMidnight) { assert localMidnight.toLocalTime().equals(LocalTime.of(0, 0, 0)) : "firstTimeOnDay should only be called at midnight"; @@ -1121,7 +1173,7 @@ public byte id() { @Override public Prepared prepare(long minUtcMillis, long maxUtcMillis) { - return wrapPreparedRounding(delegate.prepare(minUtcMillis, maxUtcMillis)); + return wrapPreparedRounding(delegate.prepare(minUtcMillis - offset, maxUtcMillis - offset)); } @Override @@ -1196,4 +1248,41 @@ public static Rounding read(StreamInput in) throws IOException { throw new ElasticsearchException("unknown rounding id [" + id + "]"); } } + + /** + * Implementation of {@link Prepared} using pre-calculated "round down" points. + */ + private static class ArrayRounding implements Prepared { + private final long[] values; + private final int max; + private final Prepared delegate; + + private ArrayRounding(long[] values, int max, Prepared delegate) { + this.values = values; + this.max = max; + this.delegate = delegate; + } + + @Override + public long round(long utcMillis) { + assert values[0] <= utcMillis : "utcMillis must be after " + values[0]; + int idx = Arrays.binarySearch(values, 0, max, utcMillis); + assert idx != -1 : "The insertion point is before the array! This should have tripped the assertion above."; + assert -1 - idx <= values.length : "This insertion point is after the end of the array."; + if (idx < 0) { + idx = -2 - idx; + } + return values[idx]; + } + + @Override + public long nextRoundingValue(long utcMillis) { + return delegate.nextRoundingValue(utcMillis); + } + + @Override + public double roundingSize(long utcMillis, DateTimeUnit timeUnit) { + return delegate.roundingSize(utcMillis, timeUnit); + } + } } diff --git a/server/src/test/java/org/elasticsearch/common/LocalTimeOffsetTests.java b/server/src/test/java/org/elasticsearch/common/LocalTimeOffsetTests.java index 2b762500f5267..1455c3c0db062 100644 --- a/server/src/test/java/org/elasticsearch/common/LocalTimeOffsetTests.java +++ b/server/src/test/java/org/elasticsearch/common/LocalTimeOffsetTests.java @@ -19,6 +19,7 @@ package org.elasticsearch.common; +import org.elasticsearch.bootstrap.JavaVersion; import org.elasticsearch.common.LocalTimeOffset.Gap; import org.elasticsearch.common.LocalTimeOffset.Overlap; import org.elasticsearch.common.time.DateFormatter; @@ -51,20 +52,22 @@ public void testNotFixed() { } public void testUtc() { - assertFixOffset(ZoneId.of("UTC"), 0); + assertFixedOffset(ZoneId.of("UTC"), 0); } public void testFixedOffset() { ZoneOffset zone = ZoneOffset.ofTotalSeconds(between((int) -TimeUnit.HOURS.toSeconds(18), (int) TimeUnit.HOURS.toSeconds(18))); - assertFixOffset(zone, zone.getTotalSeconds() * 1000); + assertFixedOffset(zone, zone.getTotalSeconds() * 1000); } - private void assertFixOffset(ZoneId zone, long offsetMillis) { + private void assertFixedOffset(ZoneId zone, long offsetMillis) { LocalTimeOffset fixed = LocalTimeOffset.fixedOffset(zone); assertThat(fixed, notNullValue()); LocalTimeOffset.Lookup lookup = LocalTimeOffset.lookup(zone, Long.MIN_VALUE, Long.MAX_VALUE); assertThat(lookup.size(), equalTo(1)); + assertThat(lookup.anyMoveBackToPreviousDay(), equalTo(false)); + long min = randomLong(); long max = randomValueOtherThan(min, ESTestCase::randomLong); if (min > max) { @@ -72,8 +75,10 @@ private void assertFixOffset(ZoneId zone, long offsetMillis) { min = max; max = s; } + LocalTimeOffset fixedInRange = lookup.fixedInRange(min, max); assertThat(fixedInRange, notNullValue()); + assertThat(fixedInRange.anyMoveBackToPreviousDay(), equalTo(false)); assertRoundingAtOffset(randomBoolean() ? fixed : fixedInRange, randomLong(), offsetMillis); } @@ -118,6 +123,7 @@ private void assertTransitions(ZoneId zone, long min, long max, long between, lo assertRoundingAtOffset(lookup.lookup(max), max, minMaxOffset); assertThat(lookup.fixedInRange(min, max), nullValue()); assertThat(lookup.fixedInRange(min, sameOffsetAsMin), sameInstance(lookup.lookup(min))); + assertThat(lookup.anyMoveBackToPreviousDay(), equalTo(false)); // None of the examples we use this method with move back } // Some sanity checks for when you pas a single time. We don't expect to do this much but it shouldn't be totally borked. @@ -241,6 +247,28 @@ public void testGap() { assertThat(gapOffset.localToUtc(localSkippedTime, useValueForGap(gapValue)), equalTo(gapValue)); } + public void testKnownMovesBackToPreviousDay() { + assertKnownMovesBacktoPreviousDay("America/Goose_Bay", "2010-11-07T03:01:00"); + assertKnownMovesBacktoPreviousDay("America/Moncton", "2006-10-29T03:01:00"); + assertKnownMovesBacktoPreviousDay("America/Moncton", "2005-10-29T03:01:00"); + assertKnownMovesBacktoPreviousDay("America/St_Johns", "2010-11-07T02:31:00"); + assertKnownMovesBacktoPreviousDay("Canada/Newfoundland", "2010-11-07T02:31:00"); + assertKnownMovesBacktoPreviousDay("Europe/Paris", "1911-03-1T00:01:00"); + if (JavaVersion.current().compareTo(JavaVersion.parse("11")) > 0) { + // Added in java 12 + assertKnownMovesBacktoPreviousDay("Pacific/Guam", "1969-01-25T13:01:00"); + assertKnownMovesBacktoPreviousDay("Pacific/Saipan", "1969-01-25T13:01:00"); + } + } + + private void assertKnownMovesBacktoPreviousDay(String zone, String time) { + long utc = utcTime(time); + assertTrue( + zone + " just after " + time + " should move back", + LocalTimeOffset.lookup(ZoneId.of(zone), utc, utc + 1).anyMoveBackToPreviousDay() + ); + } + private static long utcTime(String time) { return DateFormatter.forPattern("date_optional_time").parseMillis(time); } diff --git a/server/src/test/java/org/elasticsearch/common/RoundingTests.java b/server/src/test/java/org/elasticsearch/common/RoundingTests.java index cc838cc4af48e..e0d464c875f15 100644 --- a/server/src/test/java/org/elasticsearch/common/RoundingTests.java +++ b/server/src/test/java/org/elasticsearch/common/RoundingTests.java @@ -36,6 +36,8 @@ import java.time.format.DateTimeFormatter; import java.time.temporal.TemporalAccessor; import java.time.zone.ZoneOffsetTransition; +import java.time.zone.ZoneOffsetTransitionRule; +import java.time.zone.ZoneRules; import java.util.ArrayList; import java.util.List; import java.util.concurrent.TimeUnit; @@ -214,6 +216,9 @@ public void testOffsetRounding() { assertThat(rounding.nextRoundingValue(0), equalTo(oneDay - twoHours)); assertThat(rounding.withoutOffset().round(0), equalTo(0L)); assertThat(rounding.withoutOffset().nextRoundingValue(0), equalTo(oneDay)); + + rounding = Rounding.builder(Rounding.DateTimeUnit.DAY_OF_MONTH).timeZone(ZoneId.of("America/New_York")).offset(-twoHours).build(); + assertThat(rounding.round(time("2020-11-01T09:00:00")), equalTo(time("2020-11-01T02:00:00"))); } /** @@ -231,51 +236,55 @@ public void testRandomTimeUnitRounding() { for (int i = 0; i < 1000; ++i) { Rounding.DateTimeUnit unit = randomFrom(Rounding.DateTimeUnit.values()); ZoneId tz = randomZone(); - Rounding rounding = new Rounding.TimeUnitRounding(unit, tz); - long[] bounds = randomDateBounds(); - Rounding.Prepared prepared = rounding.prepare(bounds[0], bounds[1]); - - // Check that rounding is internally consistent and consistent with nextRoundingValue - long date = dateBetween(bounds[0], bounds[1]); - long unitMillis = unit.getField().getBaseUnit().getDuration().toMillis(); - // FIXME this was copy pasted from the other impl and not used. breaks the nasty date actually gets assigned - if (randomBoolean()) { - nastyDate(date, tz, unitMillis); - } - final long roundedDate = prepared.round(date); - final long nextRoundingValue = prepared.nextRoundingValue(roundedDate); - - assertInterval(roundedDate, date, nextRoundingValue, rounding, tz); - - // check correct unit interval width for units smaller than a day, they should be fixed size except for transitions - if (unitMillis <= 86400 * 1000) { - // if the interval defined didn't cross timezone offset transition, it should cover unitMillis width - int offsetRounded = tz.getRules().getOffset(Instant.ofEpochMilli(roundedDate - 1)).getTotalSeconds(); - int offsetNextValue = tz.getRules().getOffset(Instant.ofEpochMilli(nextRoundingValue + 1)).getTotalSeconds(); - if (offsetRounded == offsetNextValue) { - assertThat("unit interval width not as expected for [" + unit + "], [" + tz + "] at " - + Instant.ofEpochMilli(roundedDate), nextRoundingValue - roundedDate, equalTo(unitMillis)); - } + long[] bounds = randomDateBounds(unit); + assertUnitRoundingSameAsJavaUtilTimeImplementation(unit, tz, bounds[0], bounds[1]); + } + } + + private void assertUnitRoundingSameAsJavaUtilTimeImplementation(Rounding.DateTimeUnit unit, ZoneId tz, long start, long end) { + Rounding rounding = new Rounding.TimeUnitRounding(unit, tz); + Rounding.Prepared prepared = rounding.prepare(start, end); + + // Check that rounding is internally consistent and consistent with nextRoundingValue + long date = dateBetween(start, end); + long unitMillis = unit.getField().getBaseUnit().getDuration().toMillis(); + // FIXME this was copy pasted from the other impl and not used. breaks the nasty date actually gets assigned + if (randomBoolean()) { + nastyDate(date, tz, unitMillis); + } + final long roundedDate = prepared.round(date); + final long nextRoundingValue = prepared.nextRoundingValue(roundedDate); + + assertInterval(roundedDate, date, nextRoundingValue, rounding, tz); + + // check correct unit interval width for units smaller than a day, they should be fixed size except for transitions + if (unitMillis <= 86400 * 1000) { + // if the interval defined didn't cross timezone offset transition, it should cover unitMillis width + int offsetRounded = tz.getRules().getOffset(Instant.ofEpochMilli(roundedDate - 1)).getTotalSeconds(); + int offsetNextValue = tz.getRules().getOffset(Instant.ofEpochMilli(nextRoundingValue + 1)).getTotalSeconds(); + if (offsetRounded == offsetNextValue) { + assertThat("unit interval width not as expected for [" + unit + "], [" + tz + "] at " + + Instant.ofEpochMilli(roundedDate), nextRoundingValue - roundedDate, equalTo(unitMillis)); } + } - // Round a whole bunch of dates and make sure they line up with the known good java time implementation - Rounding.Prepared javaTimeRounding = rounding.prepareJavaTime(); - for (int d = 0; d < 1000; d++) { - date = dateBetween(bounds[0], bounds[1]); - long javaRounded = javaTimeRounding.round(date); - long esRounded = prepared.round(date); - if (javaRounded != esRounded) { - fail("Expected [" + rounding + "] to round [" + Instant.ofEpochMilli(date) + "] to [" - + Instant.ofEpochMilli(javaRounded) + "] but instead rounded to [" + Instant.ofEpochMilli(esRounded) + "]"); - } - long javaNextRoundingValue = javaTimeRounding.nextRoundingValue(date); - long esNextRoundingValue = prepared.nextRoundingValue(date); - if (javaNextRoundingValue != esNextRoundingValue) { - fail("Expected [" + rounding + "] to round [" + Instant.ofEpochMilli(date) + "] to [" - + Instant.ofEpochMilli(esRounded) + "] and nextRoundingValue to be [" - + Instant.ofEpochMilli(javaNextRoundingValue) + "] but instead was to [" - + Instant.ofEpochMilli(esNextRoundingValue) + "]"); - } + // Round a whole bunch of dates and make sure they line up with the known good java time implementation + Rounding.Prepared javaTimeRounding = rounding.prepareJavaTime(); + for (int d = 0; d < 1000; d++) { + date = dateBetween(start, end); + long javaRounded = javaTimeRounding.round(date); + long esRounded = prepared.round(date); + if (javaRounded != esRounded) { + fail("Expected [" + rounding + "] to round [" + Instant.ofEpochMilli(date) + "] to [" + + Instant.ofEpochMilli(javaRounded) + "] but instead rounded to [" + Instant.ofEpochMilli(esRounded) + "]"); + } + long javaNextRoundingValue = javaTimeRounding.nextRoundingValue(date); + long esNextRoundingValue = prepared.nextRoundingValue(date); + if (javaNextRoundingValue != esNextRoundingValue) { + fail("Expected [" + rounding + "] to round [" + Instant.ofEpochMilli(date) + "] to [" + + Instant.ofEpochMilli(esRounded) + "] and nextRoundingValue to be [" + + Instant.ofEpochMilli(javaNextRoundingValue) + "] but instead was to [" + + Instant.ofEpochMilli(esNextRoundingValue) + "]"); } } } @@ -715,6 +724,70 @@ public void testDST_America_St_Johns() { } } + /** + * Tests for DST transitions that cause the rounding to jump "backwards" because they round + * from one back to the previous day. Usually these rounding start before + */ + public void testForwardsBackwardsTimeZones() { + for (String zoneId : JAVA_ZONE_IDS) { + ZoneId tz = ZoneId.of(zoneId); + ZoneRules rules = tz.getRules(); + for (ZoneOffsetTransition transition : rules.getTransitions()) { + checkForForwardsBackwardsTransition(tz, transition); + } + int firstYear; + if (rules.getTransitions().isEmpty()) { + // Pick an arbitrary year to start the range + firstYear = 1999; + } else { + ZoneOffsetTransition lastTransition = rules.getTransitions().get(rules.getTransitions().size() - 1); + firstYear = lastTransition.getDateTimeAfter().getYear() + 1; + } + // Pick an arbitrary year to end the range too + int lastYear = 2050; + int year = randomFrom(firstYear, lastYear); + for (ZoneOffsetTransitionRule transitionRule : rules.getTransitionRules()) { + ZoneOffsetTransition transition = transitionRule.createTransition(year); + checkForForwardsBackwardsTransition(tz, transition); + } + } + } + + private void checkForForwardsBackwardsTransition(ZoneId tz, ZoneOffsetTransition transition) { + if (transition.getDateTimeBefore().getYear() < 1950) { + // We don't support transitions far in the past at all + return; + } + if (false == transition.isOverlap()) { + // Only overlaps cause the array rounding to have trouble + return; + } + if (transition.getDateTimeBefore().getDayOfMonth() == transition.getDateTimeAfter().getDayOfMonth()) { + // Only when the rounding changes the day + return; + } + if (transition.getDateTimeBefore().getMinute() == 0) { + // But roundings that change *at* midnight are safe because they don't "jump" to the next day. + return; + } + logger.info( + "{} from {}{} to {}{}", + tz, + transition.getDateTimeBefore(), + transition.getOffsetBefore(), + transition.getDateTimeAfter(), + transition.getOffsetAfter() + ); + long millisSinceEpoch = TimeUnit.SECONDS.toMillis(transition.toEpochSecond()); + long twoHours = TimeUnit.HOURS.toMillis(2); + assertUnitRoundingSameAsJavaUtilTimeImplementation( + Rounding.DateTimeUnit.DAY_OF_MONTH, + tz, + millisSinceEpoch - twoHours, + millisSinceEpoch + twoHours + ); + } + /** * tests for dst transition with overlaps and day roundings. */ @@ -957,6 +1030,11 @@ private static boolean isTimeWithWellDefinedRounding(ZoneId tz, long t) { return t <= time("2010-03-03T00:00:00Z") || t >= time("2010-03-07T00:00:00Z"); } + if (tz.getId().equals("Pacific/Guam") || tz.getId().equals("Pacific/Saipan")) { + // Clocks went back at 00:01 in 1969, causing overlapping days. + return t <= time("1969-01-25T00:00:00Z") + || t >= time("1969-01-26T00:00:00Z"); + } return true; } @@ -965,8 +1043,13 @@ private static long randomDate() { return Math.abs(randomLong() % (2 * (long) 10e11)); // 1970-01-01T00:00:00Z - 2033-05-18T05:33:20.000+02:00 } - private static long[] randomDateBounds() { + private static long[] randomDateBounds(Rounding.DateTimeUnit unit) { long b1 = randomDate(); + if (randomBoolean()) { + // Sometimes use a fairly close date + return new long[] {b1, b1 + unit.extraLocalOffsetLookup() * between(1, 40)}; + } + // Otherwise use a totally random date long b2 = randomValueOtherThan(b1, RoundingTests::randomDate); if (b1 < b2) { return new long[] {b1, b2};