MaxGekk commented on code in PR #56637:
URL: https://github.com/apache/spark/pull/56637#discussion_r3448983224
##########
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, AnyTimeType),
StringTypeWithCollation(supportsTrimCollation = true))
override def withTimeZone(timeZoneId: String): TimeZoneAwareExpression =
copy(timeZoneId = Option(timeZoneId))
override protected def nullSafeEval(timestamp: Any, format: Any): Any = {
- val formatter = formatterOption.getOrElse(getFormatter(format.toString))
- UTF8String.fromString(formatter.format(timestamp.asInstanceOf[Long]))
+ left.dataType match {
+ case _: TimeType =>
+ val tf = timeFormatterOption.getOrElse(
+ TimeFormatter(format.toString, isParsing = false))
+ UTF8String.fromString(tf.format(timestamp.asInstanceOf[Long]))
Review Comment:
Follow-up on the prior thread (now that the exception type is pinned). A
date-only pattern like `yyyy-MM-dd` is syntactically valid, so
`validatePatternString()` passes at construction and this `format` call throws
a raw `java.time.UnsupportedTemporalTypeException` at runtime — there's no
Spark error class, so the PR description's "rejected at runtime with a clear
error" is still overstated, and the test pins it only on the interpreted
`.eval` path.
Centralizing the TIME format in one helper that both `nullSafeEval` and
`doGenCode` call maps it to a proper error and covers codegen for free:
```scala
def formatTime(tf: TimeFormatter, nanos: Long, pattern: String): UTF8String =
try UTF8String.fromString(tf.format(nanos))
catch { case e: DateTimeException =>
throw QueryExecutionErrors.invalidPatternError("to_char", pattern, e) }
```
`QueryExecutionErrors.invalidPatternError` already exists
(`INVALID_PARAMETER_VALUE.PATTERN`) — no new error class needed. Then swap the
test's interpreted-only `intercept[...] { expr.eval(InternalRow(...)) }` for
`checkErrorInExpression[SparkRuntimeException](expr,
"INVALID_PARAMETER_VALUE.PATTERN", ...)`, which runs interpreted *and* codegen.
If a Spark error is out of scope here, the minimal alternative is to drop
"clear error" from the description and use
`checkExceptionInExpression[DateTimeException]` (which still covers codegen).
Non-blocking — fine to defer.
--
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]
