Fix invalid rounding value for TimeIntervalRounding close to DST transition

There are edge cases where rounding a date to a certain interval using a time
zone with DST shifts can currently cause the rounded date to be bigger than the
original date. This happens when rounding a date closely after a DST start and
the rounded date falls into the DST gap.

Here is an example for CET time zone, where local time is set forward by one
hour at 2016-03-27T02:00:00+01:00 to 2016-03-27T03:00:00.000+02:00:

The date 2016-03-27T03:01:00.000+02:00 (1459040460000) which is just after the
DST change is first converted to local time (1459047660000). If we then apply
interval rounding for a 14m interval in local time, this  takes us to
1459047240000, which unfortunately falls into the DST gap.  When converting
this back to UTC, joda provides options to throw exceptions on illegal dates
like this, or correct this by adjusting the date to the new time zone offset.
We currently do the later, but this leads to converting this illegal date back
to 2016-03-27T03:54:00.000+02:00 (1459043640000), giving us a date that is
larger than the original date we wanted to round.

This change fixes this by using the "strict" option of 'convertLocalToUTC()'
to detect rounded dates that fall into the DST gap. If this happens, we can use
the time of the DST change instead as the interval start.

Even before this change, intervals around DST shifts like this can be shorter
than the desired interval.  This, for example, happens when the requested
interval width doesn't completely fit into the remaining time span when the DST
shift happens. For example, using a 14m interval in UTC+1 (CET before DST
starts) leads to the following valid rounding values around the time where DST
happens:

2016-03-27T01:30:00+01:00
2016-03-27T01:44:00+01:00
2016-03-27T01:58:00+01:00
2016-03-27T02:12:00+01:00
2016-03-27T02:26:00+01:00
...

while the rounding values in UTC+2 (CET after DST start) are placed like this
around the same time:

2016-03-27T02:40:00+02:00
2016-03-27T02:54:00+02:00
2016-03-27T03:08:00+02:00
2016-03-27T03:22:00+02:00
...

From this we can see then when we switch from UTC+1 to UTC+2 at 02:00 the last
rounding value in UTC+1 is at 01:58 and the first valid one in UTC+2 is at
03:08, so even if we decide to put all the dates in between into one rounding
interval, it will only cover 10 minutes. With this change we choose to use the
moment of DST shift as an aditional interval separator, leaving us with a 2min
interval from [01:58,02:00) before the shift and an 8min interval from
[03:00,03:08) after the shift.

This change also adds tests for the above example and adds randomization to the
existing TimeIntervalRounding tests.

core/src/main/java/org/elasticsearch/common/rounding/TimeZoneRounding.java

@@ -25,6 +25,7 @@ import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.unit.TimeValue;
 import org.joda.time.DateTimeField;
 import org.joda.time.DateTimeZone;
+import org.joda.time.IllegalInstantException;
 
 import java.io.IOException;
 import java.util.Objects;
