feat: route date and timestamp interval arithmetic through codegen dispatch

[dwsmith1983]

Which issue does this PR close?

Closes #3094, closes #3112 (default interval mode; legacy interval mode keeps its Spark fallback, see below), closes #3086, closes #3115, closes #3114.

Rationale for this change

Spark's date and timestamp interval arithmetic (date - date, timestamp - timestamp, date + interval, timestamp + interval) had no serde at all, so any projection using them fell back to Spark with a columnar-to-row transition. The recent temporal additions (timestampadd, timestampdiff, make_interval) show these run inside the Comet pipeline through the codegen dispatcher with a one-line serde each, and the dispatcher already accepts every interval type on input and output, so nothing native needs to change.

What changes are included in this PR?

• CometSubtractDates, CometSubtractTimestamps, CometDateAddInterval, CometDateAddYMInterval and CometTimestampAddYMInterval as CometCodegenDispatch objects in serde/datetime.scala, registered in temporalExpressions.
• timestamp + day-time interval is TimeAdd on Spark 3.4, 3.5 and 4.0 and TimestampAddInterval on 4.1 and later, so those two go through the version shims' misc expressions: TimeAdd is registered inline as new CometCodegenDispatch[TimeAdd] in the 3.4, 3.5 and 4.0 shims, and a CometTimestampAddInterval serde lives in spark-4.1+.
• The date - interval and timestamp - interval forms need nothing extra: Spark rewrites DatetimeSub into the add form over a negated interval, and the dispatcher binds the whole subtree.
• Notes on the + and - rows of the expressions guide.

timestamp - timestamp in legacy interval mode is not dispatched: its result is a CalendarInterval whose microseconds can exceed the roughly 292 years the dispatcher's calendar-interval output can carry, so that mode keeps its Spark fallback with a stated reason, and a 330-year span is pinned in both modes.

How are these changes tested?

Eleven SQL-file fixtures under sql-tests/expressions/datetime/, all over parquet tables so nothing folds to a literal, with NULL and negative operands, month-end clamping for year-month intervals, rows across both DST transitions in America/Los_Angeles, TIMESTAMP and TIMESTAMP_NTZ inputs, both values of spark.sql.legacy.interval.enabled where the result type changes, and one query per file through native shuffle. The ANSI rejection of a date plus an interval with a time part is split by Spark version, since 3.x reports a plain message and 4.x reports INVALID_INTERVAL_WITH_MICROSECONDS_ADDITION.

Before the serde change, seven of the fixtures failed with Expected only Comet native operators, but found Project. After it: 20 of 20 interval fixtures on Spark 3.5, the ANSI fixtures on the Spark 4.0 profile, and test-compile on the 3.4, 4.0 and 4.1 profiles, spotless clean.

One pre-existing gap surfaced while writing the fixtures: in legacy-interval mode Spark's null propagation folds CAST(NULL AS DATE) - date'...' into a bare NULL literal of CalendarIntervalType, which CometLiteral admits and the native planner rejects at execution. That is #5058 (fix in #5133); the two fixture notes cite it so the literal cases can be added once it lands.

[sunchao]

Correctness

Reviewed 61345818df27 against f29a236128b3. The PR adds five shared serdes for DateAddInterval, DateAddYMInterval, TimestampAddYMInterval, SubtractDates and SubtractTimestamps. Version shims add TimeAdd for Spark 3.4–4.0 and TimestampAddInterval for 4.1+. These expressions previously made the enclosing operator fall back to Spark. The new path serializes the bound Catalyst subtree and evaluates Spark's generated code within the Comet pipeline.

The maintained Spark 3.5 and 4.0 sources agree with this approach for null propagation, negative operands, month-end/leap-year clamping and timezone handling. Timestamp arithmetic uses the session zone for TIMESTAMP and UTC for TIMESTAMP_NTZ. Date-plus-calendar-interval retains the ANSI rejection of a nonzero time component and the non-ANSI timestamp conversion path. Default date subtraction retains checked subtraction and multiplication. Interval subtraction is resolved to addition over a negated interval, so a separate subtraction kernel is unnecessary.

