MaxGekk commented on code in PR #56637:
URL: https://github.com/apache/spark/pull/56637#discussion_r3448099109
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala:
##########
@@ -1139,34 +1139,63 @@ case class DateFormatClass(left: Expression, right:
Expression, timeZoneId: Opti
def this(left: Expression, right: Expression) = this(left, right, None)
override def inputTypes: Seq[AbstractDataType] =
- Seq(TimestampType, StringTypeWithCollation(supportsTrimCollation = true))
+ Seq(TypeCollection(TimestampType, TimeType(TimeType.DEFAULT_PRECISION)),
StringTypeWithCollation(supportsTrimCollation = true))
Review Comment:
This accepts only `TIME(6)`. A TIME value of any other precision (`TIME(3)`,
`current_time(3)`, a `CAST(... AS TIME(0))` column) doesn't match by exact
precision, so it gets implicitly coerced to `TimestampType` (the first
collection member) — and `TIME -> TIMESTAMP` isn't a valid cast, so the query
fails at analysis instead of formatting. Every other TIME-accepting expression
(`MinutesOfTime`, `HoursOfTime`, `TimeFromMicros`) uses `AnyTimeType`, which
accepts all precisions 0-6 directly.
```suggestion
Seq(TypeCollection(TimestampType, AnyTimeType),
StringTypeWithCollation(supportsTrimCollation = true))
```
Worth adding a regression test on a non-6 precision input — every current
test and golden query uses a `TIME'...'` literal, which defaults to `TIME(6)`,
so this gap is untested today.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala:
##########
@@ -328,6 +328,28 @@ class DateExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
}
}
+ test("SPARK-57575: DateFormat with TimeType (to_char/to_varchar)") {
+ // 12:13:14 = (12*3600 + 13*60 + 14) seconds, stored as nanoseconds
+ val timeMicros = (12L * 3600 + 13 * 60 + 14) * 1000000000L
+ val timeLit = Literal.create(timeMicros,
TimeType(TimeType.DEFAULT_PRECISION))
+
+ checkEvaluation(DateFormatClass(timeLit, Literal("HH:mm:ss"), UTC_OPT),
"12:13:14")
+ checkEvaluation(DateFormatClass(timeLit, Literal("HH"), UTC_OPT), "12")
+ checkEvaluation(DateFormatClass(timeLit, Literal("mm"), UTC_OPT), "13")
+ checkEvaluation(DateFormatClass(timeLit, Literal("ss"), UTC_OPT), "14")
+
+ // Null handling
+ checkEvaluation(
+ DateFormatClass(Literal.create(null,
TimeType(TimeType.DEFAULT_PRECISION)),
+ Literal("HH:mm:ss"), UTC_OPT), null)
+
+ // Date-only pattern fields should error for TIME input
+ val datePatternExpr = DateFormatClass(timeLit, Literal("yyyy-MM-dd"),
UTC_OPT)
+ intercept[Exception] {
Review Comment:
`intercept[Exception]` passes on any throwable and only exercises the
interpreted `eval`, not codegen. The exception it catches is a raw `java.time`
`DateTimeException` from `LocalTime.format` (the pattern's syntax is valid, so
`validatePatternString` passes and the failure only surfaces at format time) —
there's no Spark error class, so the PR description's "clear error" is
overstated. Consider rejecting date-only patterns with a proper Spark error,
then pin this test to that specific exception/message and cover the codegen
path too.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala:
##########
@@ -328,6 +328,28 @@ class DateExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
}
}
+ test("SPARK-57575: DateFormat with TimeType (to_char/to_varchar)") {
+ // 12:13:14 = (12*3600 + 13*60 + 14) seconds, stored as nanoseconds
+ val timeMicros = (12L * 3600 + 13 * 60 + 14) * 1000000000L
Review Comment:
`timeMicros` holds a nanosecond value (the comment says "stored as
nanoseconds", and TimeType is encoded in nanos-since-midnight). Rename to
`timeNanos` to avoid misleading the next reader.
--
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]
