From a2372778ddb7445282f8a977b81464afcc631b12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Tue, 31 May 2016 21:42:45 +0200 Subject: [PATCH] Fix problem with TimeIntervalRounding on DST end Due to an error in our current TimeIntervalRounding, two dates can round to the same key, even when they are 1h apart when using short interval roundings (e.g. 20m) and a time zone with DST change. Here is an example for the CET time zone: On 25 October 2015, 03:00:00 clocks are turned backward 1 hour to 02:00:00 local standard time. The dates "2015-10-25T02:15:00+02:00" (1445732100000) (before DST end) and "2015-10-25T02:15:00+01:00" (1445735700000) (after DST end) are thus 1h apart, but currently they round to the same value "2015-10-25T02:00:00.000+01:00" (1445734800000). This violates an important invariant of rounding, namely that the rounded value must be less or equal to the value that is rounded. It also leads to wrong histogram bucket counts because documents in [02:00:00+02:00, 02:20:00+02:00) go to the same bucket as documents from [02:00:00+01:00, 02:20:00+01:00). The problem happens because in TimeIntervalRounding#roundKey() we need to perform the rounding operation in local time, but on converting back to UTC we don't honor the original values time zone offset. This fix changes that and adds tests both for DST start and DST end as well as a test that demonstrates what happens to bucket sizes when the dst change is not evently divisibly by the interval. --- .../common/rounding/TimeZoneRounding.java | 5 +- .../rounding/TimeZoneRoundingTests.java | 51 +++++++++++++++++++ 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/rounding/TimeZoneRounding.java b/core/src/main/java/org/elasticsearch/common/rounding/TimeZoneRounding.java index 5bb868ed253..c78d6c730b6 100644 --- a/core/src/main/java/org/elasticsearch/common/rounding/TimeZoneRounding.java +++ b/core/src/main/java/org/elasticsearch/common/rounding/TimeZoneRounding.java @@ -216,10 +216,9 @@ public abstract class TimeZoneRounding extends Rounding { @Override public long roundKey(long utcMillis) { - long timeLocal = utcMillis; - timeLocal = timeZone.convertUTCToLocal(utcMillis); + long timeLocal = timeZone.convertUTCToLocal(utcMillis); long rounded = Rounding.Interval.roundValue(Rounding.Interval.roundKey(timeLocal, interval), interval); - return timeZone.convertLocalToUTC(rounded, false); + return timeZone.convertLocalToUTC(rounded, false, utcMillis); } @Override diff --git a/core/src/test/java/org/elasticsearch/common/rounding/TimeZoneRoundingTests.java b/core/src/test/java/org/elasticsearch/common/rounding/TimeZoneRoundingTests.java index af7b53712c1..49ba26231da 100644 --- a/core/src/test/java/org/elasticsearch/common/rounding/TimeZoneRoundingTests.java +++ b/core/src/test/java/org/elasticsearch/common/rounding/TimeZoneRoundingTests.java @@ -258,6 +258,57 @@ public class TimeZoneRoundingTests extends ESTestCase { } } + /** + * test DST end with interval rounding + * CET: 25 October 2015, 03:00:00 clocks were turned backward 1 hour to 25 October 2015, 02:00:00 local standard time + */ + public void testTimeIntervalCET_DST_End() { + long interval = TimeUnit.MINUTES.toMillis(20); + TimeZoneRounding rounding = new TimeIntervalRounding(interval, DateTimeZone.forID("CET")); + + assertThat(rounding.round(time("2015-10-25T01:55:00+02:00")), equalTo(time("2015-10-25T01:40:00+02:00"))); + assertThat(rounding.round(time("2015-10-25T02:15:00+02:00")), equalTo(time("2015-10-25T02:00:00+02:00"))); + assertThat(rounding.round(time("2015-10-25T02:35:00+02:00")), equalTo(time("2015-10-25T02:20:00+02:00"))); + assertThat(rounding.round(time("2015-10-25T02:55:00+02:00")), equalTo(time("2015-10-25T02:40:00+02:00"))); + // after DST shift + assertThat(rounding.round(time("2015-10-25T02:15:00+01:00")), equalTo(time("2015-10-25T02:00:00+01:00"))); + assertThat(rounding.round(time("2015-10-25T02:35:00+01:00")), equalTo(time("2015-10-25T02:20:00+01:00"))); + assertThat(rounding.round(time("2015-10-25T02:55:00+01:00")), equalTo(time("2015-10-25T02:40:00+01:00"))); + assertThat(rounding.round(time("2015-10-25T03:15:00+01:00")), equalTo(time("2015-10-25T03:00:00+01:00"))); + } + + /** + * test DST start with interval rounding + * CET: 27 March 2016, 02:00:00 clocks were turned forward 1 hour to 27 March 2016, 03:00:00 local daylight time + */ + public void testTimeIntervalCET_DST_Start() { + long interval = TimeUnit.MINUTES.toMillis(20); + TimeZoneRounding rounding = new TimeIntervalRounding(interval, DateTimeZone.forID("CET")); + // test DST start + assertThat(rounding.round(time("2016-03-27T01:55:00+01:00")), equalTo(time("2016-03-27T01:40:00+01:00"))); + assertThat(rounding.round(time("2016-03-27T02:00:00+01:00")), equalTo(time("2016-03-27T03:00:00+02:00"))); + assertThat(rounding.round(time("2016-03-27T03:15:00+02:00")), equalTo(time("2016-03-27T03:00:00+02:00"))); + assertThat(rounding.round(time("2016-03-27T03:35:00+02:00")), equalTo(time("2016-03-27T03:20:00+02:00"))); + } + + /** + * test DST start with offset not fitting interval, e.g. Asia/Kathmandu + * adding 15min on 1986-01-01T00:00:00 the interval from + * 1986-01-01T00:15:00+05:45 to 1986-01-01T00:20:00+05:45 to only be 5min + * long + */ + public void testTimeInterval_Kathmandu_DST_Start() { + long interval = TimeUnit.MINUTES.toMillis(20); + TimeZoneRounding rounding = new TimeIntervalRounding(interval, DateTimeZone.forID("Asia/Kathmandu")); + assertThat(rounding.round(time("1985-12-31T23:55:00+05:30")), equalTo(time("1985-12-31T23:40:00+05:30"))); + assertThat(rounding.round(time("1986-01-01T00:16:00+05:45")), equalTo(time("1986-01-01T00:15:00+05:45"))); + assertThat(time("1986-01-01T00:15:00+05:45") - time("1985-12-31T23:40:00+05:30"), equalTo(TimeUnit.MINUTES.toMillis(20))); + assertThat(rounding.round(time("1986-01-01T00:26:00+05:45")), equalTo(time("1986-01-01T00:20:00+05:45"))); + assertThat(time("1986-01-01T00:20:00+05:45") - time("1986-01-01T00:15:00+05:45"), equalTo(TimeUnit.MINUTES.toMillis(5))); + assertThat(rounding.round(time("1986-01-01T00:46:00+05:45")), equalTo(time("1986-01-01T00:40:00+05:45"))); + assertThat(time("1986-01-01T00:40:00+05:45") - time("1986-01-01T00:20:00+05:45"), equalTo(TimeUnit.MINUTES.toMillis(20))); + } + /** * randomized test on {@link TimeIntervalRounding} with random interval and time zone offsets */