feat: support multiply_ym_interval with YearMonth interval codegen dispatch

[peterxcli]

Which issue does this PR close?

Close #3102.

Rationale for this change

This adds support for Spark's multiply_ym_interval expression, allowing YearMonth interval multiplication to run through Comet instead of falling back to
Spark.

What changes are included in this PR?

• Add YearMonthIntervalType serialization to the Comet proto/native type mapping.
• Route MakeYMInterval and MultiplyYMInterval through the JVM codegen dispatcher.
• Add IntervalYearVector input/output support in the codegen kernel path.
• Add YearMonth interval literal support.
• Update expression/datatype support docs.
• Add SQL coverage for YearMonth interval multiplication with integer, long, float, double, and decimal multipliers.

How are these changes tested?

• cargo fmt
• ./mvnw -Pspark-3.5 -Pscala-2.12 -DskipTests compile
• cargo +1.94.0 check -p datafusion-comet
• cargo +1.94.0 build
• ./mvnw test -Pspark-3.5 -Pscala-2.12 -Dtest=none -Dsuites="org.apache.comet.CometSqlFileTestSuite multiply_ym_interval"

[andygrove]

Thanks for this. Using the codegen dispatcher for multiply_ym_interval is the right call, since it runs Spark's own generated code and so matches Spark by construction, including the half-up rounding and the overflow behavior. The multiplier coverage in the SQL test is nice and broad (int, long, float, double, decimal, plus the half-up rounding case and the numeric-on-the-left normalization).

A few things worth looking at:

Rebase on main. This looks like it was branched before #4541 landed, and #4541 already added the YearMonth and DayTime interval type plumbing plus make_ym_interval via codegen dispatch. After a rebase, these pieces here become duplicates and will conflict: types.proto, serde.rs, Utils.scala, serializeDataType, isSupportedDataType, the CometBatchKernelCodegenOutput changes, CometMakeYMInterval and its registration, and the make_ym_interval row in expressions.md. Stripping those would leave just the multiply-specific work: MultiplyYMInterval, the interval input support in CometBatchKernelCodegenInput (which #4541 did not touch and which multiply genuinely needs), the interval literals in literals.scala and planner.rs, and the test. That would make the diff much easier to review. It should also clear the failing preflight, which is currently tripping on installation.md, a file this PR does not touch, which is a symptom of the branch being stale.

Is the supportedDataType change needed? #4541 deliberately kept intervals out of QueryPlanSerde.supportedDataType because codegen dispatch does not consult it (it serializes the return type via serializeDataType). Adding YearMonthIntervalType there is a broader claim: it tells Comet that YM interval columns can flow through any native operator (scan, filter, project, sort, shuffle), not just the dispatch path, and nothing here exercises those paths. Is it actually required for multiply_ym_interval? If it is, a test that pushes a YM interval column through a plain projection or a shuffle would give confidence the FFI round-trip is correct. If it is not, dropping it keeps the scope tight.

Overflow test. MultiplyYMInterval always throws on overflow via multiplyExact / toIntExact / intValueExact, independent of ANSI mode. The dispatched code should raise the same exception, but it is not covered. Could we add an expect_error case near Int.MaxValue to confirm Comet surfaces the same error as Spark rather than falling back or wrapping? A null-interval input case such as make_ym_interval(NULL, m) * 2 would also round out the null coverage, which currently only exercises null multipliers.

One small heads-up: the make_ym_interval row in expressions.md also changes in #4790, so expect a conflict there.

On benchmarks: since this runs in the JVM via dispatch rather than as a native Rust kernel, I do not think the usual native microbenchmark expectation applies here.

[peterxcli]

@andygrove Thanks for the review! I've move the YearMonthIntervalType branch from QueryPlanSerde's to CometLiteral's supportedDataType, and also added the overflow sql test. Please take another look, thanks!

[andygrove]

Thanks for the rebase, this is much cleaner to review now, and the three points from last round are all addressed well. Moving the YM interval support into CometLiteral rather than the global supportedDataType is exactly right, and the overflow and null-interval test cases are good additions. I traced the literal path and it is consistent: the literal round-trips as an IntervalYearMonth array through planner.rs into the kernel and is read via getInt, and the literal-input query exercises it.

