Skip to content

Commit

Permalink
Fix toMicros() should work for all spark timestamps
Browse files Browse the repository at this point in the history
  • Loading branch information
boneanxs committed Dec 6, 2024
1 parent 3ead2f4 commit 09a1ee4
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 13 deletions.
26 changes: 13 additions & 13 deletions velox/type/Timestamp.h
Original file line number Diff line number Diff line change
Expand Up @@ -160,11 +160,10 @@ struct Timestamp {
// there are cases such that seconds*1000 does not fit in int64_t,
// but seconds*1000 + nanos does, an example is TimeStamp::minMillis().

// If the final result does not fit in int64_tw we throw.
// If the final result does not fit in int64_t we throw.
__int128_t result =
(__int128_t)seconds_ * 1'000 + (int64_t)(nanos_ / 1'000'000);
if (result < std::numeric_limits<int64_t>::min() ||
result > std::numeric_limits<int64_t>::max()) {
if (result < INT64_MIN || result > INT64_MAX) {
VELOX_USER_FAIL(
"Could not convert Timestamp({}, {}) to milliseconds",
seconds_,
Expand All @@ -183,19 +182,20 @@ struct Timestamp {

// Keep it in header for getting inlined.
int64_t toMicros() const {
// When an integer overflow occurs in the calculation,
// an exception will be thrown.
try {
return checkedPlus(
checkedMultiply(seconds_, (int64_t)1'000'000),
(int64_t)(nanos_ / 1'000));
} catch (const std::exception& e) {
// We use int128_t to make sure the computation does not overflows since
// there are cases such that seconds*1000000 does not fit in int64_t,
// but seconds*1000000 + nanos does, an example is TimeStamp::minMillis().

// If the final result does not fit in int64_t we throw.
__int128_t result =
(__int128_t)seconds_ * 1'000'000 + (int64_t)(nanos_ / 1'000);
if (result < INT64_MIN || result > INT64_MAX) {
VELOX_USER_FAIL(
"Could not convert Timestamp({}, {}) to microseconds, {}",
"Could not convert Timestamp({}, {}) to microseconds",
seconds_,
nanos_,
e.what());
nanos_);
}
return result;
}

/// Exports the current timestamp as a std::chrono::time_point of millisecond
Expand Down
2 changes: 2 additions & 0 deletions velox/type/tests/TimestampTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,8 @@ TEST(TimestampTest, arithmeticOverflow) {
0));
ASSERT_NO_THROW(Timestamp::minMillis().toMillis());
ASSERT_NO_THROW(Timestamp::maxMillis().toMillis());
ASSERT_NO_THROW(Timestamp::minMillis().toMicros());
ASSERT_NO_THROW(Timestamp::maxMillis().toMicros());
}

TEST(TimestampTest, toAppend) {
Expand Down

0 comments on commit 09a1ee4

Please sign in to comment.