Skip to content

Commit

Permalink
Fix Timestamp::toTimezone for negative timestamps (facebookincubator#…
Browse files Browse the repository at this point in the history
…6788)

Summary:
The value of `seconds_` in `Timepstamp` is the starting point of one
second, so when converting a `time_point` from milliseconds to seconds,
it should always be rounded down.

However, `std::chrono::duration_cast` will always round to 0, so for
negative numbers (times before 1970-01-01 00:00:00), the result will
be wrong(will differ from the correct result 1 second).

Correcting the rounding behavior also affects the unit test of
`date_format`, which is also fixed in this PR.

Fixes facebookincubator#6489 and facebookincubator#6787.

Pull Request resolved: facebookincubator#6788

Reviewed By: zacw7

Differential Revision: D49755789

Pulled By: pedroerp

fbshipit-source-id: 65cb6967c871f1235eded924804635379e11d88d
  • Loading branch information
lingbin authored and facebook-github-bot committed Oct 2, 2023
1 parent 2a5845b commit d090149
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 7 deletions.
7 changes: 4 additions & 3 deletions velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -688,6 +688,7 @@ TEST_F(DateTimeFunctionsTest, hour) {
EXPECT_EQ(std::nullopt, hour(std::nullopt));
EXPECT_EQ(0, hour(Timestamp(0, 0)));
EXPECT_EQ(23, hour(Timestamp(-1, 9000)));
EXPECT_EQ(23, hour(Timestamp(-1, Timestamp::kMaxNanos)));
EXPECT_EQ(7, hour(Timestamp(4000000000, 0)));
EXPECT_EQ(7, hour(Timestamp(4000000000, 123000000)));
EXPECT_EQ(10, hour(Timestamp(998474645, 321000000)));
Expand All @@ -697,7 +698,7 @@ TEST_F(DateTimeFunctionsTest, hour) {

EXPECT_EQ(std::nullopt, hour(std::nullopt));
EXPECT_EQ(13, hour(Timestamp(0, 0)));
EXPECT_EQ(13, hour(Timestamp(-1, Timestamp::kMaxNanos)));
EXPECT_EQ(12, hour(Timestamp(-1, Timestamp::kMaxNanos)));
// Disabled for now because the TZ for Pacific/Apia in 2096 varies between
// systems.
// EXPECT_EQ(21, hour(Timestamp(4000000000, 0)));
Expand Down Expand Up @@ -3061,7 +3062,7 @@ TEST_F(DateTimeFunctionsTest, dateFormat) {
fromTimestampString("2000-02-29 00:00:00.987"),
"%Y-%m-%d %H:%i:%s.%f"));
EXPECT_EQ(
"-2000-02-29 05:53:29.987000",
"-2000-02-29 05:53:28.987000",
dateFormat(
fromTimestampString("-2000-02-29 00:00:00.987"),
"%Y-%m-%d %H:%i:%s.%f"));
Expand All @@ -3082,7 +3083,7 @@ TEST_F(DateTimeFunctionsTest, dateFormat) {
fromTimestampString("2000-02-29 00:00:00.987"),
"%Y-%m-%d %H:%i:%s.%f"));
EXPECT_EQ(
"-2000-02-28 16:07:03.987000",
"-2000-02-28 16:07:02.987000",
dateFormat(
fromTimestampString("-2000-02-29 00:00:00.987"),
"%Y-%m-%d %H:%i:%s.%f"));
Expand Down
8 changes: 4 additions & 4 deletions velox/type/Timestamp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -94,17 +94,17 @@ void validateTimePoint(const std::chrono::time_point<

std::chrono::time_point<std::chrono::system_clock, std::chrono::milliseconds>
Timestamp::toTimePoint() const {
auto tp = std::chrono::
time_point<std::chrono::system_clock, std::chrono::milliseconds>(
std::chrono::milliseconds(toMillis()));
using namespace std::chrono;
auto tp = time_point<system_clock, milliseconds>(milliseconds(toMillis()));
validateTimePoint(tp);
return tp;
}

void Timestamp::toTimezone(const date::time_zone& zone) {
auto tp = toTimePoint();
auto epoch = zone.to_local(tp).time_since_epoch();
seconds_ = std::chrono::duration_cast<std::chrono::seconds>(epoch).count();
// NOTE: Round down to get the seconds of the current time point.
seconds_ = std::chrono::floor<std::chrono::seconds>(epoch).count();
}

void Timestamp::toTimezone(int16_t tzID) {
Expand Down

0 comments on commit d090149

Please sign in to comment.