[SPARK-57852][SQL] Support TIME endpoints in the sequence function#57101
[SPARK-57852][SQL] Support TIME endpoints in the sequence function#57101vboo123 wants to merge 3 commits into
Conversation
Extend `sequence` to accept TIME start/stop endpoints with a day-time interval step, producing an array of TIME values within the day domain. `Sequence.checkInputDataTypes` now accepts TIME endpoints with an optional day-time interval step, and a new `TimeSequenceImpl` performs an integer walk in nanoseconds (no calendar, DST, or time zone arithmetic), truncating each element to the start endpoint's precision. When no step is given it defaults to INTERVAL '1' SECOND. Tests added to CollectionExpressionsSuite. Generated using Kiro (Claude Opus 4.8)
shrirangmhalgi
left a comment
There was a problem hiding this comment.
0 blocking, 1 non-blocking, 1 nit.
The TimeSequenceImpl is clean -- plain nanosecond walk with precision truncation, no calendar/TZ complexity.
What I Verified:
TimeTypestores Long nanoseconds since midnight (max 86,399,999,999,999)getSequenceLengthusesMath.subtractExactfor overflow-safe length computationtruncateTimeToPrecisionuses precomputed power-of-10 factor tablesequenceLengthbounds output toMAX_ROUNDED_ARRAY_LENGTH- Arithmetic in eval loop (
start + stepNanos * i) is safe: both endpoints < 10^14, well within Long
Non-blocking (1)
- codegen/eval asymmetry in step conversion -- see inline
Nit (1)
- A test asserting sequence(TIME'23:00:00', TIME'01:00:00', INTERVAL '1' HOUR) fails would document that TIME sequences don't wrap around midnight - relevant once #57044 SPARK-57853 (modulo-24 TIME arithmetic) lands.
Update DataFrameFunctionsSuite SEQUENCE_WRONG_INPUT_TYPES assertions to include TIME in the accepted start types, matching the extended error message. This is the cause of the failing CI Build (the assertions hardcoded the old type list). Make the codegen step microseconds->nanoseconds conversion overflow-safe with Math.multiplyExact to match the interpreted eval path. Add a test asserting TIME sequences do not wrap around midnight. Generated using Kiro (Claude Opus 4.8)
MaxGekk
left a comment
There was a problem hiding this comment.
Reviewed the TIME sequence support. I verified correctness by reading and running the suite on the branch — eval/codegen symmetry (the checkEvaluation test exercises both paths and passes), overflow bounds, and mixed-precision coercion all check out. No blocking issues; a few non-blocking items.
Correctness
- The user-facing
@ExpressionDescriptionis stale:Supported types are: byte, short, integer, long, date, timestamp.still omitstime, and the step-type rule + examples don't mention TIME. This shows up inDESCRIBE FUNCTION sequenceand the SQL functions reference page. Worth updating before merge (no golden regen needed). See inline.
Suggestions
- Three test-coverage additions, all of which I verified pass — see inline for the drafted cases.
- Optional: a small
sequenceblock intime.sqlfor end-to-end golden coverage (sequence()has no.sqltest for any type today, buttime.sqlis where TIME-accepting functions are golden-tested).
Nit
- "non day-time interval step" reads better as "non-day-time" (the type name is itself hyphenated). See inline.
Also: the codegen/eval step-conversion asymmetry raised earlier looks like a non-issue — the two paths are symmetric (both multiplyExact(step, NANOS_PER_MICROS) then walk identically), and any error-shape difference is pre-existing and shared by every sequence impl. The wrap-around test @shrirangmhalgi asked for is already present.
| YearMonthIntervalType.acceptsType(stepType) || | ||
| DayTimeIntervalType.acceptsType(stepType) | ||
| case _: TimeType => | ||
| // TIME lives within a single day, so only a day-time interval step is meaningful. |
There was a problem hiding this comment.
The user-facing @ExpressionDescription above (line ~3246) wasn't updated for TIME:
Supported types are: byte, short, integer, long, date, timestamp.→ addtime.- The step-type rule ("...'date' or 'timestamp' type then the step must resolve to...") should note that a TIME start/stop requires a day-time interval step (default 1 second).
- Consider adding an example, e.g.
SELECT sequence(TIME '08:00:00', TIME '10:00:00', INTERVAL '30' MINUTE).
This is what DESCRIBE FUNCTION sequence and the online SQL functions reference render. No golden file pins the description text, so this needs no .sql.out regen.
| assert(new Sequence( | ||
| Literal(tm(8, 0, 0), timeType), | ||
| Literal(tm(10, 0, 0), timeType), | ||
| Literal(Period.ofMonths(1))).checkInputDataTypes().isFailure) |
There was a problem hiding this comment.
Three branches aren't covered yet — I ran these and they pass; suggest adding after the year-month case:
// Only a day-time interval step is accepted: a calendar interval step (here 30 minutes as
// microseconds) is rejected at analysis time, just like the year-month interval above.
assert(new Sequence(
Literal(tm(8, 0, 0), timeType),
Literal(tm(10, 0, 0), timeType),
Literal(new org.apache.spark.unsafe.types.CalendarInterval(0, 0, 1800000000L)))
.checkInputDataTypes().isFailure)
// No step, descending range: the default step flips to -1 second.
checkEvaluation(new Sequence(
Literal(tm(8, 0, 3), timeType),
Literal(tm(8, 0, 0), timeType)),
Seq(tm(8, 0, 3), tm(8, 0, 2), tm(8, 0, 1), tm(8, 0, 0)))
// A step finer than the endpoint precision truncates every element to that precision, so
// TIME(3) with a 1-microsecond step yields repeated values rather than distinct sub-ms points.
checkEvaluation(new Sequence(
Literal(tm(8, 0, 0, 0), TimeType(3)),
Literal(tm(8, 0, 0, 3), TimeType(3)),
Literal(Duration.ofNanos(1000))), // 1 microsecond step, finer than TIME(3)
Seq(tm(8, 0, 0, 0), tm(8, 0, 0, 0), tm(8, 0, 0, 0), tm(8, 0, 0, 0)))Rationale: the existing test only rejects a year-month step (calendar-interval is a distinct rejected branch); covers descending only with an explicit step (not the default-flip); and the sub-precision case pins the truncate-to-repeats behavior against a future "dedup the walk" refactor. No new imports needed.
| Literal(Duration.ofMillis(100))), | ||
| Seq(tm(8, 0, 0, 0), tm(8, 0, 0, 100000), tm(8, 0, 0, 200000), tm(8, 0, 0, 300000))) | ||
|
|
||
| // A non day-time interval step (year-month) is rejected at analysis time. |
There was a problem hiding this comment.
Nit: "non day-time interval step" → "non-day-time interval step" (the type name "day-time" is itself hyphenated, so the negation reads more clearly hyphenated too).
Update the sequence @ExpressionDescription for TIME: add time to the supported
types, document that a TIME start/stop requires a day-time interval step with a
1-second default, and add a TIME example.
Add the review-suggested unit tests: calendar-interval step rejection,
default-step descending flip, and the sub-precision truncate-to-repeats case.
Fix a hyphenation nit ("non-day-time") in a test comment.
Add an end-to-end sequence-over-TIME block to the time.sql golden file
(ascending, descending, default step, and year-month step rejection).
Generated using Kiro (Claude Opus 4.8)
What changes were proposed in this pull request?
Extend
sequence(start, stop[, step])to accept TIME endpoints with a day-time interval step. Previously,Sequence.checkInputDataTypesin collectionOperations.scala only allowed integral, DATE, TIMESTAMP, and TIMESTAMP_NTZ endpoints. TIME fell through to the catch-all rejection branch, so any call likesequence(TIME '08:00:00', TIME '10:00:00', INTERVAL '30' MINUTE)raised an analysis error.Changes:
Sequence.checkInputDataTypesnow recognizes TIME endpoints paired with an optional day-time interval step.TimeSequenceImplgenerates the array via a plain integer walk in nanoseconds.SEQUENCE_WRONG_INPUT_TYPESerror message is extended to mention TIME.TIME is stored internally as nanoseconds-of-day while a day-time interval is stored as microseconds, so
TimeSequenceImplconverts the step from microseconds to nanoseconds and walks the range, truncating each element to the start endpoint's declared precision. It does not reuse the DATE/TIMESTAMP temporal machinery (no calendar, DST, or time-zone arithmetic) because a TIME sequence stays within the single-day domain [0, 24:00) by construction.When no step is given the default is INTERVAL '1' SECOND. This is open to reviewer feedback since DATE defaults to 1 day and TIMESTAMP defaults to 1 month, but TIME has no obvious natural default.
SPARK-57853 (defines TIME +/- INTERVAL out-of-range semantics, open PR #57044) does not conflict with this implementation because all produced values are within-day by construction.
Why are the changes needed?
The TIME type was added by SPIP SPARK-51162 (shipped in Spark 4.1.0) and works in the engine, but
sequence()rejected TIME endpoints at analysis time because the type check had no branch for it. This is a sub-task of the TIME umbrella SPARK-57550.Does this PR introduce any user-facing change?
Yes. Users can now generate TIME arrays with
sequence:How was this patch tested?
Added test("SPARK-57852: Sequence of times") to CollectionExpressionsSuite covering null arguments, ascending 30-minute step, descending negative step, stop not exactly reachable, single element (start == stop), default 1-second step, sub-second step preserving precision, and a negative case asserting a year-month interval step is rejected at analysis time. All 62 tests in CollectionExpressionsSuite pass locally.
Was this patch authored or co-authored using generative AI tooling?
Yes. Generated using Kiro (Claude Opus 4.8)