From cf5aa68b97c83d18cd436f26fdaa40f815b60117 Mon Sep 17 00:00:00 2001 From: Zac Wen Date: Fri, 26 Jul 2024 01:29:17 -0700 Subject: [PATCH] Fix timezone handling in to_iso8601(timestamp) (#10576) Summary: 1. Current adjustment changes the actual unix time which is incorrect. Based on the [Presto implementation](https://github.com/prestodb/presto/blob/6a9f3cd9ef9f25855af9c4c6eb54d8612621eb84/presto-main/src/main/java/com/facebook/presto/operator/scalar/DateTimeFunctions.java#L255-L267), only the timestamp representation should be adjusted with the specified timezone. 2. Adjustment is needed only when `adjustTimestampToTimezone` is true. Pull Request resolved: https://github.com/facebookincubator/velox/pull/10576 Reviewed By: amitkdutta, gggrace14 Differential Revision: D60270091 Pulled By: zacw7 fbshipit-source-id: aa113300fb69598fc2fcbde34a2b1eda6716042d --- velox/functions/prestosql/DateTimeFunctions.h | 15 ++--------- .../prestosql/tests/DateTimeFunctionsTest.cpp | 26 ++++++++++++------- 2 files changed, 19 insertions(+), 22 deletions(-) diff --git a/velox/functions/prestosql/DateTimeFunctions.h b/velox/functions/prestosql/DateTimeFunctions.h index 9f5acea5368f..4cd881817bfa 100644 --- a/velox/functions/prestosql/DateTimeFunctions.h +++ b/velox/functions/prestosql/DateTimeFunctions.h @@ -1726,12 +1726,7 @@ struct ToISO8601Function { const core::QueryConfig& config, const arg_type* /*input*/) { if (inputTypes[0]->isTimestamp()) { - auto sessionTzName = config.sessionTimezone(); - if (!sessionTzName.empty()) { - timeZone_ = tz::locateZone(sessionTzName); - } else { - timeZone_ = tz::locateZone("UTC"); - } + timeZone_ = getTimeZoneFromConfig(config); } } @@ -1744,13 +1739,7 @@ struct ToISO8601Function { FOLLY_ALWAYS_INLINE void call( out_type& result, const arg_type& timestamp) { - // TODO DateTimeFormatter requires timestamp in UTC. It then converts it to - // the specified timezone. We can avoid extra conversion if we change - // DateTimeFormatter to accept non-UTC timestamps. - auto utcTimestamp = timestamp; - utcTimestamp.toGMT(*timeZone_); - - toIso8601(utcTimestamp, timeZone_, result); + toIso8601(timestamp, timeZone_, result); } FOLLY_ALWAYS_INLINE void call( diff --git a/velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp b/velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp index 16cac073fbd6..0a87e93549c8 100644 --- a/velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp +++ b/velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp @@ -4426,19 +4426,27 @@ TEST_F(DateTimeFunctionsTest, toISO8601Timestamp) { return evaluateOnce( "to_iso8601(c0)", std::make_optional(parseTimestamp(timestamp))); }; + disableAdjustTimestampToTimezone(); + EXPECT_EQ("2024-11-01T10:00:00.000+00:00", toIso("2024-11-01 10:00")); + EXPECT_EQ("2024-11-04T10:00:00.000+00:00", toIso("2024-11-04 10:00")); + EXPECT_EQ("2024-11-04T15:05:34.100+00:00", toIso("2024-11-04 15:05:34.1")); + EXPECT_EQ("2024-11-04T15:05:34.123+00:00", toIso("2024-11-04 15:05:34.123")); + EXPECT_EQ("0022-11-01T10:00:00.000+00:00", toIso("22-11-01 10:00")); - setQueryTimeZone("America/New_York"); + setQueryTimeZone("America/Los_Angeles"); + EXPECT_EQ("2024-11-01T03:00:00.000-07:00", toIso("2024-11-01 10:00")); - EXPECT_EQ("2024-11-01T10:00:00.000-04:00", toIso("2024-11-01 10:00")); - EXPECT_EQ("2024-11-04T10:00:00.000-05:00", toIso("2024-11-04 10:00")); - EXPECT_EQ("2024-11-04T15:05:34.100-05:00", toIso("2024-11-04 15:05:34.1")); - EXPECT_EQ("2024-11-04T15:05:34.123-05:00", toIso("2024-11-04 15:05:34.123")); - EXPECT_EQ("0022-11-01T10:00:00.000-04:56:02", toIso("22-11-01 10:00")); + setQueryTimeZone("America/New_York"); + EXPECT_EQ("2024-11-01T06:00:00.000-04:00", toIso("2024-11-01 10:00")); + EXPECT_EQ("2024-11-04T05:00:00.000-05:00", toIso("2024-11-04 10:00")); + EXPECT_EQ("2024-11-04T10:05:34.100-05:00", toIso("2024-11-04 15:05:34.1")); + EXPECT_EQ("2024-11-04T10:05:34.123-05:00", toIso("2024-11-04 15:05:34.123")); + EXPECT_EQ("0022-11-01T05:03:58.000-04:56:02", toIso("22-11-01 10:00")); setQueryTimeZone("Asia/Kathmandu"); - - EXPECT_EQ("2024-11-01T10:00:00.000+05:45", toIso("2024-11-01 10:00")); - EXPECT_EQ("0022-11-01T10:00:00.000+05:41:16", toIso("22-11-01 10:00")); + EXPECT_EQ("2024-11-01T15:45:00.000+05:45", toIso("2024-11-01 10:00")); + EXPECT_EQ("0022-11-01T15:41:16.000+05:41:16", toIso("22-11-01 10:00")); + EXPECT_EQ("0022-11-01T15:41:16.000+05:41:16", toIso("22-11-01 10:00")); } TEST_F(DateTimeFunctionsTest, toISO8601TimestampWithTimezone) {