Address comments

facebookincubator · Oct 10, 2024 · 18ce96d · 18ce96d
1 parent 7927f61
commit 18ce96d
Show file tree

Hide file tree

Showing 6 changed files with 14 additions and 8 deletions.
diff --git a/velox/expression/CastHooks.h b/velox/expression/CastHooks.h
@@ -35,7 +35,7 @@ class CastHooks {
   virtual Expected<Timestamp> castStringToTimestamp(
       const StringView& view) const = 0;
 
-  virtual Expected<Timestamp> castIntToTimestamp(int64_t value) const = 0;
+  virtual Expected<Timestamp> castIntToTimestamp(int64_t seconds) const = 0;
 
   virtual Expected<int32_t> castStringToDate(
       const StringView& dateString) const = 0;

diff --git a/velox/expression/PrestoCastHooks.cpp b/velox/expression/PrestoCastHooks.cpp
@@ -67,7 +67,7 @@ Expected<Timestamp> PrestoCastHooks::castStringToTimestamp(
   return result.first;
 }
 
-Expected<Timestamp> PrestoCastHooks::castIntToTimestamp(int64_t value) const {
+Expected<Timestamp> PrestoCastHooks::castIntToTimestamp(int64_t seconds) const {
   return folly::makeUnexpected(
       Status::UserError("Conversion to Timestamp is not supported"));
 }

diff --git a/velox/expression/PrestoCastHooks.h b/velox/expression/PrestoCastHooks.h
@@ -30,7 +30,7 @@ class PrestoCastHooks : public CastHooks {
   Expected<Timestamp> castStringToTimestamp(
       const StringView& view) const override;
 
-  Expected<Timestamp> castIntToTimestamp(int64_t value) const override;
+  Expected<Timestamp> castIntToTimestamp(int64_t seconds) const override;
 
   // Uses standard cast mode to cast from string to date.
   Expected<int32_t> castStringToDate(

diff --git a/velox/functions/sparksql/specialforms/SparkCastHooks.cpp b/velox/functions/sparksql/specialforms/SparkCastHooks.cpp
@@ -28,10 +28,14 @@ Expected<Timestamp> SparkCastHooks::castStringToTimestamp(
 
 Expected<Timestamp> SparkCastHooks::castIntToTimestamp(int64_t seconds) const {
   // Spark internally use microsecond precision for timestamp.
-  static constexpr int64_t maxSeconds = 9223372036854L;
+  // To avoid overflow, we need to check the range of seconds.
+  static constexpr int64_t maxSeconds = std::numeric_limits<int64_t>::max() /
+      (Timestamp::kMicrosecondsInMillisecond *
+       Timestamp::kMillisecondsInSecond);
   if (seconds > maxSeconds) {
     return Timestamp::fromMicros(std::numeric_limits<int64_t>::max());
-  } else if (seconds < -maxSeconds) {
+  }
+  if (seconds < -maxSeconds) {
     return Timestamp::fromMicros(std::numeric_limits<int64_t>::min());
   }
   return Timestamp(seconds, 0);

diff --git a/velox/functions/sparksql/specialforms/SparkCastHooks.h b/velox/functions/sparksql/specialforms/SparkCastHooks.h
@@ -27,7 +27,9 @@ class SparkCastHooks : public exec::CastHooks {
   Expected<Timestamp> castStringToTimestamp(
       const StringView& view) const override;
 
-  Expected<Timestamp> castIntToTimestamp(int64_t value) const override;
+  // When casting integral value as timestamp, the input is treated as the
+  // number of seconds since the epoch (1970-01-01 00:00:00 UTC).
+  Expected<Timestamp> castIntToTimestamp(int64_t seconds) const override;
 
   /// 1) Removes all leading and trailing UTF8 white-spaces before cast. 2) Uses
   /// non-standard cast mode to cast from string to date.

diff --git a/velox/functions/sparksql/tests/SparkCastExprTest.cpp b/velox/functions/sparksql/tests/SparkCastExprTest.cpp
@@ -264,7 +264,7 @@ TEST_F(SparkCastExprTest, stringToTimestamp) {
 }
 
 TEST_F(SparkCastExprTest, intToTimestamp) {
-  // int64_t
+  // Cast bigint as timestamp.
   testCast(
       makeNullableFlatVector<int64_t>({
           0,
@@ -285,7 +285,7 @@ TEST_F(SparkCastExprTest, intToTimestamp) {
           Timestamp(-9223372036855, 224'192'000),
       }));
 
-  // Ensure that other integral types cast to int64 work as well.
+  // Cast tinyint/smallint/integer as timestamp.
   testIntegralToTimestampCast<int8_t>();
   testIntegralToTimestampCast<int16_t>();
   testIntegralToTimestampCast<int32_t>();