Refactor time point boundary check code (facebookincubator#10587)

Summary:
Pull Request resolved: facebookincubator#10587

Refactor time point boundary check code to make it less fragmented,
and adding more tests.

Reviewed By: kagamiori

Differential Revision: D60322812

fbshipit-source-id: e3483b972305d33ad3c7661964546626a2c16d6d

Author: pedroerp

velox/type/Timestamp.cpp

@@ -58,19 +58,8 @@ Timestamp Timestamp::now() {
 }
 
 void Timestamp::toGMT(const tz::TimeZone& zone) {
-  // Magic number -2^39 + 24*3600. This number and any number lower than that
-  // will cause time_zone::to_sys() to SIGABRT. We don't want that to happen.
-  VELOX_USER_CHECK_GT(
-      seconds_,
-      -1096193779200l + 86400l,
-      "Timestamp seconds out of range for time zone adjustment");
-
-  VELOX_USER_CHECK_LE(
-      seconds_,
-      kMaxSeconds,
-      "Timestamp seconds out of range for time zone adjustment");
-
   std::chrono::seconds sysSeconds;
+
   try {
     sysSeconds = zone.to_sys(std::chrono::seconds(seconds_));
   } catch (const date::ambiguous_local_time&) {
@@ -96,49 +85,16 @@ void Timestamp::toGMT(int16_t tzID) {
   }
 }
 
-namespace {
-void validateTimePoint(const std::chrono::time_point<
-                       std::chrono::system_clock,
-                       std::chrono::milliseconds>& timePoint) {
-  // Due to the limit of std::chrono we can only represent time in
-  // [-32767-01-01, 32767-12-31] date range
-  const auto minTimePoint = date::sys_days{
-      date::year_month_day(date::year::min(), date::month(1), date::day(1))};
-  const auto maxTimePoint = date::sys_days{
-      date::year_month_day(date::year::max(), date::month(12), date::day(31))};
-  if (timePoint < minTimePoint || timePoint > maxTimePoint) {
-    VELOX_USER_FAIL(
-        "Timestamp is outside of supported range of [{}-{}-{}, {}-{}-{}]",
-        (int)date::year::min(),
-        "01",
-        "01",
-        (int)date::year::max(),
-        "12",
-        "31");
-  }
-}
-} // namespace
-
 std::chrono::time_point<std::chrono::system_clock, std::chrono::milliseconds>
 Timestamp::toTimePointMs(bool allowOverflow) const {
   using namespace std::chrono;
   auto tp = time_point<system_clock, milliseconds>(
       milliseconds(allowOverflow ? toMillisAllowOverflow() : toMillis()));
-  validateTimePoint(tp);
-  return tp;
-}
-
-std::chrono::time_point<std::chrono::system_clock, std::chrono::seconds>
-Timestamp::toTimePointSec() const {
-  using namespace std::chrono;
-  auto tp = time_point<system_clock, seconds>(seconds(seconds_));
-  validateTimePoint(tp);
+  tz::validateRange(tp);
   return tp;
 }
 
 void Timestamp::toTimezone(const tz::TimeZone& zone) {
-  auto tp = toTimePointSec();
-
   try {
     seconds_ = zone.to_local(std::chrono::seconds(seconds_)).count();
   } catch (const std::invalid_argument& e) {

velox/type/Timestamp.h

@@ -207,14 +207,6 @@ struct Timestamp {
   std::chrono::time_point<std::chrono::system_clock, std::chrono::milliseconds>
   toTimePointMs(bool allowOverflow = false) const;
 
-  /// Exports the current timestamp as a std::chrono::time_point of second
-  /// precision.
-  ///
-  /// Due to the limit of velox/external/date, throws if timestamp is outside of
-  /// [-32767-01-01, 32767-12-31] range.
-  std::chrono::time_point<std::chrono::system_clock, std::chrono::seconds>
-  toTimePointSec() const;
-
   static Timestamp fromMillis(int64_t millis) {
     if (millis >= 0 || millis % 1'000 == 0) {
       return Timestamp(millis / 1'000, (millis % 1'000) * 1'000'000);

velox/type/tests/TimestampTest.cpp

@@ -397,17 +397,12 @@ TEST(TimestampTest, outOfRange) {
   auto* timezone = tz::locateZone("GMT");
   Timestamp t1(-3217830796800, 0);
 
-  VELOX_ASSERT_THROW(
-      t1.toTimePointMs(), "Timestamp is outside of supported range");
-  VELOX_ASSERT_THROW(
-      t1.toTimePointSec(), "Timestamp is outside of supported range");
-  VELOX_ASSERT_THROW(
-      t1.toTimezone(*timezone), "Timestamp is outside of supported range");
+  std::string expected = "Timepoint is outside of supported year range";
+  VELOX_ASSERT_THROW(t1.toTimePointMs(), expected);
+  VELOX_ASSERT_THROW(t1.toTimezone(*timezone), expected);
 
   timezone = tz::locateZone("America/Los_Angeles");
-  VELOX_ASSERT_THROW(
-      t1.toGMT(*timezone),
-      "Timestamp seconds out of range for time zone adjustment");
+  VELOX_ASSERT_THROW(t1.toGMT(*timezone), expected);
 
   // #2. external/date doesn't understand OS_TZDB repetition rules. Therefore,
   // for timezones with pre-defined repetition rules for daylight savings, for

velox/type/tz/TimeZoneMap.cpp

@@ -167,8 +167,33 @@ std::string normalizeTimeZone(const std::string& originalZoneId) {
   return originalZoneId;
 }
 
+template <typename TDuration>
+void validateRangeImpl(time_point<TDuration> timePoint) {
+  using namespace velox::date;
+  static constexpr auto kMinYear = date::year::min();
+  static constexpr auto kMaxYear = date::year::max();
+
+  auto year = year_month_day(floor<days>(timePoint)).year();
+
+  if (year < kMinYear || year > kMaxYear) {
+    VELOX_USER_FAIL(
+        "Timepoint is outside of supported year range: [{}, {}], got {}",
+        (int)kMinYear,
+        (int)kMaxYear,
+        (int)year);
+  }
+}
+
 } // namespace
 
+void validateRange(time_point<std::chrono::seconds> timePoint) {
+  validateRangeImpl(timePoint);
+}
+
+void validateRange(time_point<std::chrono::milliseconds> timePoint) {
+  validateRangeImpl(timePoint);
+}
+
 std::string getTimeZoneName(int64_t timeZoneID) {
   const auto& timeZoneDatabase = getTimeZoneDatabase();
 
@@ -245,6 +270,7 @@ TimeZone::seconds TimeZone::to_sys(
     TimeZone::seconds timestamp,
     TimeZone::TChoose choose) const {
   date::local_seconds timePoint{timestamp};
+  validateRange(date::sys_seconds{timestamp});
 
   if (tz_ == nullptr) {
     // We can ignore `choose` as time offset conversions are always linear.
@@ -266,6 +292,7 @@ TimeZone::seconds TimeZone::to_sys(
 
 TimeZone::seconds TimeZone::to_local(TimeZone::seconds timestamp) const {
   date::sys_seconds timePoint{timestamp};
+  validateRange(timePoint);
 
   // If this is an offset time zone.
   if (tz_ == nullptr) {

velox/type/tz/TimeZoneMap.h

@@ -57,6 +57,14 @@ int16_t getTimeZoneID(std::string_view timeZone, bool failOnError = true);
 /// [-14:00, +14:00] range.
 int16_t getTimeZoneID(int32_t offsetMinutes);
 
+// Validates that the time point can be safely used by the external date
+// library.
+template <typename T>
+using time_point = std::chrono::time_point<std::chrono::system_clock, T>;
+
+void validateRange(time_point<std::chrono::seconds> timePoint);
+void validateRange(time_point<std::chrono::milliseconds> timePoint);
+
 /// TimeZone is the proxy object for time zone management. It provides access to
 /// time zone names, their IDs (as defined in TimeZoneDatabase.cpp and
 /// consistent with Presto), and utilities for timestamp conversion across

velox/type/tz/tests/TimeZoneMapTest.cpp

@@ -18,6 +18,7 @@
 
 #include "velox/common/base/Exceptions.h"
 #include "velox/common/base/tests/GTestUtils.h"
+#include "velox/external/date/date.h"
 #include "velox/type/tz/TimeZoneMap.h"
 
 namespace facebook::velox::tz {
@@ -115,6 +116,42 @@ TEST(TimeZoneMapTest, offsetToSys) {
   EXPECT_NE(toSysTime("-07:00", ts), toSysTime("America/Los_Angeles", ts));
 }
 
+TEST(TimeZoneMapTest, timePointBoundary) {
+  using namespace date;
+
+  const auto* tz = locateZone("+00:01");
+  EXPECT_NE(tz, nullptr);
+
+  auto trySysYear = [&](year y) {
+    auto date = year_month_day(y, month(1), day(1));
+    return tz->to_sys(sys_days{date}.time_since_epoch());
+  };
+
+  auto tryLocalYear = [&](year y) {
+    auto date = year_month_day(y, month(1), day(1));
+    return tz->to_local(sys_days{date}.time_since_epoch());
+  };
+
+  EXPECT_NO_THROW(trySysYear(year(0)));
+  EXPECT_NO_THROW(trySysYear(year::max()));
+  EXPECT_NO_THROW(trySysYear(year::min()));
+
+  EXPECT_NO_THROW(tryLocalYear(year(0)));
+  EXPECT_NO_THROW(tryLocalYear(year::max()));
+  EXPECT_NO_THROW(tryLocalYear(year::min()));
+
+  std::string expected = "Timepoint is outside of supported year range";
+  VELOX_ASSERT_THROW(trySysYear(year(int(year::max()) + 1)), expected);
+  VELOX_ASSERT_THROW(trySysYear(year(int(year::min()) - 1)), expected);
+
+  VELOX_ASSERT_THROW(tryLocalYear(year(int(year::max()) + 1)), expected);
+  VELOX_ASSERT_THROW(tryLocalYear(year(int(year::min()) - 1)), expected);
+
+  // This time point triggers an assertion failure in external/date. Make sure
+  // we catch and throw before getting to that point.
+  VELOX_ASSERT_THROW(tz->to_sys(seconds{-1096193779200l - 86400l}), expected);
+}
+
 TEST(TimeZoneMapTest, getTimeZoneName) {
   EXPECT_EQ("America/Los_Angeles", getTimeZoneName(1825));
   EXPECT_EQ("Europe/Moscow", getTimeZoneName(2079));