Fix TimeZoneRounding#nextRoundingValue for hour, minute and second units

Currently rounding intervals obtained by nextRoundingValue() for hour, minute and
second units can include an extra hour when happening at DST transitions that add
an extra hour (eg CEST -> CET). This changes the rounding logic for time units
smaller or equal to an hour to fix this.

Closes #18326

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

@@ -53,6 +53,15 @@ public enum DateTimeUnit {
         return field;
     }
 
+    /**
+     * @param unit the {@link DateTimeUnit} to check
+     * @return true if the unit is a day or longer
+     */
+    public static boolean isDayOrLonger(DateTimeUnit unit) {
+        return (unit == DateTimeUnit.HOUR_OF_DAY || unit == DateTimeUnit.MINUTES_OF_HOUR
+                || unit == DateTimeUnit.SECOND_OF_MINUTE) == false;
+    }
+
     public static DateTimeUnit resolve(byte id) {
         switch (id) {
             case 1: return WEEK_OF_WEEKYEAR;

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

@@ -46,8 +46,8 @@ public abstract class TimeZoneRounding extends Rounding {
 
     public static class Builder {
 
-        private DateTimeUnit unit;
-        private long interval = -1;
+        private final DateTimeUnit unit;
+        private final long interval;
 
         private DateTimeZone timeZone = DateTimeZone.UTC;
 
@@ -142,10 +142,15 @@ public abstract class TimeZoneRounding extends Rounding {
 
         @Override
         public long nextRoundingValue(long time) {
-            long timeLocal = time;
-            timeLocal = timeZone.convertUTCToLocal(time);
-            long nextInLocalTime = durationField.add(timeLocal, 1);
-            return timeZone.convertLocalToUTC(nextInLocalTime, false);
+            if (DateTimeUnit.isDayOrLonger(unit)) {
+                time = timeZone.convertUTCToLocal(time);
+            }
+            long next = durationField.add(time, 1);
+            if (DateTimeUnit.isDayOrLonger(unit)) {
+                return timeZone.convertLocalToUTC(next, false);
+            } else {
+                return next;
+            }
         }
 
         @Override
@@ -161,12 +166,12 @@ public abstract class TimeZoneRounding extends Rounding {
             out.writeByte(unit.id());
             out.writeString(timeZone.getID());
         }
-        
+
         @Override
         public int hashCode() {
             return Objects.hash(unit, timeZone);
         }
-        
+
         @Override
         public boolean equals(Object obj) {
             if (obj == null) {
@@ -236,12 +241,12 @@ public abstract class TimeZoneRounding extends Rounding {
             out.writeVLong(interval);
             out.writeString(timeZone.getID());
         }
-        
+
         @Override
         public int hashCode() {
             return Objects.hash(interval, timeZone);
         }
-        
+
         @Override
         public boolean equals(Object obj) {
             if (obj == null) {

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

@@ -0,0 +1,75 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.elasticsearch.common.rounding;
+
+import org.elasticsearch.test.ESTestCase;
+
+import static org.elasticsearch.common.rounding.DateTimeUnit.WEEK_OF_WEEKYEAR;
+import static org.elasticsearch.common.rounding.DateTimeUnit.YEAR_OF_CENTURY;
+import static org.elasticsearch.common.rounding.DateTimeUnit.QUARTER;
+import static org.elasticsearch.common.rounding.DateTimeUnit.MONTH_OF_YEAR;
+import static org.elasticsearch.common.rounding.DateTimeUnit.DAY_OF_MONTH;
+import static org.elasticsearch.common.rounding.DateTimeUnit.HOUR_OF_DAY;
+import static org.elasticsearch.common.rounding.DateTimeUnit.MINUTES_OF_HOUR;
+import static org.elasticsearch.common.rounding.DateTimeUnit.SECOND_OF_MINUTE;
+
+public class DateTimeUnitTests extends ESTestCase {
+
+    /**
+     * test that we don't accidentally change enum ids
+     */
+    public void testEnumIds() {
+        assertEquals(1, WEEK_OF_WEEKYEAR.id());
+        assertEquals(WEEK_OF_WEEKYEAR, DateTimeUnit.resolve((byte) 1));
+
+        assertEquals(2, YEAR_OF_CENTURY.id());
+        assertEquals(YEAR_OF_CENTURY, DateTimeUnit.resolve((byte) 2));
+
+        assertEquals(3, QUARTER.id());
+        assertEquals(QUARTER, DateTimeUnit.resolve((byte) 3));
+
+        assertEquals(4, MONTH_OF_YEAR.id());
+        assertEquals(MONTH_OF_YEAR, DateTimeUnit.resolve((byte) 4));
+
+        assertEquals(5, DAY_OF_MONTH.id());
+        assertEquals(DAY_OF_MONTH, DateTimeUnit.resolve((byte) 5));
+
+        assertEquals(6, HOUR_OF_DAY.id());
+        assertEquals(HOUR_OF_DAY, DateTimeUnit.resolve((byte) 6));
+
+        assertEquals(7, MINUTES_OF_HOUR.id());
+        assertEquals(MINUTES_OF_HOUR, DateTimeUnit.resolve((byte) 7));
+
+        assertEquals(8, SECOND_OF_MINUTE.id());
+        assertEquals(SECOND_OF_MINUTE, DateTimeUnit.resolve((byte) 8));
+    }
+
+    public void testIsDayOrLonger() {
+        for (DateTimeUnit unit : DateTimeUnit.values()) {
+            if (DateTimeUnit.isDayOrLonger(unit)) {
+                assertTrue(unit == DAY_OF_MONTH ||
+                        unit == MONTH_OF_YEAR ||
+                        unit == QUARTER ||
+                        unit == YEAR_OF_CENTURY ||
+                        unit == WEEK_OF_WEEKYEAR);
+            }
+        }
+    }
+
+}

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

@@ -25,6 +25,7 @@ import org.joda.time.DateTime;
 import org.joda.time.DateTimeZone;
 import org.joda.time.format.ISODateTimeFormat;
 
+import java.util.ArrayList;
 import java.util.concurrent.TimeUnit;
 
 import static org.hamcrest.Matchers.equalTo;
@@ -147,21 +148,37 @@ public class TimeZoneRoundingTests extends ESTestCase {
         Rounding tzRounding;
         // testing savings to non savings switch
         tzRounding = TimeZoneRounding.builder(DateTimeUnit.HOUR_OF_DAY).timeZone(DateTimeZone.forID("UTC")).build();
-        assertThat(tzRounding.round(time("2014-10-26T01:01:01", DateTimeZone.forID("CET"))),
-                equalTo(time("2014-10-26T01:00:00", DateTimeZone.forID("CET"))));
+        assertThat(tzRounding.round(time("2014-10-26T01:01:01", DateTimeZone.forOffsetHours(2))),  // CEST = UTC+2
+                equalTo(time("2014-10-26T01:00:00", DateTimeZone.forOffsetHours(2))));
+        assertThat(tzRounding.nextRoundingValue(time("2014-10-26T01:00:00", DateTimeZone.forOffsetHours(2))),
+                equalTo(time("2014-10-26T02:00:00", DateTimeZone.forOffsetHours(2))));
+        assertThat(tzRounding.nextRoundingValue(time("2014-10-26T02:00:00", DateTimeZone.forOffsetHours(2))),
+                equalTo(time("2014-10-26T03:00:00", DateTimeZone.forOffsetHours(2))));
 
         tzRounding = TimeZoneRounding.builder(DateTimeUnit.HOUR_OF_DAY).timeZone(DateTimeZone.forID("CET")).build();
-        assertThat(tzRounding.round(time("2014-10-26T01:01:01", DateTimeZone.forID("CET"))),
-                equalTo(time("2014-10-26T01:00:00", DateTimeZone.forID("CET"))));
+        assertThat(tzRounding.round(time("2014-10-26T01:01:01", DateTimeZone.forOffsetHours(2))),  // CEST = UTC+2
+                equalTo(time("2014-10-26T01:00:00", DateTimeZone.forOffsetHours(2))));
+        assertThat(tzRounding.nextRoundingValue(time("2014-10-26T01:00:00", DateTimeZone.forOffsetHours(2))),
+                equalTo(time("2014-10-26T02:00:00", DateTimeZone.forOffsetHours(2))));
+        assertThat(tzRounding.nextRoundingValue(time("2014-10-26T02:00:00", DateTimeZone.forOffsetHours(2))),
+                equalTo(time("2014-10-26T03:00:00", DateTimeZone.forOffsetHours(2))));
 
         // testing non savings to savings switch
         tzRounding = TimeZoneRounding.builder(DateTimeUnit.HOUR_OF_DAY).timeZone(DateTimeZone.forID("UTC")).build();
-        assertThat(tzRounding.round(time("2014-03-30T01:01:01", DateTimeZone.forID("CET"))),
-                equalTo(time("2014-03-30T01:00:00", DateTimeZone.forID("CET"))));
+        assertThat(tzRounding.round(time("2014-03-30T01:01:01", DateTimeZone.forOffsetHours(1))),  // CET = UTC+1
+                equalTo(time("2014-03-30T01:00:00", DateTimeZone.forOffsetHours(1))));
+        assertThat(tzRounding.nextRoundingValue(time("2014-03-30T01:00:00", DateTimeZone.forOffsetHours(1))),
+                equalTo(time("2014-03-30T02:00:00", DateTimeZone.forOffsetHours(1))));
+        assertThat(tzRounding.nextRoundingValue(time("2014-03-30T02:00:00", DateTimeZone.forOffsetHours(1))),
+                equalTo(time("2014-03-30T03:00:00", DateTimeZone.forOffsetHours(1))));
 
         tzRounding = TimeZoneRounding.builder(DateTimeUnit.HOUR_OF_DAY).timeZone(DateTimeZone.forID("CET")).build();
-        assertThat(tzRounding.round(time("2014-03-30T01:01:01", DateTimeZone.forID("CET"))),
-                equalTo(time("2014-03-30T01:00:00", DateTimeZone.forID("CET"))));
+        assertThat(tzRounding.round(time("2014-03-30T01:01:01", DateTimeZone.forOffsetHours(1))),  // CET = UTC+1
+                equalTo(time("2014-03-30T01:00:00", DateTimeZone.forOffsetHours(1))));
+        assertThat(tzRounding.nextRoundingValue(time("2014-03-30T01:00:00", DateTimeZone.forOffsetHours(1))),
+                equalTo(time("2014-03-30T02:00:00", DateTimeZone.forOffsetHours(1))));
+        assertThat(tzRounding.nextRoundingValue(time("2014-03-30T02:00:00", DateTimeZone.forOffsetHours(1))),
+                equalTo(time("2014-03-30T03:00:00", DateTimeZone.forOffsetHours(1))));
 
         // testing non savings to savings switch (America/Chicago)
         tzRounding = TimeZoneRounding.builder(DateTimeUnit.HOUR_OF_DAY).timeZone(DateTimeZone.forID("UTC")).build();
@@ -210,6 +227,31 @@ public class TimeZoneRoundingTests extends ESTestCase {
         }
     }
 
+    /**
+     * Test that nextRoundingValue() for hour rounding (and smaller) is equally spaced (see #18326)
+     * Start at a random date in a random time zone, then find the next zone offset transition (if any).
+     * From there, check that when we advance by using rounding#nextRoundingValue(), we always advance by the same
+     * amount of milliseconds.
+     */
+    public void testSubHourNextRoundingEquallySpaced() {
+        String timeZone = randomFrom(new ArrayList<>(DateTimeZone.getAvailableIDs()));
+        DateTimeUnit unit = randomFrom(new DateTimeUnit[] { DateTimeUnit.HOUR_OF_DAY, DateTimeUnit.MINUTES_OF_HOUR,
+                DateTimeUnit.SECOND_OF_MINUTE });
+        DateTimeZone tz = DateTimeZone.forID(timeZone);
+        TimeZoneRounding rounding = new TimeZoneRounding.TimeUnitRounding(unit, tz);
+        // move the random date to transition for timezones that have offset change due to dst transition
+        long nextTransition = tz.nextTransition(Math.abs(randomLong() % ((long) 10e11)));
+        final long millisPerUnit = unit.field().getDurationField().getUnitMillis();
+        // start ten units before transition
+        long roundedDate = rounding.round(nextTransition - (10 * millisPerUnit));
+        while (roundedDate < nextTransition + 10 * millisPerUnit) {
+            long delta = rounding.nextRoundingValue(roundedDate) - roundedDate;
+            assertEquals("Difference between rounded values not equally spaced for [" + unit.name() + "], [" + timeZone + "] at "
+                    + new DateTime(roundedDate), millisPerUnit, delta);
+            roundedDate = rounding.nextRoundingValue(roundedDate);
+        }
+    }
+
     /**
      * randomized test on TimeIntervalRounding with random interval and time zone offsets
      */

core/src/test/java/org/elasticsearch/search/aggregations/bucket/DateHistogramIT.java

@@ -24,6 +24,7 @@ import org.elasticsearch.common.joda.DateMathParser;
 import org.elasticsearch.common.joda.Joda;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.index.mapper.core.DateFieldMapper;
+import org.elasticsearch.index.query.MatchNoneQueryBuilder;
 import org.elasticsearch.index.query.QueryBuilders;
 import org.elasticsearch.plugins.Plugin;
 import org.elasticsearch.script.Script;
@@ -1146,4 +1147,27 @@ public class DateHistogramIT extends ESIntegTestCase {
         Histogram histo = response.getAggregations().get("histo");
         assertThat(histo.getBuckets().size(), greaterThan(0));
     }
+
+    /**
+     * When DST ends, local time turns back one hour, so between 2am and 4am wall time we should have four buckets:
+     * "2015-10-25T02:00:00.000+02:00",
+     * "2015-10-25T02:00:00.000+01:00",
+     * "2015-10-25T03:00:00.000+01:00",
+     * "2015-10-25T04:00:00.000+01:00".
+     */
+    public void testDSTEndTransition() throws Exception {
+        SearchResponse response = client().prepareSearch("idx")
+                .setQuery(new MatchNoneQueryBuilder())
+                .addAggregation(dateHistogram("histo").field("date").timeZone(DateTimeZone.forID("Europe/Oslo"))
+                        .dateHistogramInterval(DateHistogramInterval.HOUR).minDocCount(0).extendedBounds(
+                                new ExtendedBounds("2015-10-25T02:00:00.000+02:00", "2015-10-25T04:00:00.000+01:00")))
+                .execute().actionGet();
+
+        Histogram histo = response.getAggregations().get("histo");
+        List<? extends Bucket> buckets = histo.getBuckets();
+        assertThat(buckets.size(), equalTo(4));
+        assertThat(((DateTime) buckets.get(1).getKey()).getMillis() - ((DateTime) buckets.get(0).getKey()).getMillis(), equalTo(3600000L));
+        assertThat(((DateTime) buckets.get(2).getKey()).getMillis() - ((DateTime) buckets.get(1).getKey()).getMillis(), equalTo(3600000L));
+        assertThat(((DateTime) buckets.get(3).getKey()).getMillis() - ((DateTime) buckets.get(2).getKey()).getMillis(), equalTo(3600000L));
+    }
 }