Fix timezone conversion bug in date_trunc (facebookincubator#10183)

Summary:
Pull Request resolved: facebookincubator#10183

date_trunc was incorrectly handling truncations during the daylight
savings transition. Since conversions during these transitions may be
ambiguous, Presto semantic is to always take the earliest UTC timestamp.

For truncations that should happen in the "latest" side of an ambiguous
conversion, converting to UTC and back would result in the truncation being
done is the earliest version of that timestamp. The bug only applied to "minute"
and "hour" truncations, naturally.

Reviewed By: kevinwilfong, mbasmanova

Differential Revision: D58467148

fbshipit-source-id: dedaff4e599516a00c13dd9f372e6a6b547c2975

velox/functions/prestosql/DateTimeFunctions.h

@@ -1030,22 +1030,42 @@ struct DateTruncFunction : public TimestampWithTimezoneSupport<T> {
     }
 
     switch (unit) {
+      // For seconds, we just truncate the nanoseconds part of the timestamp; no
+      // timezone conversion required.
       case DateTimeUnit::kSecond:
         result = Timestamp(timestamp.getSeconds(), 0);
         return;
+
+      // Same for minutes; timezones and daylight savings time are at least in
+      // the granularity of 30 mins, so we can just truncate the epoch directly.
       case DateTimeUnit::kMinute:
-        result = adjustEpoch(getSeconds(timestamp, timeZone_), 60);
-        break;
-      case DateTimeUnit::kHour:
-        result = adjustEpoch(getSeconds(timestamp, timeZone_), 60 * 60);
-        break;
+        result = adjustEpoch(timestamp.getSeconds(), 60);
+        return;
+
+      // Hour truncation has to handle the corner case of daylight savings time
+      // boundaries. Since conversions from local timezone to UTC may be
+      // ambiguous, we need to be carefull about the roundtrip of converting to
+      // local time and back. So what we do is to calculate the truncation delta
+      // in UTC, then applying it to the input timestamp.
+      case DateTimeUnit::kHour: {
+        auto epochToAdjust = getSeconds(timestamp, timeZone_);
+        auto secondsDelta =
+            epochToAdjust - adjustEpoch(epochToAdjust, 60 * 60).getSeconds();
+        result = Timestamp(timestamp.getSeconds() - secondsDelta, 0);
+        return;
+      }
+
+      // For the truncations below, we may first need to convert to the local
+      // timestamp, truncate, then convert back to GMT.
       case DateTimeUnit::kDay:
         result = adjustEpoch(getSeconds(timestamp, timeZone_), 24 * 60 * 60);
         break;
+
       default:
         auto dateTime = getDateTime(timestamp, timeZone_);
         adjustDateTime(dateTime, unit);
         result = Timestamp(Timestamp::calendarUtcToEpoch(dateTime), 0);
+        break;
     }
 
     if (timeZone_ != nullptr) {

velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp

@@ -1640,6 +1640,7 @@ TEST_F(DateTimeFunctionsTest, dateTrunc) {
   EXPECT_EQ(std::nullopt, dateTrunc("second", std::nullopt));
   EXPECT_EQ(Timestamp(0, 0), dateTrunc("second", Timestamp(0, 0)));
   EXPECT_EQ(Timestamp(0, 0), dateTrunc("second", Timestamp(0, 123)));
+
   EXPECT_EQ(Timestamp(-57600, 0), dateTrunc("day", Timestamp(0, 0)));
   EXPECT_EQ(
       Timestamp(998474645, 0),
@@ -1666,6 +1667,13 @@ TEST_F(DateTimeFunctionsTest, dateTrunc) {
       Timestamp(978336000, 0),
       dateTrunc("year", Timestamp(998'474'645, 321'001'234)));
 
+  // Check that truncation during daylight saving transition where conversions
+  // may be ambiguous return the right values.
+  EXPECT_EQ(
+      Timestamp(1667725200, 0), dateTrunc("hour", Timestamp(1667725200, 0)));
+  EXPECT_EQ(
+      Timestamp(1667725200, 0), dateTrunc("minute", Timestamp(1667725200, 0)));
+
   setQueryTimeZone("Asia/Kolkata");
 
   EXPECT_EQ(std::nullopt, dateTrunc("second", std::nullopt));