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

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));