From 1df8bf1e08a134d868bdff309e0cc421c65ebbd9 Mon Sep 17 00:00:00 2001
From: "joey.ljy" <joey.ljy@alibaba-inc.com>
Date: Sat, 23 Mar 2024 17:59:29 +0800
Subject: [PATCH] Fix negative nano

---
 velox/dwio/parquet/reader/PageReader.cpp | 15 ++++-----------
 velox/type/Timestamp.h                   | 13 ++++++++++++-
 velox/type/tests/TimestampTest.cpp       | 24 ++++++++++++++++++++++++
 3 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/velox/dwio/parquet/reader/PageReader.cpp b/velox/dwio/parquet/reader/PageReader.cpp
index 18381f7086097..b2e4791a6e085 100644
--- a/velox/dwio/parquet/reader/PageReader.cpp
+++ b/velox/dwio/parquet/reader/PageReader.cpp
@@ -388,24 +388,17 @@ void PageReader::prepareDictionary(const PageHeader& pageHeader) {
       for (auto i = dictionary_.numValues - 1; i >= 0; --i) {
         // Convert the timestamp into seconds and nanos since the Unix epoch,
         // 00:00:00.000000 on 1 January 1970.
-        uint64_t nanos;
+        int64_t nanos;
         memcpy(
             &nanos,
             parquetValues + i * sizeof(Int96Timestamp),
-            sizeof(uint64_t));
+            sizeof(int64_t));
         int32_t days;
         memcpy(
             &days,
-            parquetValues + i * sizeof(Int96Timestamp) + sizeof(uint64_t),
+            parquetValues + i * sizeof(Int96Timestamp) + sizeof(int64_t),
             sizeof(int32_t));
-        int64_t seconds = (days - Timestamp::kJulianToUnixEpochDays) *
-            Timestamp::kSecondsPerDay;
-        if (nanos > Timestamp::kMaxNanos) {
-          seconds += nanos / Timestamp::kNanosInSecond;
-          nanos -=
-              (nanos / Timestamp::kNanosInSecond) * Timestamp::kNanosInSecond;
-        }
-        values[i] = Timestamp(seconds, nanos);
+        values[i] = Timestamp::fromDaysAndNanos(days, nanos);
       }
       break;
     }
diff --git a/velox/type/Timestamp.h b/velox/type/Timestamp.h
index 88f0526f6444b..0504fb69dd6fb 100644
--- a/velox/type/Timestamp.h
+++ b/velox/type/Timestamp.h
@@ -84,7 +84,7 @@ struct Timestamp {
   static constexpr int64_t kNanosInSecond =
       kNanosecondsInMillisecond * kMillisecondsInSecond;
   static constexpr int64_t kJulianToUnixEpochDays = 2440588LL;
-  static constexpr int64_t kSecondsPerDay = 86400LL;
+  static constexpr int64_t kSecondsInDay = 86400LL;
 
   // Limit the range of seconds to avoid some problems. Seconds should be
   // in the range [INT64_MIN/1000 - 1, INT64_MAX/1000].
@@ -110,6 +110,17 @@ struct Timestamp {
     VELOX_USER_DCHECK_LE(nanos, kMaxNanos, "Timestamp nanos out of range");
   }
 
+  static Timestamp fromDaysAndNanos(int32_t days, int64_t nanos) {
+    int64_t seconds = (days - kJulianToUnixEpochDays) * kSecondsInDay +
+        nanos / kNanosInSecond;
+    int64_t remainingNanos = nanos % kNanosInSecond;
+    if (remainingNanos < 0) {
+      remainingNanos += kNanosInSecond;
+      seconds--;
+    }
+    return Timestamp(seconds, remainingNanos);
+  }
+
   // Returns the current unix timestamp (ms precision).
   static Timestamp now();
 
diff --git a/velox/type/tests/TimestampTest.cpp b/velox/type/tests/TimestampTest.cpp
index 07d338116ae50..bcdaa3a1d878b 100644
--- a/velox/type/tests/TimestampTest.cpp
+++ b/velox/type/tests/TimestampTest.cpp
@@ -36,6 +36,30 @@ std::string timestampToString(
   return result;
 }
 
+TEST(TimestampTest, fromDaysAndNanos) {
+  EXPECT_EQ(
+      Timestamp(Timestamp::kSecondsInDay + 2, 1),
+      Timestamp::fromDaysAndNanos(
+          Timestamp::kJulianToUnixEpochDays + 1,
+          2 * Timestamp::kNanosInSecond + 1));
+  EXPECT_EQ(
+      Timestamp(Timestamp::kSecondsInDay + 2, 0),
+      Timestamp::fromDaysAndNanos(
+          Timestamp::kJulianToUnixEpochDays + 1,
+          2 * Timestamp::kNanosInSecond));
+  EXPECT_EQ(
+      Timestamp(
+          Timestamp::kSecondsInDay * 5 - 3, Timestamp::kNanosInSecond - 6),
+      Timestamp::fromDaysAndNanos(
+          Timestamp::kJulianToUnixEpochDays + 5,
+          -2 * Timestamp::kNanosInSecond - 6));
+  EXPECT_EQ(
+      Timestamp(Timestamp::kSecondsInDay * 5 - 2, 0),
+      Timestamp::fromDaysAndNanos(
+          Timestamp::kJulianToUnixEpochDays + 5,
+          -2 * Timestamp::kNanosInSecond));
+}
+
 TEST(TimestampTest, fromMillisAndMicros) {
   int64_t positiveSecond = 10'000;
   int64_t negativeSecond = -10'000;