dwsmith1983 commented on code in PR #5864: URL: https://github.com/apache/datafusion-comet/pull/5864#discussion_r3991715387
########## spark/src/main/scala/org/apache/comet/serde/datetime.scala: ########## @@ -997,6 +997,19 @@ object CometTimestampAdd extends CometCodegenDispatch[TimestampAdd] object CometTimestampDiff extends CometCodegenDispatch[TimestampDiff] +// Date and timestamp interval arithmetic. `timestamp + day-time or calendar interval` resolves +// to `TimeAdd` on Spark 3.4 through 4.0 and to `TimestampAddInterval` on 4.1+, so that serde +// lives in the version shims. +object CometDateAddInterval extends CometCodegenDispatch[DateAddInterval] + +object CometDateAddYMInterval extends CometCodegenDispatch[DateAddYMInterval] + +object CometTimestampAddYMInterval extends CometCodegenDispatch[TimestampAddYMInterval] + +object CometSubtractDates extends CometCodegenDispatch[SubtractDates] + +object CometSubtractTimestamps extends CometCodegenDispatch[SubtractTimestamps] Review Comment: > Could we preserve the full legacy interval range before routing `SubtractTimestamps` here? The range cannot be preserved through the dispatcher: the calendar-interval output is an Arrow month-day-nano vector, and a span past about 292 years has no representation there without folding microseconds into days, which changes the arithmetic downstream as you note. So legacy mode is no longer dispatched. `CometSubtractTimestamps.getSupportLevel` returns `Unsupported` when the result type is `CalendarIntervalType`, with the reason stated, and the expression keeps the Spark fallback it had before this PR; default mode, whose `DayTimeIntervalType` result is a plain long of microseconds, stays dispatched. Regressions: `subtract_timestamps_long_span.sql` holds `2300-01-01` and `1970-01-01` in TIMESTAMP and TIMESTAMP_NTZ columns, in both signs and both operand orders, and runs dispatched in default mode; `subtract_timestamps_long_span_legacy.sql` runs the same table in legacy mode and asserts the fallback with Spark's answer. Before the change the legacy file failed with `java.lang.ArithmeticException: long overflow` from `Math.multiplyExact` in the generated kernel. `subtract_dates.sql` gained the same 330-year rows, which stay native in both modes since a day count fits. The DST prose was reversed and is corrected: default mode reports the local wall-clock difference across a transition, legacy mode the elapsed 23 or 25 hours. The legacy-mode DST queries moved to `subtract_timestamps_legacy.sql` as fallback assertions since the matrix leg can no longer assert native execution. -- 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]
