From 7e8f3e69ef761b8471f50e070c02b830f78bf5b6 Mon Sep 17 00:00:00 2001 From: Vishesh Garg Date: Wed, 4 Oct 2023 11:32:29 +0530 Subject: [PATCH] Avoid intermediate offsets in bucketStart calculation logic to handle DST transition (#15038) When moving timestamps by an offset using org.joda.time.chrono.ISOChronology library, if the new timestamp falls in Daylight Savings Time (DST) transition period, the library rounds it off to the nearest valid time. This can lead to incorrect final timestamp when calculated using intermediate offsets landing in DST transition, for e.g. +21D arrived at using +14D and +7D offset, where +14D lands in DST transition period. Since bucketStart values are calculated using this library, this behaviour can lead to incorrect bucketStart times. --- .../common/granularity/PeriodGranularity.java | 8 +- .../granularity/QueryGranularityTest.java | 89 ++++++++++++++++++- 2 files changed, 92 insertions(+), 5 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/java/util/common/granularity/PeriodGranularity.java b/processing/src/main/java/org/apache/druid/java/util/common/granularity/PeriodGranularity.java index 4f9acefd284..ba9ba02feb9 100644 --- a/processing/src/main/java/org/apache/druid/java/util/common/granularity/PeriodGranularity.java +++ b/processing/src/main/java/org/apache/druid/java/util/common/granularity/PeriodGranularity.java @@ -250,7 +250,7 @@ public class PeriodGranularity extends Granularity implements JsonSerializable long tt = chronology.years().add(origin, y); // always round down to the previous period (for timestamps prior to origin) if (t < tt) { - t = chronology.years().add(tt, -years); + t = chronology.years().add(origin, y - years); } else { t = tt; } @@ -268,7 +268,7 @@ public class PeriodGranularity extends Granularity implements JsonSerializable long tt = chronology.months().add(origin, m); // always round down to the previous period (for timestamps prior to origin) if (t < tt) { - t = chronology.months().add(tt, -months); + t = chronology.months().add(origin, m - months); } else { t = tt; } @@ -287,7 +287,7 @@ public class PeriodGranularity extends Granularity implements JsonSerializable long tt = chronology.weeks().add(origin, w); // always round down to the previous period (for timestamps prior to origin) if (t < tt) { - t = chronology.weeks().add(tt, -weeks); + t = chronology.weeks().add(origin, w - weeks); } else { t = tt; } @@ -308,7 +308,7 @@ public class PeriodGranularity extends Granularity implements JsonSerializable long tt = chronology.days().add(origin, d); // always round down to the previous period (for timestamps prior to origin) if (t < tt) { - t = chronology.days().add(tt, -days); + t = chronology.days().add(origin, d - days); } else { t = tt; } diff --git a/processing/src/test/java/org/apache/druid/granularity/QueryGranularityTest.java b/processing/src/test/java/org/apache/druid/granularity/QueryGranularityTest.java index 639a7c529ed..dbb952fbf1c 100644 --- a/processing/src/test/java/org/apache/druid/granularity/QueryGranularityTest.java +++ b/processing/src/test/java/org/apache/druid/granularity/QueryGranularityTest.java @@ -356,6 +356,93 @@ public class QueryGranularityTest hour.bucketStart(DateTimes.of("2012-11-04T03:30:00-08:00")) ) ); + + final PeriodGranularity p7days = new PeriodGranularity( + new Period("P7D"), + DateTimes.of("2022-03-24T02:35:00.000-07:00"), + tz + ); + assertSameDateTime( + Lists.newArrayList( + new DateTime("2022-03-03T02:35:00.000-08:00", tz), + new DateTime("2022-03-10T02:35:00.000-08:00", tz), + new DateTime("2022-03-24T02:35:00.000-07:00", tz), + new DateTime("2022-03-31T02:35:00.000-07:00", tz) + + ), + Lists.newArrayList( + p7days.bucketStart(DateTimes.of("2022-03-04T02:35:00.000-08:00")), + p7days.bucketStart(DateTimes.of("2022-03-16T02:35:00.000-07:00")), + p7days.bucketStart(DateTimes.of("2022-03-26T02:35:00.000-07:00")), + p7days.bucketStart(DateTimes.of("2022-03-31T03:35:00.000-07:00")) + ) + ); + + final PeriodGranularity week = new PeriodGranularity( + new Period("P1W"), + DateTimes.of("2022-03-24T02:35:00.000-07:00"), + tz + ); + + assertSameDateTime( + Lists.newArrayList( + new DateTime("2022-03-03T02:35:00.000-08:00", tz), + new DateTime("2022-03-10T02:35:00.000-08:00", tz), + new DateTime("2022-03-24T02:35:00.000-07:00", tz), + new DateTime("2022-03-31T02:35:00.000-07:00", tz) + + ), + Lists.newArrayList( + week.bucketStart(DateTimes.of("2022-03-04T02:35:00.000-08:00")), + week.bucketStart(DateTimes.of("2022-03-16T02:35:00.000-07:00")), + week.bucketStart(DateTimes.of("2022-03-26T02:35:00.000-07:00")), + week.bucketStart(DateTimes.of("2022-03-31T03:35:00.000-07:00")) + ) + ); + + final PeriodGranularity month = new PeriodGranularity( + new Period("P1M"), + DateTimes.of("2022-03-24T02:35:00.000-07:00"), + tz + ); + + assertSameDateTime( + Lists.newArrayList( + new DateTime("2022-02-24T02:35:00.000-08:00", tz), + new DateTime("2022-02-24T02:35:00.000-08:00", tz), + new DateTime("2022-03-24T02:35:00.000-07:00", tz), + new DateTime("2022-03-24T02:35:00.000-07:00", tz) + + ), + Lists.newArrayList( + month.bucketStart(DateTimes.of("2022-03-04T02:35:00.000-08:00")), + month.bucketStart(DateTimes.of("2022-03-16T02:35:00.000-07:00")), + month.bucketStart(DateTimes.of("2022-03-26T02:35:00.000-07:00")), + month.bucketStart(DateTimes.of("2022-03-31T03:35:00.000-07:00")) + ) + ); + + final PeriodGranularity year = new PeriodGranularity( + new Period("P1Y"), + DateTimes.of("2022-03-24T02:35:00.000-07:00"), + tz + ); + + assertSameDateTime( + Lists.newArrayList( + new DateTime("2021-03-24T02:35:00.000-07:00", tz), + new DateTime("2021-03-24T02:35:00.000-07:00", tz), + new DateTime("2022-03-24T02:35:00.000-07:00", tz), + new DateTime("2022-03-24T02:35:00.000-07:00", tz) + + ), + Lists.newArrayList( + year.bucketStart(DateTimes.of("2022-03-04T02:35:00.000-08:00")), + year.bucketStart(DateTimes.of("2022-03-16T02:35:00.000-07:00")), + year.bucketStart(DateTimes.of("2022-03-26T02:35:00.000-07:00")), + year.bucketStart(DateTimes.of("2022-03-31T03:35:00.000-07:00")) + ) + ); } @Test @@ -877,7 +964,7 @@ public class QueryGranularityTest Assert.assertFalse("actualIter not exhausted!?", actualIter.hasNext()); Assert.assertFalse("expectedIter not exhausted!?", expectedIter.hasNext()); } - + @Test public void testTruncateKathmandu() {