@@ -218,7 +219,19 @@ public abstract class TimeZoneRounding extends Rounding {
         public long roundKey(long utcMillis) {
             long timeLocal = timeZone.convertUTCToLocal(utcMillis);
             long rounded = Rounding.Interval.roundValue(Rounding.Interval.roundKey(timeLocal, interval), interval);
-            return timeZone.convertLocalToUTC(rounded, false, utcMillis);
+            try {
+                return timeZone.convertLocalToUTC(rounded, true, utcMillis);
+            } catch (IllegalInstantException e) {
+                /*
+                 * The rounded local time is illegal and landed in a DST gap. In
+                 * this case, we choose 1ms tick after the transition date. We
+                 * don't want the transition date itself because those dates,
+                 * when rounded themselves, fall into the previous interval.
+                 * This would violate the invariant that the rounding operation
+                 * should be idempotent.
+                 */
+                return timeZone.previousTransition(utcMillis) + 1;
+            }
         }
 
         @Override

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

@@ -309,24 +309,54 @@ public class TimeZoneRoundingTests extends ESTestCase {
         assertThat(time("1986-01-01T00:40:00+05:45") - time("1986-01-01T00:20:00+05:45"), equalTo(TimeUnit.MINUTES.toMillis(20)));
     }
 
+    /**
+     * Special test for intervals that don't fit evenly into rounding interval.
+     * In this case, when interval crosses DST transition point, rounding in local
+     * time can land in a DST gap which results in wrong UTC rounding values.
+     */
+    public void testIntervalRounding_NotDivisibleInteval() {
+        DateTimeZone tz = DateTimeZone.forID("CET");
+        long interval = TimeUnit.MINUTES.toMillis(14);
+        TimeZoneRounding rounding = new TimeZoneRounding.TimeIntervalRounding(interval, tz);
+
+        assertThat(rounding.round(time("2016-03-27T01:41:00+01:00")), equalTo(time("2016-03-27T01:30:00+01:00")));
+        assertThat(rounding.round(time("2016-03-27T01:51:00+01:00")), equalTo(time("2016-03-27T01:44:00+01:00")));
+        assertThat(rounding.round(time("2016-03-27T01:59:00+01:00")), equalTo(time("2016-03-27T01:58:00+01:00")));
+        assertThat(rounding.round(time("2016-03-27T03:05:00+02:00")), equalTo(time("2016-03-27T03:00:00+02:00")));
+        assertThat(rounding.round(time("2016-03-27T03:12:00+02:00")), equalTo(time("2016-03-27T03:08:00+02:00")));
+        assertThat(rounding.round(time("2016-03-27T03:25:00+02:00")), equalTo(time("2016-03-27T03:22:00+02:00")));
+        assertThat(rounding.round(time("2016-03-27T03:39:00+02:00")), equalTo(time("2016-03-27T03:36:00+02:00")));
+    }
+
     /**
      * randomized test on {@link TimeIntervalRounding} with random interval and time zone offsets
      */
     public void testIntervalRoundingRandom() {
-        for (int i = 0; i < 1000; ++i) {
+        for (int i = 0; i < 1000; i++) {
-            // max random interval is a year, can be negative
+            TimeUnit unit = randomFrom(new TimeUnit[] {TimeUnit.MINUTES, TimeUnit.HOURS, TimeUnit.DAYS});
-            long interval = Math.abs(randomLong() % (TimeUnit.DAYS.toMillis(365)));
+            long interval = unit.toMillis(randomIntBetween(1, 365));
-            TimeZoneRounding rounding;
+            DateTimeZone tz = randomDateTimeZone();
-            int timezoneOffset = randomIntBetween(-23, 23);
+            TimeZoneRounding rounding = new TimeZoneRounding.TimeIntervalRounding(interval, tz);
-            rounding = new TimeZoneRounding.TimeIntervalRounding(interval, DateTimeZone.forOffsetHours(timezoneOffset));
+            long date = Math.abs(randomLong() % (2 * (long) 10e11)); // 1970-01-01T00:00:00Z - 2033-05-18T05:33:20.000+02:00
-            long date = Math.abs(randomLong() % ((long) 10e11));
+            try {
                 final long roundedDate = rounding.round(date);
                 final long nextRoundingValue = rounding.nextRoundingValue(roundedDate);
                 assertThat("Rounding should be idempotent", roundedDate, equalTo(rounding.round(roundedDate)));
-            assertThat("Rounded value smaller or equal than unrounded, regardless of timezone", roundedDate, lessThanOrEqualTo(date));
+                assertThat("Rounded value smaller or equal than unrounded", roundedDate, lessThanOrEqualTo(date));
+                assertThat("Values smaller than rounded value should round further down", rounding.round(roundedDate - 1),
+                        lessThan(roundedDate));
+
+                if (tz.isFixed()) {
                     assertThat("NextRounding value should be greater than date", nextRoundingValue, greaterThan(roundedDate));
-            assertThat("NextRounding value should be interval from rounded value", nextRoundingValue - roundedDate, equalTo(interval));
+                    assertThat("NextRounding value should be interval from rounded value", nextRoundingValue - roundedDate,
-            assertThat("NextRounding value should be a rounded date", nextRoundingValue, equalTo(rounding.round(nextRoundingValue)));
+                            equalTo(interval));
+                    assertThat("NextRounding value should be a rounded date", nextRoundingValue,
+                            equalTo(rounding.round(nextRoundingValue)));
+                }
+            } catch (AssertionError e) {
+                logger.error("Rounding error at {}, timezone {}, interval: {},", new DateTime(date, tz), tz, interval);
+                throw e;
+            }
         }
     }