Make sure TimeIntervalRounding is monotonic for increasing dates (#19020)
Currently there are cases when using TimeIntervalRounding#round() and date1 < date2 that round(date2) < round(date1). These errors can happen when using a non-fixed time zone and the values to be rounded are slightly after a time zone offset change (e.g. DST transition). Here is an example for the "CET" time zone with a 45 minute rounding interval. The dates to be rounded are on the left (with utc time stamp), the rounded values on the right. The error case is marked: 2011-10-30T01:40:00.000+02:00 1319931600000 | 2011-10-30T01:30:00.000+02:00 1319931000000 2011-10-30T02:02:30.000+02:00 1319932950000 | 2011-10-30T01:30:00.000+02:00 1319931000000 2011-10-30T02:25:00.000+02:00 1319934300000 | 2011-10-30T02:15:00.000+02:00 1319933700000 2011-10-30T02:47:30.000+02:00 1319935650000 | 2011-10-30T02:15:00.000+02:00 1319933700000 2011-10-30T02:10:00.000+01:00 1319937000000 | 2011-10-30T01:30:00.000+02:00 1319931000000 * 2011-10-30T02:32:30.000+01:00 1319938350000 | 2011-10-30T02:15:00.000+01:00 1319937300000 2011-10-30T02:55:00.000+01:00 1319939700000 | 2011-10-30T02:15:00.000+01:00 1319937300000 2011-10-30T03:17:30.000+01:00 1319941050000 | 2011-10-30T03:00:00.000+01:00 1319940000000 We should correct this by detecting that we are crossing a transition when rounding, and in that case pick the largest valid rounded value before the transition. This change adds this correction logic to the rounding function and adds this invariant to the randomized TimeIntervalRounding tests. Also adding the example test case from above (with corrected behaviour) for illustrative purposes.
This commit is contained in:
parent
dbf1f61c5b
commit
afb5e6332b
|
@ -222,6 +222,11 @@ public abstract class TimeZoneRounding extends Rounding {
|
||||||
long roundedUTC;
|
long roundedUTC;
|
||||||
if (isInDSTGap(rounded) == false) {
|
if (isInDSTGap(rounded) == false) {
|
||||||
roundedUTC = timeZone.convertLocalToUTC(rounded, true, utcMillis);
|
roundedUTC = timeZone.convertLocalToUTC(rounded, true, utcMillis);
|
||||||
|
// check if we crossed DST transition, in this case we want the last rounded value before the transition
|
||||||
|
long transition = timeZone.previousTransition(utcMillis);
|
||||||
|
if (transition != utcMillis && transition > roundedUTC) {
|
||||||
|
roundedUTC = roundKey(transition - 1);
|
||||||
|
}
|
||||||
} else {
|
} else {
|
||||||
/*
|
/*
|
||||||
* Edge case where the rounded local time is illegal and landed
|
* Edge case where the rounded local time is illegal and landed
|
||||||
|
|
|
@ -19,6 +19,7 @@
|
||||||
|
|
||||||
package org.elasticsearch.common.rounding;
|
package org.elasticsearch.common.rounding;
|
||||||
|
|
||||||
|
import org.elasticsearch.common.collect.Tuple;
|
||||||
import org.elasticsearch.common.rounding.TimeZoneRounding.TimeIntervalRounding;
|
import org.elasticsearch.common.rounding.TimeZoneRounding.TimeIntervalRounding;
|
||||||
import org.elasticsearch.common.rounding.TimeZoneRounding.TimeUnitRounding;
|
import org.elasticsearch.common.rounding.TimeZoneRounding.TimeUnitRounding;
|
||||||
import org.elasticsearch.common.unit.TimeValue;
|
import org.elasticsearch.common.unit.TimeValue;
|
||||||
|
@ -31,10 +32,13 @@ import org.joda.time.DateTimeConstants;
|
||||||
import org.joda.time.DateTimeZone;
|
import org.joda.time.DateTimeZone;
|
||||||
import org.joda.time.format.ISODateTimeFormat;
|
import org.joda.time.format.ISODateTimeFormat;
|
||||||
|
|
||||||
|
import java.util.ArrayList;
|
||||||
|
import java.util.List;
|
||||||
import java.util.concurrent.TimeUnit;
|
import java.util.concurrent.TimeUnit;
|
||||||
|
|
||||||
import static org.hamcrest.Matchers.equalTo;
|
import static org.hamcrest.Matchers.equalTo;
|
||||||
import static org.hamcrest.Matchers.greaterThan;
|
import static org.hamcrest.Matchers.greaterThan;
|
||||||
|
import static org.hamcrest.Matchers.greaterThanOrEqualTo;
|
||||||
import static org.hamcrest.Matchers.lessThan;
|
import static org.hamcrest.Matchers.lessThan;
|
||||||
import static org.hamcrest.Matchers.lessThanOrEqualTo;
|
import static org.hamcrest.Matchers.lessThanOrEqualTo;
|
||||||
|
|
||||||
|
@ -328,7 +332,13 @@ public class TimeZoneRoundingTests extends ESTestCase {
|
||||||
long interval = unit.toMillis(randomIntBetween(1, 365));
|
long interval = unit.toMillis(randomIntBetween(1, 365));
|
||||||
DateTimeZone tz = randomDateTimeZone();
|
DateTimeZone tz = randomDateTimeZone();
|
||||||
TimeZoneRounding rounding = new TimeZoneRounding.TimeIntervalRounding(interval, tz);
|
TimeZoneRounding rounding = new TimeZoneRounding.TimeIntervalRounding(interval, tz);
|
||||||
long date = Math.abs(randomLong() % (2 * (long) 10e11)); // 1970-01-01T00:00:00Z - 2033-05-18T05:33:20.000+02:00
|
long mainDate = Math.abs(randomLong() % (2 * (long) 10e11)); // 1970-01-01T00:00:00Z - 2033-05-18T05:33:20.000+02:00
|
||||||
|
if (randomBoolean()) {
|
||||||
|
mainDate = nastyDate(mainDate, tz, interval);
|
||||||
|
}
|
||||||
|
// check two intervals around date
|
||||||
|
long previousRoundedValue = Long.MIN_VALUE;
|
||||||
|
for (long date = mainDate - 2 * interval; date < mainDate + 2 * interval; date += interval / 2) {
|
||||||
try {
|
try {
|
||||||
final long roundedDate = rounding.round(date);
|
final long roundedDate = rounding.round(date);
|
||||||
final long nextRoundingValue = rounding.nextRoundingValue(roundedDate);
|
final long nextRoundingValue = rounding.nextRoundingValue(roundedDate);
|
||||||
|
@ -336,6 +346,7 @@ public class TimeZoneRoundingTests extends ESTestCase {
|
||||||
assertThat("Rounded value smaller or equal than unrounded", 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),
|
assertThat("Values smaller than rounded value should round further down", rounding.round(roundedDate - 1),
|
||||||
lessThan(roundedDate));
|
lessThan(roundedDate));
|
||||||
|
assertThat("Rounding should be >= previous rounding value", roundedDate, greaterThanOrEqualTo(previousRoundedValue));
|
||||||
|
|
||||||
if (tz.isFixed()) {
|
if (tz.isFixed()) {
|
||||||
assertThat("NextRounding value should be greater than date", nextRoundingValue, greaterThan(roundedDate));
|
assertThat("NextRounding value should be greater than date", nextRoundingValue, greaterThan(roundedDate));
|
||||||
|
@ -344,12 +355,46 @@ public class TimeZoneRoundingTests extends ESTestCase {
|
||||||
assertThat("NextRounding value should be a rounded date", nextRoundingValue,
|
assertThat("NextRounding value should be a rounded date", nextRoundingValue,
|
||||||
equalTo(rounding.round(nextRoundingValue)));
|
equalTo(rounding.round(nextRoundingValue)));
|
||||||
}
|
}
|
||||||
|
previousRoundedValue = roundedDate;
|
||||||
} catch (AssertionError e) {
|
} catch (AssertionError e) {
|
||||||
logger.error("Rounding error at {}, timezone {}, interval: {},", new DateTime(date, tz), tz, interval);
|
logger.error("Rounding error at {}, timezone {}, interval: {},", new DateTime(date, tz), tz, interval);
|
||||||
throw e;
|
throw e;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Test that rounded values are always greater or equal to last rounded value if date is increasing.
|
||||||
|
* The example covers an interval around 2011-10-30T02:10:00+01:00, time zone CET, interval: 2700000ms
|
||||||
|
*/
|
||||||
|
public void testIntervalRoundingMonotonic_CET() {
|
||||||
|
long interval = TimeUnit.MINUTES.toMillis(45);
|
||||||
|
DateTimeZone tz = DateTimeZone.forID("CET");
|
||||||
|
TimeZoneRounding rounding = new TimeZoneRounding.TimeIntervalRounding(interval, tz);
|
||||||
|
List<Tuple<String, String>> expectedDates = new ArrayList<Tuple<String, String>>();
|
||||||
|
// first date is the date to be rounded, second the expected result
|
||||||
|
expectedDates.add(new Tuple<>("2011-10-30T01:40:00.000+02:00", "2011-10-30T01:30:00.000+02:00"));
|
||||||
|
expectedDates.add(new Tuple<>("2011-10-30T02:02:30.000+02:00", "2011-10-30T01:30:00.000+02:00"));
|
||||||
|
expectedDates.add(new Tuple<>("2011-10-30T02:25:00.000+02:00", "2011-10-30T02:15:00.000+02:00"));
|
||||||
|
expectedDates.add(new Tuple<>("2011-10-30T02:47:30.000+02:00", "2011-10-30T02:15:00.000+02:00"));
|
||||||
|
expectedDates.add(new Tuple<>("2011-10-30T02:10:00.000+01:00", "2011-10-30T02:15:00.000+02:00"));
|
||||||
|
expectedDates.add(new Tuple<>("2011-10-30T02:32:30.000+01:00", "2011-10-30T02:15:00.000+01:00"));
|
||||||
|
expectedDates.add(new Tuple<>("2011-10-30T02:55:00.000+01:00", "2011-10-30T02:15:00.000+01:00"));
|
||||||
|
expectedDates.add(new Tuple<>("2011-10-30T03:17:30.000+01:00", "2011-10-30T03:00:00.000+01:00"));
|
||||||
|
|
||||||
|
long previousDate = Long.MIN_VALUE;
|
||||||
|
for (Tuple<String, String> dates : expectedDates) {
|
||||||
|
final long roundedDate = rounding.round(time(dates.v1()));
|
||||||
|
assertThat(roundedDate, isDate(time(dates.v2()), tz));
|
||||||
|
assertThat(roundedDate, greaterThanOrEqualTo(previousDate));
|
||||||
|
previousDate = roundedDate;
|
||||||
|
}
|
||||||
|
// here's what this means for interval widths
|
||||||
|
assertEquals(TimeUnit.MINUTES.toMillis(45), time("2011-10-30T02:15:00.000+02:00") - time("2011-10-30T01:30:00.000+02:00"));
|
||||||
|
assertEquals(TimeUnit.MINUTES.toMillis(60), time("2011-10-30T02:15:00.000+01:00") - time("2011-10-30T02:15:00.000+02:00"));
|
||||||
|
assertEquals(TimeUnit.MINUTES.toMillis(45), time("2011-10-30T03:00:00.000+01:00") - time("2011-10-30T02:15:00.000+01:00"));
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* special test for DST switch from #9491
|
* special test for DST switch from #9491
|
||||||
|
|
Loading…
Reference in New Issue