sunchao commented on code in PR #5864: URL: https://github.com/apache/datafusion-comet/pull/5864#discussion_r3991181923
########## 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: ### Correctness [P2] Preserve large legacy timestamp differences through the dispatcher Could we preserve the full legacy interval range before routing `SubtractTimestamps` here? With the codegen dispatcher enabled and `spark.sql.legacy.interval.enabled=true`, Spark's eval and codegen both construct `CalendarInterval(0, 0, end - start)`. A Parquet row containing UTC timestamps `2300-01-01` and `1970-01-01` therefore produces `10413792000000000` microseconds, and reversing the operands is valid too. The shared dispatcher accepts this output type, but its [calendar-interval writer](https://github.com/apache/datafusion-comet/blob/61345818df278dd444e80cddd372e015452b61cf/spark/src/main/scala/org/apache/comet/codegen/CometBatchKernelCodegenOutput.scala#L222-L229) calls `Math.multiplyExact(microseconds, 1000L)`. That throws once the magnitude exceeds `9223372036854775` microseconds, approximately 292 years. This new registration changes a successful Spark fallback into an execution error, exposing the writer limitation already noted for `make_interval` in #5279 to timestamp subtraction as well. The same limit applies to `TIMESTAMP_NTZ` and is independent of ANSI mode. I verified the emitted calculation with the maintained Spark interval class in a Java component probe, including both signs and the last safe/first failing values. Could we add column-based regression cases for this span alongside the output-range fix? The existing timestamp fixture's short spans do not exercise this boundary. -- 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]