One thing before this is good to go: the datatypes.md change flips DayTimeIntervalType to supported, but this PR is YearMonth-only and DayTime multiply/divide are still open (#4900, #4901), so DayTime is not fully supported yet. That also contradicts this PR's own expressions.md note ("DayTime/Calendar interval forms fall back") and the "Interval types fall back to Spark today" sentence just above the table. Could you flip only YearMonthIntervalType and leave DayTime at the pending marker, and adjust that intro line?

Minor, non-blocking: a one-line comment in CometLiteral.getSupportLevel noting that YM interval is deliberately kept out of supportedDataType (so it does not get claimed as flowing through arbitrary native operators) would help the next reader.

This review was prepared with the assistance of an LLM (Claude).

[peterxcli]

@andygrove thanks for the review, addressed all of your suggestion! please take another look.

One thing before this is good to go: the datatypes.md change flips DayTimeIntervalType to supported, but this PR is YearMonth-only and DayTime multiply/divide are still open

The DayTimeIntervalType is supported along with #4541, what hasn't really support is * or / expression for it(which is documented in the expression.md.

Could you flip only YearMonthIntervalType and leave DayTime at the pending marker, and adjust that intro line?

I adjust the note to say: "Calendar interval fallback to spark now" in datatypes.md

Minor, non-blocking: a one-line comment in CometLiteral.getSupportLevel noting that YM interval is deliberately kept out of supportedDataType (so it does not get claimed as flowing through arbitrary native operators) would help the next reader.

done. good catch!

[peterxcli]

Moved CalendarIntervalType under CometLiteral#getSupportLevel

[andygrove]

Note on this review: this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled.

The expression wiring itself is straightforward and the SQL fixture covers a good range of multiplier types. Three things concern me, and two of them are about changes that are not in the description.

Removing CalendarIntervalType from QueryPlanSerde.supportedDataType

-        _: DecimalType | _: DateType | _: BooleanType | _: NullType | CalendarIntervalType =>
+        _: DecimalType | _: DateType | _: BooleanType | _: NullType =>

supportedDataType gates sinks, native and columnar shuffle eligibility, local table scans, and literal support. Dropping CalendarIntervalType from it narrows all of those at once, and none of that is mentioned in the "What changes are included" list. The comment in literals.scala explains the intent, but the blast radius deserves its own discussion and its own tests. What breaks today that this is fixing, and what did you check does not regress?

This also collides with #5025, which is refactoring these predicates and relies on CalendarIntervalType staying in that base arm. Whichever lands first will make the other's behavior different from what its author expects. Worth coordinating with that PR.

supportedSinkDataType now ORs two different predicates

case _ => supportedDataType(dt) || isTypeSupported(dt, "", ListBuffer.empty)

Passing an empty name and a throwaway ListBuffer means any fallback reason isTypeSupported produces is discarded, so a user who does hit an unsupported type at a sink gets a less specific message. Mixing DataTypeSupport into CometSink also pulls that trait's config-dependent behavior into sink eligibility, which is a wider change than "add year-month intervals". Could the sink predicate stay a single, explicit function?

Duplicate work with #5161

Both this PR and #5161 add the same two arms to planner.rs: Interval(YearMonth) in the null-literal match and in the IntVal match, with almost the same error message. They will conflict on merge and one of them will have to be rebased away. Since #5161 is about scan support and this one is about an expression, it might be cleanest to land the shared planner.rs and literals.scala interval plumbing in one place and rebase the other on top.

Two smaller notes

classOf[MultiplyYMInterval] goes in between MonthsBetween and Minute in the expression map. That map is broadly alphabetical, so it belongs after MonthsBetween but the Minute entry that follows is already out of order too. Worth fixing both while you are there.

The description lists only -Pspark-3.5 runs. The codegen dispatcher's IntervalYearVector handling is Comet's own per-version code and Comet supports 3.4 through 4.1+. Did the fixture pass on 3.4 and 4.1 as well?

[peterxcli]

@andygrove Thanks for the careful pass — pushed a2349c5 addressing it. Point by point:

Removing CalendarIntervalType from supportedDataType / the supportedSinkDataType OR — good catch, and these two were really one mistake. CalendarIntervalType was deliberately added to supportedDataType by #4898 with test coverage, and removing it narrowed the hash-expression, scalar-subquery, and nested-type gates — all out of scope for this PR. The || isTypeSupported(...) in CometSink existed only to patch calendar intervals back into sinks after that removal (main already has an explicit YM/DT arm there). I've restored supportedDataType and reverted CometSink to main exactly, so this PR no longer touches either predicate and the conflict with #5025 goes away. CometLiteral now only adds YearMonthIntervalType alongside the existing DayTimeIntervalType arm.

Duplicate planner.rs arms with #5161 — confirmed the overlap; the two arms are semantically identical in both PRs. This PR needs them for its literal-input test (INTERVAL '1-2' YEAR TO MONTH * i), so I'd propose whichever merges first wins and the other rebases — it's a mechanical one-hunk resolution. Happy to rebase this one if #5161 lands first.

Expression map ordering — that section of the map is grouped by function family rather than alphabetically (e.g. Hour after LastDay, TimestampAdd in the middle of the M's), so instead of alphabetizing I moved MultiplyYMInterval to sit with the interval cluster, directly after MultiplyDTInterval, and left Minute as is.

Version coverage — ran CometSqlFileTestSuite locally on -Pspark-3.4, -Pspark-3.5, and the default Spark 4.1 profile; all green, including this fixture and #4898's calendar_interval.sql. The overflow case is version-stable by construction: the dispatched code throws the JDK's own Math.multiplyExact "integer overflow" ArithmeticException on every Spark version, which is what expect_error(overflow) matches.