Add support ANSI cast float/double to timestamp#17219
Add support ANSI cast float/double to timestamp#17219Harthi7 wants to merge 5 commits intofacebookincubator:mainfrom
Conversation
✅ Deploy Preview for meta-velox canceled.
|
Build Impact AnalysisSelective Build Targets (building these covers all 21 affected)Total affected: 21/565 targets Affected targets (21)Directly changed (2)
Transitively affected (19)
Fast path • Graph from main@3a86cb261b0b0dfdfef527e679b93a7f1d87999d |
philo-he
left a comment
There was a problem hiding this comment.
Looks good overall. Some comments. Please check if they make sense. Thanks.
| const long double micros = | ||
| static_cast<long double>(value) * Timestamp::kMicrosecondsInSecond; | ||
|
|
||
| if (micros > static_cast<long double>(std::numeric_limits<int64_t>::max()) || |
There was a problem hiding this comment.
Suggest creating a static constexpr variable to hold static_cast(std::numeric_limits<int64_t>::max())
There was a problem hiding this comment.
Thanks. The first two suggestions make sense. I’ll factor out the int64 bounds into local constexpr variables to simplify the overflow check.
| static_cast<long double>(value) * Timestamp::kMicrosecondsInSecond; | ||
|
|
||
| if (micros > static_cast<long double>(std::numeric_limits<int64_t>::max()) || | ||
| micros < static_cast<long double>(std::numeric_limits<int64_t>::min())) { |
| if (micros > static_cast<long double>(std::numeric_limits<int64_t>::max()) || | ||
| micros < static_cast<long double>(std::numeric_limits<int64_t>::min())) { | ||
| return folly::makeUnexpected(Status::UserError( | ||
| "Cannot cast floating-point value to TIMESTAMP because the result overflows.")); |
There was a problem hiding this comment.
Could you confirm Spark's error message? It would be better to make them consistent.
There was a problem hiding this comment.
Confirmed. Spark’s ANSI float/double -> timestamp path uses doubleToTimestampAnsi(...), which throws invalidInputInCastToDatetimeError(...) for NaN/Infinity, and finite overflow goes through exact numeric conversion. So Spark’s behavior maps to the generic CAST_INVALID_INPUT and CAST_OVERFLOW error styles rather than a timestamp specific custom message.
-
ANSI implementation:
https://apache.googlesource.com/spark/%2B/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala?autodive=0%2F%2F%2F
In this Velox hook we only have the converted double value, not the full source expression/type context needed to reproduce Spark’s fully rendered message exactly. So, I’m keeping the logic as is and updating the wording to be closer to Spark’s generic error style.
…arthi7/velox into cast-float-double-to-timestamp
| "The value cannot be cast to TIMESTAMP due to an overflow.")); | ||
| } | ||
|
|
||
| return Timestamp::fromMicrosNoError(static_cast<int64_t>(micros)); |
There was a problem hiding this comment.
Since castNumberToTimestamp is also used by castIntToTimestamp, could we extend it with a field (e.g., allowOverflow) so that a single implementation supports both floating-point and integral types on ANSI ON & OFF?
Implemented ANSI support for cast float/double to timestamp.
Changes made:
Validation: