MaxGekk commented on code in PR #57101:
URL: https://github.com/apache/spark/pull/57101#discussion_r3574167721
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala:
##########
@@ -3327,6 +3327,9 @@ case class Sequence(
stepOpt.isEmpty || CalendarIntervalType.acceptsType(stepType) ||
YearMonthIntervalType.acceptsType(stepType) ||
DayTimeIntervalType.acceptsType(stepType)
+ case _: TimeType =>
+ // TIME lives within a single day, so only a day-time interval
step is meaningful.
Review Comment:
The user-facing `@ExpressionDescription` above (line ~3246) wasn't updated
for TIME:
- `Supported types are: byte, short, integer, long, date, timestamp.` → add
`time`.
- 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.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala:
##########
@@ -1296,6 +1296,78 @@ class CollectionExpressionsSuite
Timestamp.valueOf("2018-01-01 00:00:00.000")))
}
+ test("SPARK-57852: Sequence of times") {
+ val timeType = TimeType()
+ def tm(h: Int, m: Int, s: Int, micros: Int = 0): Long =
+ DateTimeTestUtils.localTime(h.toByte, m.toByte, s.toByte, micros)
+
+ // Null in any argument yields null.
+ checkEvaluation(new Sequence(
+ Literal(null, timeType), Literal(tm(10, 0, 0), timeType)), null)
+ checkEvaluation(new Sequence(
+ Literal(tm(8, 0, 0), timeType), Literal(null, timeType)), null)
+ checkEvaluation(new Sequence(
+ Literal(tm(8, 0, 0), timeType),
+ Literal(tm(10, 0, 0), timeType),
+ Literal(null, DayTimeIntervalType())), null)
+
+ // Ascending with an explicit 30-minute step (the ticket's acceptance
criterion).
+ checkEvaluation(new Sequence(
+ Literal(tm(8, 0, 0), timeType),
+ Literal(tm(10, 0, 0), timeType),
+ Literal(Duration.ofMinutes(30))),
+ Seq(tm(8, 0, 0), tm(8, 30, 0), tm(9, 0, 0), tm(9, 30, 0), tm(10, 0, 0)))
+
+ // Descending with a negative step.
+ checkEvaluation(new Sequence(
+ Literal(tm(10, 0, 0), timeType),
+ Literal(tm(8, 0, 0), timeType),
+ Literal(Duration.ofMinutes(-30))),
+ Seq(tm(10, 0, 0), tm(9, 30, 0), tm(9, 0, 0), tm(8, 30, 0), tm(8, 0, 0)))
+
+ // Stop not exactly reachable by the step: stops at the last value within
bounds.
+ checkEvaluation(new Sequence(
+ Literal(tm(8, 0, 0), timeType),
+ Literal(tm(9, 10, 0), timeType),
+ Literal(Duration.ofMinutes(30))),
+ Seq(tm(8, 0, 0), tm(8, 30, 0), tm(9, 0, 0)))
+
+ // Single element when start equals stop.
+ checkEvaluation(new Sequence(
+ Literal(tm(8, 0, 0), timeType),
+ Literal(tm(8, 0, 0), timeType),
+ Literal(Duration.ofMinutes(30))),
+ Seq(tm(8, 0, 0)))
+
+ // No step defaults to 1 second.
+ checkEvaluation(new Sequence(
+ Literal(tm(8, 0, 0), timeType),
+ Literal(tm(8, 0, 3), timeType)),
+ Seq(tm(8, 0, 0), tm(8, 0, 1), tm(8, 0, 2), tm(8, 0, 3)))
+
+ // Sub-second steps preserve the endpoint precision exactly.
+ checkEvaluation(new Sequence(
+ Literal(tm(8, 0, 0, 0), timeType),
+ Literal(tm(8, 0, 0, 300000), timeType),
+ 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.
+ assert(new Sequence(
+ Literal(tm(8, 0, 0), timeType),
+ Literal(tm(10, 0, 0), timeType),
+ Literal(Period.ofMonths(1))).checkInputDataTypes().isFailure)
Review Comment:
Three branches aren't covered yet — I ran these and they pass; suggest
adding after the year-month case:
```scala
// 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.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala:
##########
@@ -1296,6 +1296,78 @@ class CollectionExpressionsSuite
Timestamp.valueOf("2018-01-01 00:00:00.000")))
}
+ test("SPARK-57852: Sequence of times") {
+ val timeType = TimeType()
+ def tm(h: Int, m: Int, s: Int, micros: Int = 0): Long =
+ DateTimeTestUtils.localTime(h.toByte, m.toByte, s.toByte, micros)
+
+ // Null in any argument yields null.
+ checkEvaluation(new Sequence(
+ Literal(null, timeType), Literal(tm(10, 0, 0), timeType)), null)
+ checkEvaluation(new Sequence(
+ Literal(tm(8, 0, 0), timeType), Literal(null, timeType)), null)
+ checkEvaluation(new Sequence(
+ Literal(tm(8, 0, 0), timeType),
+ Literal(tm(10, 0, 0), timeType),
+ Literal(null, DayTimeIntervalType())), null)
+
+ // Ascending with an explicit 30-minute step (the ticket's acceptance
criterion).
+ checkEvaluation(new Sequence(
+ Literal(tm(8, 0, 0), timeType),
+ Literal(tm(10, 0, 0), timeType),
+ Literal(Duration.ofMinutes(30))),
+ Seq(tm(8, 0, 0), tm(8, 30, 0), tm(9, 0, 0), tm(9, 30, 0), tm(10, 0, 0)))
+
+ // Descending with a negative step.
+ checkEvaluation(new Sequence(
+ Literal(tm(10, 0, 0), timeType),
+ Literal(tm(8, 0, 0), timeType),
+ Literal(Duration.ofMinutes(-30))),
+ Seq(tm(10, 0, 0), tm(9, 30, 0), tm(9, 0, 0), tm(8, 30, 0), tm(8, 0, 0)))
+
+ // Stop not exactly reachable by the step: stops at the last value within
bounds.
+ checkEvaluation(new Sequence(
+ Literal(tm(8, 0, 0), timeType),
+ Literal(tm(9, 10, 0), timeType),
+ Literal(Duration.ofMinutes(30))),
+ Seq(tm(8, 0, 0), tm(8, 30, 0), tm(9, 0, 0)))
+
+ // Single element when start equals stop.
+ checkEvaluation(new Sequence(
+ Literal(tm(8, 0, 0), timeType),
+ Literal(tm(8, 0, 0), timeType),
+ Literal(Duration.ofMinutes(30))),
+ Seq(tm(8, 0, 0)))
+
+ // No step defaults to 1 second.
+ checkEvaluation(new Sequence(
+ Literal(tm(8, 0, 0), timeType),
+ Literal(tm(8, 0, 3), timeType)),
+ Seq(tm(8, 0, 0), tm(8, 0, 1), tm(8, 0, 2), tm(8, 0, 3)))
+
+ // Sub-second steps preserve the endpoint precision exactly.
+ checkEvaluation(new Sequence(
+ Literal(tm(8, 0, 0, 0), timeType),
+ Literal(tm(8, 0, 0, 300000), timeType),
+ 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.
Review 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
