Skip to content

Commit

Permalink
Fix timezone handling in to_iso8601(timestamp) (facebookincubator#10576)
Browse files Browse the repository at this point in the history
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: facebookincubator#10576

Reviewed By: amitkdutta, gggrace14

Differential Revision: D60270091

Pulled By: zacw7

fbshipit-source-id: aa113300fb69598fc2fcbde34a2b1eda6716042d
  • Loading branch information
zacw7 authored and facebook-github-bot committed Jul 26, 2024
1 parent 75020ec commit cf5aa68
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 22 deletions.
15 changes: 2 additions & 13 deletions velox/functions/prestosql/DateTimeFunctions.h
Original file line number Diff line number Diff line change
Expand Up @@ -1726,12 +1726,7 @@ struct ToISO8601Function {
const core::QueryConfig& config,
const arg_type<Timestamp>* /*input*/) {
if (inputTypes[0]->isTimestamp()) {
auto sessionTzName = config.sessionTimezone();
if (!sessionTzName.empty()) {
timeZone_ = tz::locateZone(sessionTzName);
} else {
timeZone_ = tz::locateZone("UTC");
}
timeZone_ = getTimeZoneFromConfig(config);
}
}

Expand All @@ -1744,13 +1739,7 @@ struct ToISO8601Function {
FOLLY_ALWAYS_INLINE void call(
out_type<Varchar>& result,
const arg_type<Timestamp>& 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(
Expand Down
26 changes: 17 additions & 9 deletions velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4426,19 +4426,27 @@ TEST_F(DateTimeFunctionsTest, toISO8601Timestamp) {
return evaluateOnce<std::string>(
"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) {
Expand Down

0 comments on commit cf5aa68

Please sign in to comment.