Fix comment

facebookincubator · Apr 24, 2024 · 3988715 · 3988715
1 parent ef9f0c9
commit 3988715
Show file tree

Hide file tree

Showing 5 changed files with 33 additions and 20 deletions.
diff --git a/velox/expression/CastExpr-inl.h b/velox/expression/CastExpr-inl.h
@@ -733,8 +733,7 @@ void CastExpr::applyCastPrimitives(
   // current GMT timezone to the user provided session timezone.
   if constexpr (ToKind == TypeKind::TIMESTAMP) {
     const auto& queryConfig = context.execCtx()->queryCtx()->queryConfig();
-    auto* timeZone =
-        facebook::velox::functions::getTimeZoneFromConfig(queryConfig);
+    const auto* timeZone = functions::getTimeZoneFromConfig(queryConfig);
     if (timeZone) {
       // When context.throwOnError is false, some rows will be marked as
       // 'failed'. These rows should not be processed further. 'remainingRows'

diff --git a/velox/expression/CastExpr.cpp b/velox/expression/CastExpr.cpp
@@ -182,8 +182,7 @@ VectorPtr CastExpr::castFromDate(
     case TypeKind::TIMESTAMP: {
       static const int64_t kMillisPerDay{86'400'000};
       const auto& queryConfig = context.execCtx()->queryCtx()->queryConfig();
-      const auto* timeZone =
-          facebook::velox::functions::getTimeZoneFromConfig(queryConfig);
+      const auto* timeZone = functions::getTimeZoneFromConfig(queryConfig);
       auto* resultFlatVector = castResult->as<FlatVector<Timestamp>>();
       applyToSelectedNoThrowLocal(context, rows, castResult, [&](int row) {
         auto timestamp = Timestamp::fromMillis(
@@ -231,8 +230,7 @@ VectorPtr CastExpr::castToDate(
     }
     case TypeKind::TIMESTAMP: {
       const auto& queryConfig = context.execCtx()->queryCtx()->queryConfig();
-      auto* timeZone =
-          facebook::velox::functions::getTimeZoneFromConfig(queryConfig);
+      const auto* timeZone = functions::getTimeZoneFromConfig(queryConfig);
       if (timeZone) {
         castTimestampToDate<true>(rows, input, context, castResult, timeZone);
       } else {

diff --git a/velox/expression/PrestoCastHooks.cpp b/velox/expression/PrestoCastHooks.cpp
@@ -16,6 +16,7 @@
 
 #include "velox/expression/PrestoCastHooks.h"
 #include "velox/external/date/tz.h"
+#include "velox/functions/lib/TimeUtils.h"
 #include "velox/type/TimestampConversion.h"
 
 namespace facebook::velox::exec {
@@ -25,10 +26,7 @@ PrestoCastHooks::PrestoCastHooks(const core::QueryConfig& config)
   if (!legacyCast_) {
     options_.zeroPaddingYear = true;
     options_.dateTimeSeparator = ' ';
-    const auto sessionTzName = config.sessionTimezone();
-    if (config.adjustTimestampToTimezone() && !sessionTzName.empty()) {
-      options_.timeZone = date::locate_zone(sessionTzName);
-    }
+    options_.timeZone = functions::getTimeZoneFromConfig(config);
   }
 }
 

diff --git a/velox/functions/lib/TimeUtils.h b/velox/functions/lib/TimeUtils.h
@@ -26,23 +26,22 @@ inline constexpr int64_t kSecondsInDay = 86'400;
 inline constexpr int64_t kDaysInWeek = 7;
 extern const folly::F14FastMap<std::string, int8_t> kDayOfWeekNames;
 
-// Format timezone to make it compatible with date::locate_zone.
-// For example, converts "GMT+8" to "Etc/GMT-8".
-// Here is the list of IANA timezone names:
-// https://en.wikipedia.org/wiki/List_of_tz_database_time_zones
+/// Format timezone to make it compatible with date::locate_zone.
+/// For example, converts "GMT+8" to "Etc/GMT-8".
+/// Here is the list of IANA timezone names:
+/// https://en.wikipedia.org/wiki/List_of_tz_database_time_zones
 FOLLY_ALWAYS_INLINE std::string formatTimezone(const std::string& timezone) {
   if (timezone.find("GMT") == 0 && timezone.size() > 4) {
-    if (timezone[3] != '+' && timezone[3] != '-') {
-      return timezone;
+    if (timezone[3] == '+') {
+      return "Etc/GMT-" + timezone.substr(4);
+    }
+    if (timezone[3] == '-') {
+      return "Etc/GMT+" + timezone.substr(4);
     }
-    std::string prefix = timezone[3] == '+' ? "Etc/GMT-" : "Etc/GMT+";
-    return prefix + timezone.substr(4);
   }
   return timezone;
 }
 
-// If user doesn't explicitly ask us to adjust the timezone or configured
-// session timezone is empty, returns nullptr.
 FOLLY_ALWAYS_INLINE const date::time_zone* getTimeZoneFromConfig(
     const core::QueryConfig& config) {
   if (config.adjustTimestampToTimezone()) {

diff --git a/velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp b/velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp
@@ -3341,6 +3341,25 @@ TEST_F(DateTimeFunctionsTest, dateFormat) {
           fromTimestampString("-2000-02-29 00:00:00.987"),
           "%Y-%m-%d %H:%i:%s.%f"));
 
+  setQueryTimeZone("GMT-8");
+
+  EXPECT_EQ(
+      "1969-12-31", dateFormat(fromTimestampString("1970-01-01"), "%Y-%m-%d"));
+  EXPECT_EQ(
+      "2000-02-28 04:00:00 PM",
+      dateFormat(
+          fromTimestampString("2000-02-29 00:00:00.987"), "%Y-%m-%d %r"));
+  EXPECT_EQ(
+      "2000-02-28 16:00:00.987000",
+      dateFormat(
+          fromTimestampString("2000-02-29 00:00:00.987"),
+          "%Y-%m-%d %H:%i:%s.%f"));
+  EXPECT_EQ(
+      "-2000-02-28 16:00:00.987000",
+      dateFormat(
+          fromTimestampString("-2000-02-29 00:00:00.987"),
+          "%Y-%m-%d %H:%i:%s.%f"));
+
   // User format errors or unsupported errors
   EXPECT_THROW(
       dateFormat(fromTimestampString("-2000-02-29 00:00:00.987"), "%D"),