andygrove commented on code in PR #4821:
URL: https://github.com/apache/datafusion-comet/pull/4821#discussion_r3604113771
##########
spark/src/main/scala/org/apache/comet/codegen/CometBatchKernelCodegen.scala:
##########
@@ -87,6 +88,7 @@ object CometBatchKernelCodegen extends Logging with
CometExprTraitShim {
case _: StringType | _: BinaryType => true
case DateType | TimestampType | TimestampNTZType => true
case _: YearMonthIntervalType | _: DayTimeIntervalType => true
+ case dt if isTimeType(dt) => true
Review Comment:
Gated at plan time: \`isSupportedDataType\` now returns false for
ArrayType/StructType/MapType containing TIME, so \`canHandle\` returns a
fallback reason instead of letting the codegen crash mid-compile. The
array/struct/map getters do not yet route TIME to \`getLong\`; teaching them is
tracked separately.
##########
spark/src/main/spark-4.1+/org/apache/comet/shims/CometExprShim.scala:
##########
@@ -46,6 +47,22 @@ trait CometExprShim extends Spark4xCometExprShim {
}
}
+ // Function names on `DateTimeUtils` reached via `StaticInvoke` from the
Spark 4.1 `TIME` family of
+ // `RuntimeReplaceable` expressions (`HoursOfTime`, `MinutesOfTime`,
`SecondsOfTime`,
+ // `SecondsOfTimeWithFraction`, `TimeAddInterval`, `SubtractTimes`,
`TimeDiff`, `TimeTrunc`).
+ // Each of these is codegen-safe (the replacement itself is a `StaticInvoke`
with a proper
+ // `doGenCode`) but has no native lowering yet, so we route them through the
JVM codegen
+ // dispatcher to keep the enclosing projection native.
+ private val timeCodegenDispatchFunctions: Set[String] = Set(
+ "getHoursOfTime",
+ "getMinutesOfTime",
+ "getSecondsOfTime",
+ "getSecondsOfTimeWithFraction",
+ "timeAddInterval",
Review Comment:
Added `DurationVector` (and `IntervalYearVector`) to
`CometScalaUDFCodegen.specFor` and to the `getLong` ordinal switch in
`CometBatchKernelCodegenInput`. A non-foldable `DayTimeIntervalType` attribute
passed into `timeAddInterval` now evaluates through the dispatcher instead of
throwing `UnsupportedOperationException`.
##########
docs/source/user-guide/latest/expressions.md:
##########
@@ -278,13 +278,13 @@ The type-name conversion functions (`bigint`, `binary`,
`boolean`, `date`, `deci
| `quarter` | ✅ | |
| `second` | ✅ | |
| `session_window` | 🔜 | Batch session-window grouping falls back
(`UpdatingSessionsExec` is not yet native); tracked by
[#4785](https://github.com/apache/datafusion-comet/issues/4785) |
-| `time_diff` | 🔜 | Spark 4.1 TIME type; tracked by
[#4288](https://github.com/apache/datafusion-comet/issues/4288) |
-| `time_trunc` | 🔜 | Spark 4.1 TIME type; tracked by
[#4288](https://github.com/apache/datafusion-comet/issues/4288) |
+| `time_diff` | ✅ | Spark 4.1+; routes through the JVM codegen dispatcher |
+| `time_trunc` | ✅ | Spark 4.1+; routes through the JVM codegen dispatcher |
| `timestamp_micros` | ✅ | |
| `timestamp_millis` | ✅ | |
| `timestamp_seconds` | ✅ | |
| `to_date` | ✅ | Rewrites to `Cast` (or `Cast(GetTimestamp)` with a format)
before Comet sees the plan |
-| `to_time` | 🔜 | Spark 4.1 TIME type; tracked by
[#4288](https://github.com/apache/datafusion-comet/issues/4288) |
+| `to_time` | ✅ | Spark 4.1+ |
Review Comment:
Updated both rows: `to_time` and `try_to_time` now state that only the
default-format form is supported; formatted forms fall back to Spark.
--
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]