There is one P2 correctness finding on the new SubtractTimestamps route: a valid legacy interval spanning more than approximately 292 years overflows when the dispatcher converts microseconds to Arrow nanoseconds. The inline comment gives the source path and a concrete column-based regression case. Spark's arithmetic itself succeeds. Separately, the timestamp fixture's description of DST subtraction is reversed on both maintained branches: default mode uses local wall-clock differences, while legacy mode uses elapsed microseconds.

Validation

The eight SQL files contain 38 queries, including version-specific ANSI error cases, both interval modes, nulls, literal/column combinations, DST cases and six shuffle queries. Their ordinary query mode compares Spark/Comet answers and requires Comet operators. Constant folding is disabled. This checks operator coverage, but the fixtures do not explicitly assert dispatcher expression names or test dispatcher-off fallback.

Local validation checked all 18 changed files and exact source identities. An eight-case Java component probe confirmed the legacy output boundary using the maintained Spark interval class and source-matched emitted calculations. This was not a full Spark SQL or JNI run. The author's reported suite/profile runs remain author evidence. CI and the Delta gate currently have no jobs and await approval. Only labeling passed. Maintained Spark 3.4 and 4.1 sources were unavailable, so their semantics are not independently qualified here.

Performance

Keeping a projection in Comet can avoid row conversion and preserve native work around the dispatched expression. The new route still pays for expression serialization during planning, first-use compilation or cached-kernel lookup, a JNI/Arrow boundary and output allocation per batch, plus Spark's per-row date arithmetic. These costs matter particularly for inexpensive default date - date and small batches.

No matched benchmark for these new routes is included. Please add focused Spark, Comet dispatcher-on and Comet dispatcher-off measurements for a simple subtraction and a mixed temporal projection, with small/large inputs, verified equal results and executed plans, and separate first-use versus warm timings. Operator coverage and correctness comparisons do not establish a speedup.

Design

Reusing Spark's generated expression tree keeps timezone, ANSI and negation behavior in one implementation. The shim-specific naming change is contained, and the 4.x overrides preserve the inherited expression map. No new native arithmetic kernel or protocol is introduced. The important compatibility boundary is the representation of the result: reusing Spark's calculation alone does not guarantee that a calendar interval can cross the Arrow boundary unchanged.

Abstraction & complexity

The shared CometCodegenDispatch abstraction fits these small registrations. The two identical CometTimeAdd files correspond to existing source-root boundaries, so introducing a new compatibility layer just to remove those copies would add little value. The output-range fix should preserve the legacy interval's components. Moving microseconds into days would change subsequent timezone-sensitive interval arithmetic.

[sunchao]

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 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.

[dwsmith1983]

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.

[sunchao]

Re-reviewed be973f533083 against 8b818b53bd6a, including the changes since the published review of 61345818df27.

The legacy timestamp-subtraction P2 is addressed. The result-type guard returns Unsupported for CalendarIntervalType, and this handler does not implement CodegenDispatchFallback, so the shared serde preserves Spark fallback. Default DayTimeIntervalType results remain dispatched. The new column-based 330-year fixtures cover both signs and both timestamp types, with explicit fallback-reason checks in legacy mode. The DST descriptions are corrected. No remaining or new verified P1/P2 findings.

I rechecked the maintained Spark 3.5/4.0 sources and reran the eight-case arithmetic/output component probe. That probe confirms the writer boundary; it does not execute the new Spark/JNI fallback. Full product CI is still pending: CI and the Delta gate await approval with no jobs. Maintained Spark 3.4/4.1 source qualification and matched performance measurements remain unavailable.

[andygrove]

I checked this out and ran it. All the new fixtures pass on the 4.1 profile, and test-compile is clean on 3.4, 3.5 and 4.0 as well, so all four shim roots are covered.

