Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Date_add throws or produces wrong results when the result would be nonexistent time in the time zone #11845

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions velox/functions/prestosql/DateTimeFunctions.h
Original file line number Diff line number Diff line change
Expand Up @@ -1269,8 +1269,13 @@ struct DateAddFunction : public TimestampWithTimezoneSupport<T> {
result = Timestamp(
resultTimestamp.getSeconds() + offset, resultTimestamp.getNanos());
} else {
resultTimestamp.toGMT(*sessionTimeZone_);
result = resultTimestamp;
result = Timestamp(
sessionTimeZone_
->correct_nonexistent_time(
std::chrono::seconds(resultTimestamp.getSeconds()))
.count(),
resultTimestamp.getNanos());
result.toGMT(*sessionTimeZone_);
}
} else {
result = addToTimestamp(timestamp, unit, (int32_t)value);
Expand Down
6 changes: 6 additions & 0 deletions velox/functions/prestosql/DateTimeImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,12 @@ FOLLY_ALWAYS_INLINE int64_t addToTimestampWithTimezone(
.count());
auto updatedTimeStamp =
addToTimestamp(originalTimestamp, unit, (int32_t)value);
updatedTimeStamp = Timestamp(
timeZone
->correct_nonexistent_time(
std::chrono::seconds(updatedTimeStamp.getSeconds()))
.count(),
updatedTimeStamp.getNanos());
finalSysMs =
timeZone
->to_sys(
Expand Down
21 changes: 21 additions & 0 deletions velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2375,6 +2375,16 @@ TEST_F(DateTimeFunctionsTest, dateAddTimestamp) {
"year",
-2,
Timestamp(1582970400, 500'999'999) /*2020-02-29 10:00:00.500*/));

// Test cases where the result would end up in the nonexistent gap between
// daylight savings time and standard time. 2023-03-12 02:30:00.000 does not
// exist in America/Los_Angeles since that hour is skipped.
EXPECT_EQ(
Timestamp(1678617000, 0), /*2023-03-12 03:30:00*/
dateAdd("day", 45, Timestamp(1674729000, 0) /*2023-01-26 02:30:00*/));
EXPECT_EQ(
Timestamp(1678617000, 0), /*2023-03-12 03:30:00*/
dateAdd("day", -45, Timestamp(1682501400, 0) /*2023-04-26 02:30:00*/));
}

TEST_F(DateTimeFunctionsTest, dateAddTimestampWithTimeZone) {
Expand Down Expand Up @@ -2553,6 +2563,17 @@ TEST_F(DateTimeFunctionsTest, dateAddTimestampWithTimeZone) {
"2023-03-11 00:00:00.000 America/Los_Angeles",
dateAddAndCast(
"year", -1, "2024-03-11 00:00:00.000 America/Los_Angeles"));

// Test cases where the result would end up in the nonexistent gap between
// daylight savings time and standard time. 2023-03-12 02:30:00.000 does not
// exist in America/Los_Angeles since that hour is skipped.
EXPECT_EQ(
"2023-03-12 03:30:00.000 America/Los_Angeles",
dateAddAndCast("day", 45, "2023-01-26 02:30:00.000 America/Los_Angeles"));
EXPECT_EQ(
"2023-03-12 03:30:00.000 America/Los_Angeles",
dateAddAndCast(
"day", -45, "2023-04-26 02:30:00.000 America/Los_Angeles"));
}

TEST_F(DateTimeFunctionsTest, dateDiffDate) {
Expand Down
18 changes: 18 additions & 0 deletions velox/type/tz/TimeZoneMap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,24 @@ TimeZone::milliseconds TimeZone::to_local(
return toLocalImpl(timestamp, tz_, offset_);
}

TimeZone::seconds TimeZone::correct_nonexistent_time(
TimeZone::seconds timestamp) const {
// If this is an offset time zone.
if (tz_ == nullptr) {
return timestamp;
}

const auto localInfo = tz_->get_info(date::local_time<seconds>{timestamp});

if (localInfo.result != date::local_info::nonexistent) {
return timestamp;
}

const auto adjustment = localInfo.second.offset - localInfo.first.offset;

return timestamp + adjustment;
}

std::string TimeZone::getShortName(
TimeZone::milliseconds timestamp,
TimeZone::TChoose choose) const {
Expand Down
8 changes: 8 additions & 0 deletions velox/type/tz/TimeZoneMap.h
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,14 @@ class TimeZone {
seconds to_local(seconds timestamp) const;
milliseconds to_local(milliseconds timestamp) const;

/// If a local time is nonexistent, i.e. refers to a time that exists in the
/// gap during a time zone conversion, this returns the time adjusted by
/// the difference between the two time zones, so that it lies in the later
/// time zone.
///
/// If the local time exists then the same time is returned.
seconds correct_nonexistent_time(seconds timestamp) const;

const std::string& name() const {
return timeZoneName_;
}
Expand Down
Loading