Browse Source

Fix bug in faster interval rounding (#56433)

This fixes a bug in the interval rounding and two test bugs that showed
up when I ran 1000s of iterations of the tests. The bug was that we
could end up with duplicate transitions if we only needed the last
transition from the list of fully defined transitions and the
transitions overlapped with the rules. This happens. Its ok. We had
defence against that if we needed more than one transition but forgot it
in this case. Now we have it and assertions to make sure we catch any
similar mistakes.

One test bug had to do with the randomized test calling
`round(round(min))` because sometimes the first `round` call will return
a time less than min. Specifically if `min` is near daylight savings
time. Anyway, this change fixes it by building an extra-wide prepared
rounding just in case.

The other test bug came up when adding a test for the real bug, namely
that `assertRoundingAtOffset` made an assertion that wasn't true if the
time to round ended up being in an overlap. This just drops that
assertion. We have similar assertions in cases where we *know* what kind
of offsets we're working with in other tests.

Closes #56400
Nik Everett 5 years ago
parent
commit
cb47853a41

+ 9 - 0
server/src/main/java/org/elasticsearch/common/LocalTimeOffset.java

@@ -494,6 +494,10 @@ public abstract class LocalTimeOffset {
             long utcStart = transition.toEpochSecond() * 1000;
             long offsetBeforeMillis = transition.getOffsetBefore().getTotalSeconds() * 1000;
             long offsetAfterMillis = transition.getOffsetAfter().getTotalSeconds() * 1000;
+            assert (false == previous instanceof Transition) || ((Transition) previous).startUtcMillis < utcStart :
+                    "transition list out of order at [" + previous + "] and [" + transition + "]";
+            assert previous.millis != offsetAfterMillis :
+                    "transition list is has a duplicate at [" + previous + "] and [" + transition + "]";
             if (transition.isGap()) {
                 long firstMissingLocalTime = utcStart + offsetBeforeMillis;
                 long firstLocalTimeAfterGap = utcStart + offsetAfterMillis;
@@ -560,6 +564,11 @@ public abstract class LocalTimeOffset {
         if (false == itr.hasNext()) {
             if (minSecond < t.toEpochSecond() && t.toEpochSecond() < maxSecond) {
                 transitions.add(t);
+                /*
+                 * Sometimes the rules duplicate the transitions. And
+                 * duplicates confuse us. So we have to skip past them.
+                 */
+                minSecond = t.toEpochSecond() + 1;
             }
             transitions = buildTransitionsFromRules(transitions, zone, rules, minSecond, maxSecond);
             if (transitions != null && transitions.isEmpty()) {

+ 22 - 2
server/src/test/java/org/elasticsearch/common/LocalTimeOffsetTests.java

@@ -28,6 +28,7 @@ import java.time.Instant;
 import java.time.ZoneId;
 import java.time.ZoneOffset;
 import java.time.zone.ZoneOffsetTransition;
+import java.time.zone.ZoneRules;
 import java.util.List;
 import java.util.concurrent.TimeUnit;
 
@@ -76,11 +77,10 @@ public class LocalTimeOffsetTests extends ESTestCase {
 
         assertRoundingAtOffset(randomBoolean() ? fixed : fixedInRange, randomLong(), offsetMillis);
     }
-    
+
     private void assertRoundingAtOffset(LocalTimeOffset offset, long time, long offsetMillis) {
         assertThat(offset.utcToLocalTime(time), equalTo(time + offsetMillis));
         assertThat(offset.localToUtcInThisOffset(time + offsetMillis), equalTo(time));
-        assertThat(offset.localToUtc(time + offsetMillis, unusedStrategy()), equalTo(time));
     }
 
     public void testJustTransitions() {
@@ -155,6 +155,26 @@ public class LocalTimeOffsetTests extends ESTestCase {
         assertRoundingAtOffset(lookup.lookup(time), time, TimeUnit.MINUTES.toMillis(345));
     }
 
+    /**
+     * America/Tijuana's
+     * {@link ZoneRules#getTransitions() fully defined transitions} overlap
+     * with its {@link ZoneRules#getTransitionRules() future rules} and if
+     * we're not careful we can end up with duplicate transitions because we
+     * have to collect them independently. That will trip assertions, failing
+     * this test real fast. If they don't trip the assertions then trying to
+     * use the transitions will produce incorrect results, failing the
+     * size assertion.
+     */
+    public void testLastTransitionOverlapsRules() {
+        ZoneId zone = ZoneId.of("America/Tijuana");
+        long min = utcTime("2011-11-06T08:31:57.091Z");
+        long max = utcTime("2011-11-06T09:02:57.091Z");
+        LocalTimeOffset.Lookup lookup = LocalTimeOffset.lookup(zone, min, max);
+        assertThat(lookup.size(), equalTo(2));
+        assertRoundingAtOffset(lookup.lookup(min), min, -TimeUnit.HOURS.toMillis(7));
+        assertRoundingAtOffset(lookup.lookup(max), max, -TimeUnit.HOURS.toMillis(8));
+    }
+
     public void testOverlap() {
         /*
          * Europe/Rome turn their clocks back an hour 1978 which is totally

+ 10 - 2
server/src/test/java/org/elasticsearch/common/RoundingTests.java

@@ -392,15 +392,23 @@ public class RoundingTests extends ESTestCase {
             long interval = unit.toMillis(unitCount);
             ZoneId tz = randomZone();
             Rounding rounding = new Rounding.TimeIntervalRounding(interval, tz);
-            long mainDate = Math.abs(randomLong() % (2 * (long) 10e11)); // 1970-01-01T00:00:00Z - 2033-05-18T05:33:20.000+02:00
+            long mainDate = randomDate();
             if (randomBoolean()) {
                 mainDate = nastyDate(mainDate, tz, interval);
             }
             long min = mainDate - 2 * interval;
             long max = mainDate + 2 * interval;
 
+            /*
+             * Prepare a rounding with one extra interval of range because
+             * in the tests far below we call round(round(min)). The first
+             * round might spit out a time below the min if min is near a
+             * daylight savings time transition. So we request an extra big
+             * range just in case.
+             */
+            Rounding.Prepared prepared = rounding.prepare(min - interval, max);
+
             // Round a whole bunch of dates and make sure they line up with the known good java time implementation
-            Rounding.Prepared prepared = rounding.prepare(min, max);
             Rounding.Prepared javaTimeRounding = rounding.prepareJavaTime();
             for (int d = 0; d < 1000; d++) {
                 long date = dateBetween(min, max);