I also mutated it two ways to be sure the fixtures aren't vacuous. Dropping the five temporalExpressions entries and the 4.1 shim registration fails all eleven cases that run on 4.1, and the two legacy files fail on the specific reason string rather than just "some fallback". Replacing the Unsupported branch with Compatible() makes subtract_timestamps_long_span_legacy.sql abort the stage with ArithmeticException: long overflow, so the 292-year guard is load-bearing and the fixture pins it. Good.

Comments inline. They are all about tying this back to issues that already exist rather than anything I found wrong with the code.

One on the description itself: since legacy interval mode still falls back, could we note that next to closes #3112? Read afterwards, "Support Spark expression: subtract_timestamps" will look fully done when one mode isn't. Same for #5061, whose status table still lists date + interval, ts + interval, ts - ts and date - date as falling back, so that row can be ticked.

[andygrove]

This is the same Math.multiplyExact(interval.microseconds, 1000L) limit that CometMakeInterval documents forty lines up, and that one links #5279. Could this reason string carry the link too? Otherwise when #5279 lands and CalendarInterval crosses the boundary losslessly, there is nothing here pointing at the branch that became removable.

Worth a sentence on why the two take opposite approaches to the same bug, as well. MakeInterval keeps dispatching and documents the limit as a compatible note, this one declines the whole legacy mode. I think declining is right, since MakeInterval only overflows on extreme arguments whereas ts - ts can produce an arbitrary span from ordinary-looking data, and legacy interval mode is off by default so almost nobody pays for it. But someone comparing the two in the same file will wonder.

[dwsmith1983]

Done. The reason string links #5279, and the comment above the branch says why this one declines where CometMakeInterval keeps dispatching: same multiplyExact limit, but make_interval only overflows on extreme arguments while ts - ts produces an arbitrary span from ordinary data, and legacy mode is off by default. It also says to remove the branch once #5279 carries CalendarInterval across losslessly.

[andygrove]

This omission already has a tracking issue, #5058, filed under the interval EPIC #5061. Could the comment cite it, here and in the matching one in subtract_timestamps.sql? Then whoever fixes #5058 has a grep target for the fixtures that can be extended once it lands, and the "will get its own issue" line in the description can go.

[dwsmith1983]

Cited #5058 in both fixture notes, and the description now points at it (and at #5133, which fixes it) instead of promising a new issue.

[andygrove]

This file and the spark-4.0 copy are byte-identical, same blob hash. The description says no source root covers exactly 3.4 through 4.0, but I think shims.minorPlusVerSrc is that root: it is spark-none on 3.4, 3.5 and 4.0 and spark-4.1+ on 4.1 and 4.2, and spark/pom.xml already adds src/main/${shims.minorPlusVerSrc} as a source directory. Giving that property a real directory name on the 3.x and 4.0 profiles would let this live in one place.

If reworking the build isn't worth it for 28 lines, the other way out is to drop the named object and register new CometCodegenDispatch[TimeAdd] inline in the three shim maps. It is a concrete class, so that compiles. Either beats two copies that can drift.

[dwsmith1983]

Took the inline route: the 3.4, 3.5 and 4.0 shims register new CometCodegenDispatch[TimeAdd] directly and both CometTimeAdd.scala copies are gone. Reworking minorPlusVerSrc for 28 lines did not seem worth it, and CometTimestampAddInterval stays as the one object in spark-4.1+, where it has a single home.

[dwsmith1983]

Thanks for the mutation runs; that is the check I wanted on the fixtures. All three inline items are in, and the description now says next to #3112 that legacy interval mode keeps its Spark fallback. The #5061 row for date + interval, ts + interval, ts - ts and date - date can move to done once this merges, with the legacy ts - ts caveat noted on #5058's line.

[andygrove]

Re-checked at a45bb878. The three things I raised are all in, and I re-ran it rather than just reading the diff.

