Address comments

facebookincubator · Oct 11, 2024 · b3d55fe · b3d55fe
1 parent a825dd8
commit b3d55fe
Show file tree

Hide file tree

Showing 2 changed files with 20 additions and 14 deletions.
diff --git a/velox/functions/sparksql/DateTimeFunctions.h b/velox/functions/sparksql/DateTimeFunctions.h
@@ -235,16 +235,15 @@ struct UnixTimestampParseWithFormatFunction
   FOLLY_ALWAYS_INLINE void call(
       int64_t& result,
       const arg_type<Timestamp>& input,
-      const arg_type<Varchar>& format) {
+      const arg_type<Varchar>& /*format*/) {
     result = input.getSeconds();
   }
 
   FOLLY_ALWAYS_INLINE void call(
       int64_t& result,
       const arg_type<Date>& input,
-      const arg_type<Varchar>& format) {
-    auto seconds = input * kSecondsInDay;
-    Timestamp timestamp{seconds, 0};
+      const arg_type<Varchar>& /*format*/) {
+    auto timestamp = Timestamp::fromDate(input);
     timestamp.toGMT(*this->sessionTimeZone_);
     result = timestamp.getSeconds();
   }

diff --git a/velox/functions/sparksql/tests/DateTimeFunctionsTest.cpp b/velox/functions/sparksql/tests/DateTimeFunctionsTest.cpp
@@ -301,25 +301,32 @@ TEST_F(DateTimeFunctionsTest, unixTimestampCustomFormat) {
   EXPECT_EQ(
       std::nullopt,
       unixTimestamp("2022-12-12 asd 07:45:31", "yyyy-MM-dd 'asd HH:mm:ss"));
+}
 
-  const auto unixTimestamp1 = [&](std::optional<Timestamp> timestamp,
-                                  std::optional<StringView> formatStr) {
+TEST_F(DateTimeFunctionsTest, unixTimestampTimestampFormat) {
+  const auto unixTimestamp = [&](std::optional<Timestamp> timestamp,
+                                 std::optional<StringView> formatStr) {
     return evaluateOnce<int64_t>(
         "unix_timestamp(c0, c1)", timestamp, formatStr);
   };
-  EXPECT_EQ(0, unixTimestamp1(Timestamp(0, 0), "yyyy-MM-dd"));
-  EXPECT_EQ(1, unixTimestamp1(Timestamp(1, 990), "yyyy-MM-dd"));
-  EXPECT_EQ(61, unixTimestamp1(Timestamp(61, 0), "yyyy-MM-dd"));
+  EXPECT_EQ(0, unixTimestamp(Timestamp(0, 0), "yyyy-MM-dd"));
+  EXPECT_EQ(1, unixTimestamp(Timestamp(1, 990), "yyyy-MM-dd"));
+  EXPECT_EQ(61, unixTimestamp(Timestamp(61, 0), "yyyy-MM-dd"));
+}
 
-  const auto unixTimestamp2 = [&](std::optional<int32_t> date,
-                                  std::optional<StringView> formatStr) {
+TEST_F(DateTimeFunctionsTest, unixTimestampDateFormat) {
+  const auto unixTimestamp = [&](std::optional<int32_t> date,
+                                 std::optional<StringView> formatStr) {
     return evaluateOnce<int64_t>(
         "unix_timestamp(c0, c1)", {DATE(), VARCHAR()}, date, formatStr);
   };
-  EXPECT_EQ(0, unixTimestamp2(parseDate("1970-01-01"), "yyyy-MM-dd"));
-  EXPECT_EQ(1727740800, unixTimestamp2(parseDate("2024-10-01"), "yyyy-MM-dd"));
+  EXPECT_EQ(0, unixTimestamp(parseDate("1970-01-01"), "yyyy-MM-dd"));
+  EXPECT_EQ(1727740800, unixTimestamp(parseDate("2024-10-01"), "yyyy-MM-dd"));
+  VELOX_ASSERT_THROW(
+      unixTimestamp(kMax, "yyyy-MM-dd"),
+      "Timepoint is outside of supported year range");
   setQueryTimeZone("America/Los_Angeles");
-  EXPECT_EQ(1727766000, unixTimestamp2(parseDate("2024-10-01"), "yyyy-MM-dd"));
+  EXPECT_EQ(1727766000, unixTimestamp(parseDate("2024-10-01"), "yyyy-MM-dd"));
 }
 
 // unix_timestamp and to_unix_timestamp are aliases.