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.

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

@@ -110,6 +110,7 @@ public abstract class Rounding implements Streamable {
         private DateTimeUnit unit;
         private DateTimeField field;
         private DateTimeZone timeZone;
+        private boolean unitRoundsToMidnight;
 
         TimeUnitRounding() { // for serialization
         }
@@ -117,6 +118,7 @@ public abstract class Rounding implements Streamable {
         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 abstract class Rounding implements Streamable {
             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) {
+            // field.roundFloor() works as long as the offset doesn't change.  It is worth getting this case out of the way first, as
-                // special cases for non-fixed time zones with dst transitions
+            // the calculations for fixing things near to offset changes are a little expensive and are unnecessary in the common case
-                if (timeZone.getOffset(utcMillis) != timeZone.getOffset(rounded)) {
+            // of working in UTC.
-                    /*
+            if (timeZone.isFixed()) {
-                     * the offset change indicates a dst transition. In some
+                return field.roundFloor(utcMillis);
-                     * 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
+            // When rounding to hours we consider any local time of the form 'xx:00:00' as rounded, even though this gives duplicate
-                     * have no effect in cases where we already had a valid
+            // bucket names for the times when the clocks go back. Shorter units behave similarly. However, longer units round down to
-                     * rounded value
+            // 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.
-                    rounded = field.roundFloor(rounded);
+            if (unitRoundsToMidnight) {
-                } else {
+                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
-                     * check if the current time instant is at a start of a DST
+                // zone.  Mostly this just means "midnight", which is fine, and on days with no local midnight it's the first time that
-                     * overlap by comparing the offset of the instant and the
+                // does occur on that day which is also ok. However, on days with >1 local midnight this is _one_ of the midnights, but
-                     * previous millisecond. We want to detect negative offset
+                // may not be the first. Check whether this is happening, and fix it if so.
-                     * changes that result in an overlap
+
-                     */
+                final long previousTransition = previousTransition(anyLocalStartOfDay);
-                    if (timeZone.getOffset(rounded) < timeZone.getOffset(rounded - 1)) {
+
-                        /*
+                if (previousTransition == Long.MAX_VALUE) {
-                         * we are rounding a date just after a DST overlap. if
+                    // No previous transitions, so there can't be another earlier local midnight.
-                         * the overlap is smaller than the time unit we are
+                    return anyLocalStartOfDay;
-                         * rounding to, we want to add the overlapping part to
+                }
-                         * the following rounding interval
+
-                         */
+                final long currentOffset = timeZone.getOffset(anyLocalStartOfDay);
-                        long previousRounded = field.roundFloor(rounded - 1);
+                final long previousOffset = timeZone.getOffset(previousTransition);
-                        if (rounded - previousRounded < field.getDurationField().getUnitMillis()) {
+                assert currentOffset != previousOffset;
-                            rounded = previousRounded;
+
-                        }
+                // 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 abstract class Rounding implements Streamable {
             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 class TimeZoneRoundingTests extends ESTestCase {
         }
     }
 
+    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 class TimeZoneRoundingTests extends ESTestCase {
 
         // 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 @@ public class TimeZoneRoundingTests extends ESTestCase {
      * @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,
+    private static void assertInterval(long rounded, long unrounded, long nextRoundingValue, Rounding rounding, DateTimeZone tz) {
-            DateTimeZone tz) {
-        assert rounded <= unrounded && unrounded <= nextRoundingValue;
         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);
+        if (isTimeWithWellDefinedRounding(tz, unrounded)) {
-        assertThat("dateBetween should round down to roundedDate", rounding.round(dateBetween), isDate(rounded, tz));
+            assertThat("nextRounding value should be greater than date" + rounding, nextRoundingValue, greaterThan(unrounded));
-        assertThat("dateBetween should round up to nextRoundingValue", rounding.nextRoundingValue(dateBetween),
+
-                isDate(nextRoundingValue, tz));
+            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)));
+        long dateBetween = randomLongBetween(lower, upper - 1);
-        assert lower <= dateBetween && dateBetween <= upper;
+        assert lower <= dateBetween && dateBetween < upper;
         return dateBetween;
     }
 
@@ -629,7 +755,7 @@ public class TimeZoneRoundingTests extends ESTestCase {
 
             @Override
             public void describeTo(Description description) {
-                description.appendText("Expected: " + new DateTime(expected, tz) + " [" + expected + "] ");
+                description.appendText(new DateTime(expected, tz) + " [" + expected + "] ");
             }
 
             @Override