138 datetime fixtures pass on the default 4.1 profile and 138 on 3.5. The default profile does not compile the 3.4, 3.5 or 4.0 shims at all, so the inline new CometCodegenDispatch[TimeAdd] that replaced the duplicated object is not exercised there. I ran 3.5 and then deleted the registration: timestamp_add_interval.sql fails with Project [COMET: timeadd is not supported], so it is load-bearing rather than silently inert. miscExpressions is a val and CometCodegenDispatch holds no state, so the instance is built once at class init and nothing is lost by dropping the named object.

I also read DateTimeUtils on 3.4.3, 3.5.8, 4.0.1 and 4.1.3, which covers the two versions sunchao could not reach. subtractDates builds new CalendarInterval(months, days, 0) on all four, so the microseconds field a legacy date - date writes is always zero and SubtractDates really is safe to dispatch in both modes. The ANSI split is right too. 3.4 and 3.5 throw the plain ansiIllegalArgumentError, 4.0 and 4.1 throw invalidIntervalWithMicrosecondsAdditionError, which matches the MaxSparkVersion and MinSparkVersion split across the two ANSI files.

One thing I went looking for and did not find, worth recording so nobody re-derives it. The dispatcher's calendar-interval writer multiplies microseconds by 1000, and I wanted to confirm the read side divides. It does, in CometPlainVector.IntervalChildVector.getLong. Every calendar interval these fixtures round-trip has microseconds of zero, so a missing divide would have been invisible to them.

Three comments inline. None of them block.

[andygrove]

The + and - notes are broader than what actually happens for a day-granular interval on a date. Spark rewrites (DateType, DayTimeIntervalType(DAY, DAY)) to DateAdd(l, ExtractANSIIntervalDays(r)), in BinaryArithmeticWithDatetimeResolver on 4.0 and 4.1 and in Analyzer.ResolveBinaryArithmetic on 3.4 and 3.5, and CometDateAdd serdes that natively. So date + INTERVAL '30' DAY never reaches the dispatcher, and that is the spelling most real queries use, TPC-DS included.

Could the notes carve it out? Something like "A calendar or year-month interval on a date, and any interval on a timestamp, route through the JVM codegen dispatcher. date +/- INTERVAL '<n>' DAY is rewritten to date_add and stays native. A finer day-time interval on a date is cast to timestamp and dispatched." The * row two lines up already splits by interval type, so it would read consistently.

The other direction is worth a thought as well. For a non-foldable DAY-precision interval column, ExtractANSIIntervalDays has no serde, so the projection falls back rather than dispatching.

[andygrove]

These prove the answers match and the operator stays native, but not that any of it went through the dispatcher, which is the thing the PR is claiming. expect_dispatch(...) already exists for exactly this and is used in math/round.sql and string/upper.sql. CometSqlFileTestSuite accepts it as a file sentinel, so it is a strictly stronger assertion than a plain query rather than an extra one to maintain.

Could the lead query in each file become query expect_dispatch(subtractdates) and so on? It matters most for the legacy ts - ts decline. That branch is only correct because default mode is known to hit the dispatcher's duration writer, and right now nothing pins that. It is also the assertion that would settle the date + interval point I left on the expressions page, since date + make_dt_interval(...) and date + INTERVAL '1' DAY take different routes and these fixtures cannot tell them apart.

timestamp_add_interval.sql is the awkward one, since the name changes from timeadd to timestampaddinterval at 4.1, so leaving that file on a plain query seems fine.

[andygrove]

Reading this right above the CometSubtractTimestamps guard, the obvious question is why this one does not need the same branch when its legacy result is also a CalendarIntervalType. The answer is in subtract_dates.sql but not here, and it is the invariant whoever adds the next calendar-interval-producing serde will need.

Worth a line? Something like "Legacy mode returns a CalendarIntervalType, but DateTimeUtils.subtractDates always sets microseconds to 0, so the dispatcher's multiplyExact cannot overflow and both modes dispatch." I checked that on 3.4.3, 3.5.8, 4.0.1 and 4.1.3 and it holds on all four.