MaxGekk commented on code in PR #53320:
URL: https://github.com/apache/spark/pull/53320#discussion_r3482607898
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -1036,3 +1036,82 @@ case class TimeToMicros(child: Expression)
override protected def withNewChildInternal(newChild: Expression):
TimeToMicros =
copy(child = newChild)
}
+
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = "_FUNC_(time, format) - Converts a time to a value of string in the
format specified by the date format given by the second argument.",
+ arguments = """
+ Arguments:
+ * time - A time value to be converted to string.
+ * format - Time format pattern to follow. See <a
href="https://spark.apache.org/docs/latest/sql-ref-datetime-pattern.html";>Datetime
Patterns</a> for valid
+ time format patterns. Note: Only time-related patterns (H, h,
m, s, S, a) are meaningful for TIME values.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_(TIME'14:30:45', 'HH:mm:ss');
+ 14:30:45
+ > SELECT _FUNC_(TIME'14:30:45', 'hh:mm:ss a');
+ 02:30:45 PM
+ > SELECT _FUNC_(TIME'14:30:45.123456', 'HH:mm:ss.SSSSSS');
+ 14:30:45.123456
+ > SELECT _FUNC_(TIME'09:05:00', 'h:mm a');
+ 9:05 AM
+ """,
+ group = "datetime_funcs",
+ since = "4.2.0")
+// scalastyle:on line.size.limit
+case class TimeFormat(left: Expression, right: Expression)
+ extends BinaryExpression
+ with ImplicitCastInputTypes {
+
+ override def nullIntolerant: Boolean = true
+
+ override def inputTypes: Seq[AbstractDataType] =
+ Seq(AnyTimeType, StringTypeWithCollation(supportsTrimCollation = true))
+
+ override def dataType: DataType = StringType
+
+ // Cache the formatter if the format string is a foldable expression
+ @transient private lazy val formatterOption: Option[TimeFormatter] =
+ if (right.foldable) {
+ Option(right.eval()).map { format =>
+ TimeFormatter(format.toString, TimeFormatter.defaultLocale, isParsing
= false)
+ }
+ } else {
+ None
+ }
+
+ override protected def nullSafeEval(time: Any, format: Any): Any = {
+ val nanos = time.asInstanceOf[Long]
+ val formatter = formatterOption.getOrElse {
+ TimeFormatter(format.toString, TimeFormatter.defaultLocale, isParsing =
false)
+ }
+ UTF8String.fromString(formatter.format(nanos))
Review Comment:
A format that references a date-only field (e.g. `MM`, `yyyy`, `dd`) leaks a
raw `java.time.temporal.UnsupportedTemporalTypeException: Unsupported field:
MonthOfYear` here — see the golden output for `SELECT
time_format(TIME'14:30:45', 'HH:MM:ss')` in `time.sql.out`.
`TimeFormatter.validatePatternString` passes (`MM` is a valid letter), then
`LocalTime.format` throws at runtime because a `TIME` has no date fields.
Syntactically-invalid patterns are already wrapped cleanly (the `'invalid[[['`
test), so this valid-but-inapplicable-letter path is the gap; `date_format`
never hits it (timestamps carry all fields). Suggest validating the pattern to
time-applicable fields, or catching the `UnsupportedTemporalTypeException` and
raising a clear Spark error (an `INVALID_DATETIME_PATTERN`-style message scoped
to TIME) rather than leaking the JDK exception.
##########
sql/core/src/test/resources/sql-tests/inputs/time.sql:
##########
@@ -365,3 +365,119 @@ SELECT time_to_millis(time_from_millis(52200500));
SELECT time_from_millis(time_to_millis(TIME'14:30:00.5'));
SELECT time_to_micros(time_from_micros(52200500000));
SELECT time_from_micros(time_to_micros(TIME'14:30:00.5'));
+
+-- TIME_FORMAT function tests
+-- Test basic 24-hour format
+SELECT time_format(TIME'14:30:45', 'HH:mm:ss');
+
+-- Test 12-hour format with AM/PM
+SELECT time_format(TIME'14:30:45', 'hh:mm:ss a');
+SELECT time_format(TIME'09:15:30', 'hh:mm:ss a');
+
+-- Test with different precisions
+SELECT time_format(TIME'14:30:45.123', 'HH:mm:ss.SSS');
+SELECT time_format(TIME'14:30:45.123456', 'HH:mm:ss.SSSSSS');
+
+-- Test various format patterns
+SELECT time_format(TIME'14:30:45', 'HH:mm');
+SELECT time_format(TIME'14:30:45', 'HH');
+SELECT time_format(TIME'09:05:00', 'H:mm a');
+
+-- Test edge cases - midnight
+SELECT time_format(TIME'00:00:00', 'HH:mm:ss');
+SELECT time_format(TIME'00:00:00', 'hh:mm:ss a');
+
+-- Test edge cases - noon
+SELECT time_format(TIME'12:00:00', 'HH:mm:ss');
+SELECT time_format(TIME'12:00:00', 'hh:mm:ss a');
+
+-- Test edge cases - end of day
+SELECT time_format(TIME'23:59:59', 'HH:mm:ss');
+SELECT time_format(TIME'23:59:59.999999', 'HH:mm:ss.SSSSSS');
+
+-- Test null handling
+SELECT time_format(NULL, 'HH:mm:ss');
+SELECT time_format(TIME'14:30:45', NULL);
+SELECT time_format(CAST(NULL AS TIME), 'HH:mm:ss');
+
+-- Test custom formats with different separators
+SELECT time_format(TIME'14:30:45', 'hh-mm-ss a');
+SELECT time_format(TIME'14:30:45', 'HH.mm.ss');
+SELECT time_format(TIME'14:30:45', 'HH_mm_ss');
+
+-- Test with text literals
+SELECT time_format(TIME'14:30:45', '''Time:'' HH:mm:ss');
+
+-- Test verbose formats
+SELECT time_format(TIME'02:20:30.040', 'H ''hours'' mm ''mins'' ss ''seconds''
SSS ''milliseconds''');
+SELECT time_format(TIME'14:05:15', 'H ''hours,'' mm ''minutes and'' ss
''seconds''');
+
+-- Test AM/PM edge cases
+SELECT time_format(TIME'00:00:00', 'a');
+SELECT time_format(TIME'11:59:59', 'a');
+SELECT time_format(TIME'12:00:00', 'a');
+SELECT time_format(TIME'13:00:00', 'a');
+SELECT time_format(TIME'23:59:59', 'a');
+
+-- Test hour edge cases for 12-hour format
+SELECT time_format(TIME'00:00:00', 'hh:mm a');
+SELECT time_format(TIME'00:30:00', 'hh:mm a');
+SELECT time_format(TIME'01:00:00', 'hh:mm a');
+SELECT time_format(TIME'11:00:00', 'hh:mm a');
+SELECT time_format(TIME'12:00:00', 'hh:mm a');
+SELECT time_format(TIME'13:00:00', 'hh:mm a');
+
+-- Test single vs double digit hour (H vs HH)
+SELECT time_format(TIME'09:15:30', 'H:mm:ss');
+SELECT time_format(TIME'09:15:30', 'HH:mm:ss');
+SELECT time_format(TIME'09:15:30', 'h:mm a');
+SELECT time_format(TIME'09:15:30', 'hh:mm a');
+
+-- Test precision variations (additional subsecond formats)
+SELECT time_format(TIME'14:30:45.1', 'HH:mm:ss.S');
+SELECT time_format(TIME'14:30:45.12', 'HH:mm:ss.SS');
+
+-- Test with COALESCE
+SELECT coalesce(time_format(NULL, 'HH:mm'), 'N/A');
+SELECT coalesce(time_format(TIME'14:30:45', NULL), 'N/A');
+
+-- Test concatenation with formatted time
+SELECT concat('The time is ', time_format(TIME'14:30:45', 'hh:mm a'));
+
+-- Test single format showing both 24-hour and 12-hour in one output
+SELECT time_format(TIME'14:30:45', 'HH:mm:ss (hh:mm:ss a)');
+SELECT time_format(TIME'09:15:30', 'HH:mm (h:mm a)');
+SELECT time_format(TIME'23:45:00', '''24hr:'' HH:mm ''12hr:'' hh:mm a');
+SELECT time_format(TIME'00:30:00', 'HH:mm:ss = hh:mm:ss a');
+SELECT time_format(TIME'14:30:45', 'HH:mm:ss ''(24h)'' / hh:mm:ss a
''(12h)''');
+
+-- Test multiple formats in single query
+SELECT time_format(TIME'14:30:45', 'HH:mm:ss'), time_format(TIME'14:30:45',
'hh:mm:ss a');
+SELECT time_format(TIME'09:15:30', 'HH:mm'), time_format(TIME'09:15:30', 'h:mm
a');
+SELECT
+ time_format(TIME'14:30:45.123456', 'HH:mm:ss') as format_24hr,
+ time_format(TIME'14:30:45.123456', 'hh:mm:ss a') as format_12hr,
+ time_format(TIME'14:30:45.123456', 'HH:mm:ss.SSSSSS') as format_with_micros;
+
+-- Test special characters and patterns
+SELECT time_format(TIME'14:30:45', 'HH:mm:ss ''UTC''');
+SELECT time_format(TIME'14:30:45', '[HH:mm:ss]');
+SELECT time_format(TIME'14:30:45', 'HH|mm|ss');
+SELECT time_format(TIME'14:30:45', 'HH-mm-ss');
+
+-- Test common mistake: MM (month) vs mm (minute)
+-- MM is for month, mm is for minute - TIME has no date so MM returns epoch
month (01)
Review Comment:
This comment says `MM` "returns epoch month (01)", but the golden output for
the query below is `UnsupportedTemporalTypeException: Unsupported field:
MonthOfYear` — it throws, it doesn't return `01`. Date fields don't default to
epoch for a `TIME`. Worth correcting the comment to say date fields are
unsupported and raise an error (this also ties to the error-handling suggestion
on the expression).
##########
sql/api/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -6889,6 +6889,22 @@ object functions {
Column.fn("time_diff", unit, start, end)
}
+ /**
+ * Converts a time to a string in the specified format.
+ *
+ * @param time
+ * A column of time values to be formatted.
+ * @param format
+ * A time format string. for valid patterns.
Review Comment:
`@param format` reads "A time format string. for valid patterns." — a
dangling fragment (a clause referencing the Datetime Patterns doc was dropped).
Complete it, e.g. "A time format string. See <a
href=\"https://spark.apache.org/docs/latest/sql-ref-datetime-pattern.html\";>Datetime
Patterns</a> for valid patterns."
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/TimeExpressionsSuite.scala:
##########
@@ -666,4 +666,66 @@ class TimeExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
val timeVal = TimeFromSeconds(Literal(secondsValue))
checkEvaluation(TimeToMicros(timeVal), 14500000L)
}
+
+ test("TimeFormat - foldable and non-foldable formats with codegen") {
+ val time = localTime(14, 30, 45, 123456)
+ val timeLit = Literal(time, TimeType())
+
+ // Foldable format - formatter cached at planning time
+ val foldableExpr = TimeFormat(timeLit, Literal("HH:mm:ss"))
+ checkEvaluation(foldableExpr, "14:30:45")
+ checkEvaluation(
+ TimeFormat(Literal(localTime(9, 5, 0), TimeType()), Literal("hh:mm:ss
a")),
+ "09:05:00 AM")
+ checkEvaluation(
+ TimeFormat(timeLit, Literal("HH:mm:ss.SSSSSS")),
Review Comment:
Optional: the fractional-second cases stop at 6 digits (micros). `TIME`
supports precision up to 9 (nanos), so a `TIME'…123456789'` with `SSSSSSSSS`
case (which `checkEvaluation` exercises in both interpreted and codegen paths)
would lock in nanosecond formatting.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -1036,3 +1036,82 @@ case class TimeToMicros(child: Expression)
override protected def withNewChildInternal(newChild: Expression):
TimeToMicros =
copy(child = newChild)
}
+
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+ usage = "_FUNC_(time, format) - Converts a time to a value of string in the
format specified by the date format given by the second argument.",
Review Comment:
Nit: the usage string says "...in the format specified by the **date**
format given by the second argument" — copied from `date_format`. This function
formats a TIME, so it should read "time format".
--
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]
