Fix rounding of time values near to overlapping days (#28151)

Sometimes, in some places, the clocks are set back across midnight, leading to
overlapping days. This was not handled as expected, and this change fixes this.

Additionally, in this situation it is not true that rounding a time down to the
nearest day is a monotonic operation, as asserted in these tests. This change 
suppresses those assertions in those rare cases.

Fixes #27966.

Author: DaveCTurner

server/src/main/java/org/elasticsearch/common/rounding/Rounding.java

@@ -110,13 +110,15 @@ static class TimeUnitRounding extends Rounding {
         private DateTimeUnit unit;
         private DateTimeField field;
         private DateTimeZone timeZone;
+        private boolean unitRoundsToMidnight;
 
         TimeUnitRounding() { // for serialization
         }
 
         TimeUnitRounding(DateTimeUnit unit, DateTimeZone timeZone) {
             this.unit = unit;
             this.field = unit.field(timeZone);
+            unitRoundsToMidnight = this.field.getDurationField().getUnitMillis() > 60L * 60L * 1000L;
             this.timeZone = timeZone;
         }
 
@@ -125,44 +127,102 @@ public byte id() {
             return ID;
         }
 
+        /**
+         * @return The latest timestamp T which is strictly before utcMillis
+         * and such that timeZone.getOffset(T) != timeZone.getOffset(utcMillis).
+         * If there is no such T, returns Long.MAX_VALUE.
+         */
+        private long previousTransition(long utcMillis) {
+            final int offsetAtInputTime = timeZone.getOffset(utcMillis);
+            do {
+                // Some timezones have transitions that do not change the offset, so we have to
+                // repeatedly call previousTransition until a nontrivial transition is found.
+
+                long previousTransition = timeZone.previousTransition(utcMillis);
+                if (previousTransition == utcMillis) {
+                    // There are no earlier transitions
+                    return Long.MAX_VALUE;
+                }
+                assert previousTransition < utcMillis; // Progress was made
+                utcMillis = previousTransition;
+            } while (timeZone.getOffset(utcMillis) == offsetAtInputTime);
+
+            return utcMillis;
+        }
+
         @Override
         public long round(long utcMillis) {
-            long rounded = field.roundFloor(utcMillis);
-            if (timeZone.isFixed() == false) {
-                // special cases for non-fixed time zones with dst transitions
-                if (timeZone.getOffset(utcMillis) != timeZone.getOffset(rounded)) {
-                    /*
-                     * the offset change indicates a dst transition. In some
-                     * edge cases this will result in a value that is not a
-                     * rounded value before the transition. We round again to
-                     * make sure we really return a rounded value. This will
-                     * have no effect in cases where we already had a valid
-                     * rounded value
-                     */
-                    rounded = field.roundFloor(rounded);
-                } else {
-                    /*
-                     * check if the current time instant is at a start of a DST
-                     * overlap by comparing the offset of the instant and the
-                     * previous millisecond. We want to detect negative offset
-                     * changes that result in an overlap
-                     */
-                    if (timeZone.getOffset(rounded) < timeZone.getOffset(rounded - 1)) {
-                        /*
-                         * we are rounding a date just after a DST overlap. if
-                         * the overlap is smaller than the time unit we are
-                         * rounding to, we want to add the overlapping part to
-                         * the following rounding interval
-                         */
-                        long previousRounded = field.roundFloor(rounded - 1);
-                        if (rounded - previousRounded < field.getDurationField().getUnitMillis()) {
-                            rounded = previousRounded;
-                        }
-                    }
+
+            // field.roundFloor() works as long as the offset doesn't change.  It is worth getting this case out of the way first, as
+            // the calculations for fixing things near to offset changes are a little expensive and are unnecessary in the common case
+            // of working in UTC.
+            if (timeZone.isFixed()) {
+                return field.roundFloor(utcMillis);
+            }
+
+            // When rounding to hours we consider any local time of the form 'xx:00:00' as rounded, even though this gives duplicate
+            // bucket names for the times when the clocks go back. Shorter units behave similarly. However, longer units round down to
+            // midnight, and on the days where there are two midnights we would rather pick the earlier one, so that buckets are
+            // uniquely identified by the date.
+            if (unitRoundsToMidnight) {
+                final long anyLocalStartOfDay = field.roundFloor(utcMillis);
+                // `anyLocalStartOfDay` is _supposed_ to be the Unix timestamp for the start of the day in question in the current time
+                // zone.  Mostly this just means "midnight", which is fine, and on days with no local midnight it's the first time that
+                // does occur on that day which is also ok. However, on days with >1 local midnight this is _one_ of the midnights, but
+                // may not be the first. Check whether this is happening, and fix it if so.
+
+                final long previousTransition = previousTransition(anyLocalStartOfDay);
+
+                if (previousTransition == Long.MAX_VALUE) {
+                    // No previous transitions, so there can't be another earlier local midnight.
+                    return anyLocalStartOfDay;
+                }
+
+                final long currentOffset = timeZone.getOffset(anyLocalStartOfDay);
+                final long previousOffset = timeZone.getOffset(previousTransition);
+                assert currentOffset != previousOffset;
+
+                // NB we only assume interference from one previous transition. It's theoretically possible to have two transitions in
+                // quick succession, both of which have a midnight in them, but this doesn't appear to happen in the TZDB so (a) it's
+                // pointless to implement and (b) it won't be tested. I recognise that this comment is tempting fate and will likely
+                // cause this very situation to occur in the near future, and eagerly look forward to fixing this using a loop over
+                // previous transitions when it happens.
+
+                final long alsoLocalStartOfDay = anyLocalStartOfDay + currentOffset - previousOffset;
+                // `alsoLocalStartOfDay` is the Unix timestamp for the start of the day in question if the previous offset were in
+                // effect.
+
+                if (alsoLocalStartOfDay <= previousTransition) {
+                    // Therefore the previous offset _is_ in effect at `alsoLocalStartOfDay`, and it's earlier than anyLocalStartOfDay,
+                    // so this is the answer to use.
+                    return alsoLocalStartOfDay;
+                }
+                else {
+                    // The previous offset is not in effect at `alsoLocalStartOfDay`, so the current offset must be.
+                    return anyLocalStartOfDay;
                 }
+
+            } else {
+                do {
+                    long rounded = field.roundFloor(utcMillis);
+
+                    // field.roundFloor() mostly works as long as the offset hasn't changed in [rounded, utcMillis], so look at where
+                    // the offset most recently changed.
+
+                    final long previousTransition = previousTransition(utcMillis);
+
+                    if (previousTransition == Long.MAX_VALUE || previousTransition < rounded) {
+                        // The offset did not change in [rounded, utcMillis], so roundFloor() worked as expected.
+                        return rounded;
+                    }
+
+                    // The offset _did_ change in [rounded, utcMillis]. Put differently, this means that none of the times in
+                    // [previousTransition+1, utcMillis] were rounded, so the rounded time must be <= previousTransition.  This means
+                    // it's sufficient to try and round previousTransition down.
+                    assert previousTransition < utcMillis;
+                    utcMillis = previousTransition;
+                } while (true);
             }
-            assert rounded == field.roundFloor(rounded);
-            return rounded;
         }
 
         @Override
@@ -182,6 +242,7 @@ public void readFrom(StreamInput in) throws IOException {
             unit = DateTimeUnit.resolve(in.readByte());
             timeZone = DateTimeZone.forID(in.readString());
             field = unit.field(timeZone);
+            unitRoundsToMidnight = field.getDurationField().getUnitMillis() > 60L * 60L * 1000L;
         }
 
         @Override

server/src/test/java/org/elasticsearch/common/rounding/TimeZoneRoundingTests.java

@@ -514,6 +514,101 @@ public void testEdgeCasesTransition() {
         }
     }
 
+    public void testDST_Europe_Rome() {
+        // time zone "Europe/Rome", rounding to days. Rome had two midnights on the day the clocks went back in 1978, and
+        // timeZone.convertLocalToUTC() gives the later of the two because Rome is east of UTC, whereas we want the earlier.
+
+        DateTimeUnit timeUnit = DateTimeUnit.DAY_OF_MONTH;
+        DateTimeZone tz = DateTimeZone.forID("Europe/Rome");
+        Rounding rounding = new TimeUnitRounding(timeUnit, tz);
+
+        {
+            long timeBeforeFirstMidnight = time("1978-09-30T23:59:00+02:00");
+            long floor = rounding.round(timeBeforeFirstMidnight);
+            assertThat(floor, isDate(time("1978-09-30T00:00:00+02:00"), tz));
+        }
+
+        {
+            long timeBetweenMidnights = time("1978-10-01T00:30:00+02:00");
+            long floor = rounding.round(timeBetweenMidnights);
+            assertThat(floor, isDate(time("1978-10-01T00:00:00+02:00"), tz));
+        }
+
+        {
+            long timeAfterSecondMidnight = time("1978-10-01T00:30:00+01:00");
+            long floor = rounding.round(timeAfterSecondMidnight);
+            assertThat(floor, isDate(time("1978-10-01T00:00:00+02:00"), tz));
+
+            long prevFloor = rounding.round(floor - 1);
+            assertThat(prevFloor, lessThan(floor));
+            assertThat(prevFloor, isDate(time("1978-09-30T00:00:00+02:00"), tz));
+        }
+    }
+
+    /**
+     * Test for a time zone whose days overlap because the clocks are set back across midnight at the end of DST.
+     */
+    public void testDST_America_St_Johns() {
+        // time zone "America/St_Johns", rounding to days.
+        DateTimeUnit timeUnit = DateTimeUnit.DAY_OF_MONTH;
+        DateTimeZone tz = DateTimeZone.forID("America/St_Johns");
+        Rounding rounding = new TimeUnitRounding(timeUnit, tz);
+
+        // 29 October 2006 - Daylight Saving Time ended, changing the UTC offset from -02:30 to -03:30.
+        // This happened at 02:31 UTC, 00:01 local time, so the clocks were set back 1 hour to 23:01 on the 28th.
+        // This means that 2006-10-29 has _two_ midnights, one in the -02:30 offset and one in the -03:30 offset.
+        // Only the first of these is considered "rounded". Moreover, the extra time between 23:01 and 23:59
+        // should be considered as part of the 28th even though it comes after midnight on the 29th.
+
+        {
+            // Times before the first midnight should be rounded up to the first midnight.
+            long timeBeforeFirstMidnight = time("2006-10-28T23:30:00.000-02:30");
+            long floor = rounding.round(timeBeforeFirstMidnight);
+            assertThat(floor, isDate(time("2006-10-28T00:00:00.000-02:30"), tz));
+            long ceiling = rounding.nextRoundingValue(timeBeforeFirstMidnight);
+            assertThat(ceiling, isDate(time("2006-10-29T00:00:00.000-02:30"), tz));
+            assertInterval(floor, timeBeforeFirstMidnight, ceiling, rounding, tz);
+        }
+
+        {
+            // Times between the two midnights which are on the later day should be rounded down to the later day's midnight.
+            long timeBetweenMidnights = time("2006-10-29T00:00:30.000-02:30");
+            // (this is halfway through the last minute before the clocks changed, in which local time was ambiguous)
+
+            long floor = rounding.round(timeBetweenMidnights);
+            assertThat(floor, isDate(time("2006-10-29T00:00:00.000-02:30"), tz));
+
+            long ceiling = rounding.nextRoundingValue(timeBetweenMidnights);
+            assertThat(ceiling, isDate(time("2006-10-30T00:00:00.000-03:30"), tz));
+
+            assertInterval(floor, timeBetweenMidnights, ceiling, rounding, tz);
+        }
+
+        {
+            // Times between the two midnights which are on the earlier day should be rounded down to the earlier day's midnight.
+            long timeBetweenMidnights = time("2006-10-28T23:30:00.000-03:30");
+            // (this is halfway through the hour after the clocks changed, in which local time was ambiguous)
+
+            long floor = rounding.round(timeBetweenMidnights);
+            assertThat(floor, isDate(time("2006-10-28T00:00:00.000-02:30"), tz));
+
+            long ceiling = rounding.nextRoundingValue(timeBetweenMidnights);
+            assertThat(ceiling, isDate(time("2006-10-29T00:00:00.000-02:30"), tz));
+
+            assertInterval(floor, timeBetweenMidnights, ceiling, rounding, tz);
+        }
+
+        {
+            // Times after the second midnight should be rounded down to the first midnight.
+            long timeAfterSecondMidnight = time("2006-10-29T06:00:00.000-03:30");
+            long floor = rounding.round(timeAfterSecondMidnight);
+            assertThat(floor, isDate(time("2006-10-29T00:00:00.000-02:30"), tz));
+            long ceiling = rounding.nextRoundingValue(timeAfterSecondMidnight);
+            assertThat(ceiling, isDate(time("2006-10-30T00:00:00.000-03:30"), tz));
+            assertInterval(floor, timeAfterSecondMidnight, ceiling, rounding, tz);
+        }
+    }
+
     /**
      * tests for dst transition with overlaps and day roundings.
      */
@@ -527,12 +622,17 @@ public void testDST_END_Edgecases() {
 
         // Sunday, 29 October 2000, 01:00:00 clocks were turned backward 1 hour
         // to Sunday, 29 October 2000, 00:00:00 local standard time instead
+        // which means there were two midnights that day.
 
         long midnightBeforeTransition = time("2000-10-29T00:00:00", tz);
+        long midnightOfTransition = time("2000-10-29T00:00:00-01:00");
+        assertEquals(60L * 60L * 1000L, midnightOfTransition - midnightBeforeTransition);
         long nextMidnight = time("2000-10-30T00:00:00", tz);
 
         assertInterval(midnightBeforeTransition, nextMidnight, rounding, 25 * 60, tz);
 
+        assertThat(rounding.round(time("2000-10-29T06:00:00-01:00")), isDate(time("2000-10-29T00:00:00Z"), tz));
+
         // Second case, dst happens at 0am local time, switching back one hour to 23pm local time.
         // We want the overlapping hour to count for the previous day here
 
@@ -584,26 +684,52 @@ private static void assertInterval(long rounded, long nextRoundingValue, Roundin
      * @param nextRoundingValue the expected upper end of the rounding interval
      * @param rounding the rounding instance
      */
-    private static void assertInterval(long rounded, long unrounded, long nextRoundingValue, Rounding rounding,
-            DateTimeZone tz) {
-        assert rounded <= unrounded && unrounded <= nextRoundingValue;
+    private static void assertInterval(long rounded, long unrounded, long nextRoundingValue, Rounding rounding, DateTimeZone tz) {
         assertThat("rounding should be idempotent ", rounding.round(rounded), isDate(rounded, tz));
         assertThat("rounded value smaller or equal than unrounded" + rounding, rounded, lessThanOrEqualTo(unrounded));
         assertThat("values less than rounded should round further down" + rounding, rounding.round(rounded - 1), lessThan(rounded));
-        assertThat("nextRounding value should be greater than date" + rounding, nextRoundingValue, greaterThan(unrounded));
         assertThat("nextRounding value should be a rounded date", rounding.round(nextRoundingValue), isDate(nextRoundingValue, tz));
         assertThat("values above nextRounding should round down there", rounding.round(nextRoundingValue + 1),
                 isDate(nextRoundingValue, tz));
 
-        long dateBetween = dateBetween(rounded, nextRoundingValue);
-        assertThat("dateBetween should round down to roundedDate", rounding.round(dateBetween), isDate(rounded, tz));
-        assertThat("dateBetween should round up to nextRoundingValue", rounding.nextRoundingValue(dateBetween),
-                isDate(nextRoundingValue, tz));
+        if (isTimeWithWellDefinedRounding(tz, unrounded)) {
+            assertThat("nextRounding value should be greater than date" + rounding, nextRoundingValue, greaterThan(unrounded));
+
+            long dateBetween = dateBetween(rounded, nextRoundingValue);
+            assertThat("dateBetween [" + new DateTime(dateBetween, tz) + "] should round down to roundedDate",
+                rounding.round(dateBetween), isDate(rounded, tz));
+            assertThat("dateBetween [" + new DateTime(dateBetween, tz) + "] should round up to nextRoundingValue",
+                rounding.nextRoundingValue(dateBetween), isDate(nextRoundingValue, tz));
+        }
+    }
+
+    private static boolean isTimeWithWellDefinedRounding(DateTimeZone tz, long t) {
+        if (tz.getID().equals("America/St_Johns")
+            || tz.getID().equals("America/Goose_Bay")
+            || tz.getID().equals("America/Moncton")
+            || tz.getID().equals("Canada/Newfoundland")) {
+
+            // Clocks went back at 00:01 between 1987 and 2010, causing overlapping days.
+            // These timezones are otherwise uninteresting, so just skip this period.
+
+            return t <= time("1987-10-01T00:00:00Z")
+                || t >= time("2010-12-01T00:00:00Z");
+        }
+
+        if (tz.getID().equals("Antarctica/Casey")) {
+
+            // Clocks went back 3 hours at 02:00 on 2010-03-05, causing overlapping days.
+
+            return t <= time("2010-03-03T00:00:00Z")
+                || t >= time("2010-03-07T00:00:00Z");
+        }
+
+        return true;
     }
 
     private static long dateBetween(long lower, long upper) {
-        long dateBetween = lower + Math.abs((randomLong() % (upper - lower)));
-        assert lower <= dateBetween && dateBetween <= upper;
+        long dateBetween = randomLongBetween(lower, upper - 1);
+        assert lower <= dateBetween && dateBetween < upper;
         return dateBetween;
     }
 
@@ -629,7 +755,7 @@ public boolean matchesSafely(final Long item) {
 
             @Override
             public void describeTo(Description description) {
-                description.appendText("Expected: " + new DateTime(expected, tz) + " [" + expected + "] ");
+                description.appendText(new DateTime(expected, tz) + " [" + expected + "] ");
             }
 
             @Override