feat(function): Add Spark CAST string to TIME with ANSI mode support#16123
Open
malinjawi wants to merge 9 commits intofacebookincubator:mainfrom
Open
feat(function): Add Spark CAST string to TIME with ANSI mode support#16123malinjawi wants to merge 9 commits intofacebookincubator:mainfrom
malinjawi wants to merge 9 commits intofacebookincubator:mainfrom
Conversation
malinjawi
requested review from
jinchengchenghh,
rui-mo and
zhli1142015
as code owners
✅ Deploy Preview for meta-velox canceled.
|
meta-cla
bot
added
the
CLA Signed
rui-mo
reviewed
Jan 26, 2026
|
|
||
| :: | ||
|
|
||
| SELECT cast('24:00:00' as time); -- NULL (hour out of range) |
Collaborator
There was a problem hiding this comment.
Perhaps combine the ANSI on/off behaviors with /.
velox/expression/CastExpr.cpp
Outdated
| } else { | ||
| VELOX_USER_FAIL( | ||
| makeErrorMessage(input, row, TIME()) + " " + | ||
| timeResult.error().message()); |
Collaborator
There was a problem hiding this comment.
Can we extract this lambda function for reuse?
velox/velox/expression/CastExpr-inl.h
Line 104 in 51495c4
Contributor
Author
There was a problem hiding this comment.
@rui-mo I've addressed both pieces of feedback:
- Combined ANSI on/off documentation examples using
/separator for more concise format - Extracted the setError lambda inside the applyToSelectedNoThrowLocal callback to avoid duplication and properly capture the row parameter
The branch has been rebased on latest main and all changes are formatted. Ready for re-review. Thanks!
malinjawi
force-pushed
the
spark-cast-string-to-time-ansi
branch
from
b5432ee to
1f0c2f7
Compare
- Implements ANSI-compliant string-to-time casting - Adds scope guard in test for proper config cleanup - Aligns with string-to-boolean PR feedback - Combines ANSI support checks for both boolean and time casts
malinjawi
force-pushed
the
spark-cast-string-to-time-ansi
branch
from
1f0c2f7 to
28da2ed
Compare
…mbda - Combined ANSI on/off documentation examples using '/' separator - Extracted setError lambda inside applyToSelectedNoThrowLocal for code reuse - Follows pattern from existing code at line 104 Addresses feedback from facebookincubator#16123
Commit 28da2ed introduced hooks architecture for VARCHAR to TIME casting but incorrectly made PrestoCastHooks return 'not supported', breaking the existing Presto VARCHAR to TIME functionality that was added in 2191ecb. This commit restores the original behavior by implementing castStringToTime() to call TIME()->valueToTime(), which is what Presto expects based on the 605 lines of tests in velox/expression/tests/CastExprTest.cpp. The Spark-specific ANSI mode behavior is handled separately in SparkCastHooks.
The original Presto implementation of VARCHAR to TIME casting (commit 2191ecb) required timezone and session start time for proper DST handling. When the hooks architecture was introduced, these parameters were missing from the hook signature, causing Presto tests to fail with incorrect time values. This commit: - Updates CastHooks interface to include timeZone and sessionStartTimeMs parameters - Updates CastExpr.cpp to pass these parameters from the query context - Updates PrestoCastHooks to use TIME()->valueToTime() with all 3 parameters - Updates SparkCastHooks to accept but ignore these parameters (Spark doesn't use timezone for TIME) This fixes the Presto varcharToTimeCast and varcharToTimeDSTGapHandling test failures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements string-to-TIME casting following Spark semantics with ANSI mode
support. TIME values are represented as microseconds since midnight
(0-86,399,999,999).Added
castStringToTime()to CastHooks interface and implementedSpark-specific behavior in SparkCastHooks. Supports format
H:m:s[.SSS]with whitespace trimming. ANSI mode integration via
isAnsiSupported()helper determines whether invalid input throws error (ANSI ON) or
returns NULL (ANSI OFF).
Comprehensive tests added covering valid/invalid inputs for both ANSI
modes with verified microsecond precision calculations.
Part of apache/incubator-gluten#10134