Skip to content

Commit

Permalink
Add check for signed integer overflow in diffTimestamp (facebookincub…
Browse files Browse the repository at this point in the history
…ator#10675)

Summary:
Pull Request resolved: facebookincubator#10675

Handle unsinged integer overflow error and rethrow with more informative error message.

Reviewed By: kevinwilfong

Differential Revision: D60876240

fbshipit-source-id: 5d096db6162fb91873c798396afde109e26e7a61
  • Loading branch information
Daniel Hunte authored and facebook-github-bot committed Aug 9, 2024
1 parent c6c67f1 commit 1c538f2
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 4 deletions.
23 changes: 19 additions & 4 deletions velox/functions/prestosql/DateTimeImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -282,10 +282,25 @@ FOLLY_ALWAYS_INLINE int64_t diffTimestamp(
std::chrono::duration_cast<std::chrono::milliseconds>(
fromTimepoint - fromDaysTimepoint)
.count();
const uint64_t toTimeInstantOfDay =
std::chrono::duration_cast<std::chrono::milliseconds>(
toTimepoint - toDaysTimepoint)
.count();

uint64_t toTimeInstantOfDay = 0;
uint64_t toTimePointMillis = toTimepoint.time_since_epoch().count();
uint64_t toDaysTimepointMillis =
std::chrono::
time_point<std::chrono::system_clock, std::chrono::milliseconds>(
toDaysTimepoint)
.time_since_epoch()
.count();
bool overflow = __builtin_sub_overflow(
toTimePointMillis, toDaysTimepointMillis, &toTimeInstantOfDay);
VELOX_USER_CHECK_EQ(
overflow,
false,
"{} - {} Causes arithmetic overflow: {} - {}",
fromTimestamp.toString(),
toTimestamp.toString(),
toTimePointMillis,
toDaysTimepointMillis)
const uint8_t fromDay = static_cast<unsigned>(fromCalDate.day()),
fromMonth = static_cast<unsigned>(fromCalDate.month());
const uint8_t toDay = static_cast<unsigned>(toCalDate.day()),
Expand Down
17 changes: 17 additions & 0 deletions velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2558,6 +2558,23 @@ TEST_F(DateTimeFunctionsTest, dateDiffTimestamp) {
dateDiff("invalid_unit", Timestamp(1, 0), Timestamp(0, 0)),
"Unsupported datetime unit: invalid_unit");

// Check for integer overflow when result unit is month or larger.
EXPECT_EQ(
-622191233,
dateDiff("day", Timestamp(0, 0), Timestamp(9223372036854775, 0)));
EXPECT_EQ(
-88884461,
dateDiff("week", Timestamp(0, 0), Timestamp(9223372036854775, 0)));
VELOX_ASSERT_THROW(
dateDiff("month", Timestamp(0, 0), Timestamp(9223372036854775, 0)),
"Causes arithmetic overflow:");
VELOX_ASSERT_THROW(
dateDiff("quarter", Timestamp(0, 0), Timestamp(9223372036854775, 0)),
"Causes arithmetic overflow:");
VELOX_ASSERT_THROW(
dateDiff("year", Timestamp(0, 0), Timestamp(9223372036854775, 0)),
"Causes arithmetic overflow:");

// Simple tests
EXPECT_EQ(
60 * 1000 + 500,
Expand Down

0 comments on commit 1c538f2

Please sign in to comment.