From 671e12679ab047d960a99f62d06f78be64134164 Mon Sep 17 00:00:00 2001
From: Laith Sakka <lsakka@meta.com>
Date: Fri, 10 Nov 2023 15:56:52 -0800
Subject: [PATCH] Fix toMillis should work for all presto timestamps (#7506)

Summary:
Pull Request resolved: https://github.com/facebookincubator/velox/pull/7506

Presto timestamps are in the range TimeStamp::minMillis() to TimeStamp::maxMillis().
moreover minMillis has a comment:
```
// The minimum Timestamp that toMillis() method will not overflow.
// Used to calculate the minimum value of the Presto timestamp.
```
Before this diff  TimeStamp::minMillis().toMillis throws because the computation
overflows. However in that case we multiply negative number then add positive number
so the final result still fits in int64_t.
It is important the toMillis does not throw when it should not, since it is used in
functions like inPredicate() and other functions that do not expect it to throw
for some values. Throwing will show up as incorrect results compared to presto.
This diff fixes that .
I wonder if we shall throw a runtime error in toMillis() instead.

found while investigating https://github.com/facebookincubator/velox/issues/7161

Reviewed By: kagamiori

Differential Revision: D51177715

fbshipit-source-id: 5ec2d4d9c5448f7951a795ffb7a8c5c36fdee15f
---
 .../prestosql/tests/DateTimeFunctionsTest.cpp |  2 +-
 velox/type/Timestamp.h                        | 22 ++++++++++---------
 velox/type/tests/TimestampTest.cpp            |  2 ++
 3 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp b/velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp
index 9d63978c5f81..ced3aeb985be 100644
--- a/velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp
+++ b/velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp
@@ -858,7 +858,7 @@ TEST_F(DateTimeFunctionsTest, minusTimestampIntervalDayTime) {
   EXPECT_EQ(-1000, minus(1, 2));
   VELOX_ASSERT_THROW(
       minus(Timestamp::kMinSeconds, Timestamp::kMaxSeconds),
-      "integer overflow");
+      "Could not convert Timestamp(-9223372036854776, 0) to milliseconds");
 }
 
 TEST_F(DateTimeFunctionsTest, dayOfMonthTimestampWithTimezone) {
diff --git a/velox/type/Timestamp.h b/velox/type/Timestamp.h
index 377fd5b1fff3..46cb32a2a5f3 100644
--- a/velox/type/Timestamp.h
+++ b/velox/type/Timestamp.h
@@ -120,19 +120,21 @@ struct Timestamp {
   }
 
   int64_t toMillis() const {
-    // When an integer overflow occurs in the calculation,
-    // an exception will be thrown.
-    try {
-      return checkedPlus(
-          checkedMultiply(seconds_, (int64_t)1'000),
-          (int64_t)(nanos_ / 1'000'000));
-    } catch (const std::exception& e) {
+    // We use int128_t to make sure the computation does not overflows since
+    // there are cases such that seconds*1000 does not fit in int64_t,
+    // but seconds*1000 + nanos does, an example is TimeStamp::minMillis().
+
+    // If the final result does not fit in int64_tw we throw.
+    __int128_t result =
+        (__int128_t)seconds_ * 1'000 + (int64_t)(nanos_ / 1'000'000);
+    if (result < std::numeric_limits<int64_t>::min() ||
+        result > std::numeric_limits<int64_t>::max()) {
       VELOX_USER_FAIL(
-          "Could not convert Timestamp({}, {}) to milliseconds, {}",
+          "Could not convert Timestamp({}, {}) to milliseconds",
           seconds_,
-          nanos_,
-          e.what());
+          nanos_);
     }
+    return result;
   }
 
   int64_t toMicros() const {
diff --git a/velox/type/tests/TimestampTest.cpp b/velox/type/tests/TimestampTest.cpp
index b78e38cf3234..10a3c3f64343 100644
--- a/velox/type/tests/TimestampTest.cpp
+++ b/velox/type/tests/TimestampTest.cpp
@@ -116,6 +116,8 @@ TEST(TimestampTest, arithmeticOverflow) {
           "Could not convert Timestamp({}, {}) to nanoseconds",
           negativeSecond,
           0));
+  ASSERT_NO_THROW(Timestamp::minMillis().toMillis());
+  ASSERT_NO_THROW(Timestamp::maxMillis().toMillis());
 }
 
 TEST(TimestampTest, toAppend) {