feat: add native make_interval support

[peterxcli]

Which issue does this PR close?

Closes #3099.

Rationale for this change

Comet does not currently support native execution of Spark’s make_interval expression.

What changes are included in this PR?

• Map Catalyst MakeInterval to a native Comet scalar function.
• Use DataFusion’s SparkMakeInterval implementation.
• Preserve Spark ANSI overflow behavior.
• Add ANSI and non-ANSI SQL coverage.

How are these changes tested?

• make core
• cargo check -p datafusion-comet-spark-expr
• ./mvnw spotless:check -DskipTests
• Spark 4.1 CometSqlFileTestSuite tests for make_interval

[andygrove]

Could this use scalarFunctionExprToProtoWithReturnType("make_interval", CalendarIntervalType, expr.failOnError, childExprs: _*) instead? The make_interval name collides with the DataFusion UDF registered in register_datafusion_spark_function, and the sibling CometMakeDate right above sets the return type explicitly for the same reason (see the "When to set the return type explicitly" section of the contributor guide). It currently works because DataFusion's signature happens to line up, but pinning the return type keeps this robust against future signature changes.

[andygrove]

There's a compatibility concern I'd like to flag with the underlying DataFusion kernel. Two related issues:

Nanosecond vs microsecond overflow. Spark's IntervalUtils.makeInterval stores time components as int64 microseconds via secs.toUnscaledLong, so a Decimal(18, 6) seconds value fits comfortably (max ≈ 1e18 micros, well under Long.MaxValue). DataFusion's kernel accumulates in nanoseconds, so it overflows at roughly secs > 9_223_372_036 (~292 years). Any Decimal(18, 6) seconds value beyond that boundary silently returns null under this PR (or throws under ANSI) while Spark returns a valid interval. Spark's own sql-tests/inputs/interval.sql exercises exactly this range:

select make_interval(1, 2, 3, 4, 0, 0, 123456789012.123456);

Float64 coercion loses microsecond precision. DataFusion's SparkMakeInterval signature coerces secs to Float64, but Spark's MakeInterval.inputTypes is Decimal(18, 6) and preserves microseconds exactly. For secs = 999999999.999999, the Float64 round-trip yields frac * 1e9 ≈ 999999046 instead of 999999000 — a ~46 ns drift that translates into a wrong microsecond count on the JVM side. The small values currently in the fixture (7.123456, 100.000001, -1.5) happen to be exactly representable so they don't expose this.

Given both, would it make sense to mark this expression Incompatible(Some("...")) in getSupportLevel, and add a getIncompatibleReasons() string so the auto-generated compat page warns users? Marking it Native in expressions.md with no caveat currently overstates the compatibility.

[andygrove]

Small perf nit for the ANSI path. ColumnarValue::values_to_arrays runs here, and then again inside DataFusion's make_scalar_function when self.inner.invoke_with_args(args) is called. Scalar inputs get expanded to length-N arrays twice.

Also, on line 69–70 the overflow check iterates 0..values.len() unconditionally. For batches with null_count() == 0, this scans the whole range even though there's nothing to detect. A cheap improvement would be to short-circuit when values.null_count() == 0 and, otherwise, walk values.nulls() positions directly.

All ANSI-only, so blast radius is small — happy to defer if you'd rather keep the code simple.

[andygrove]

secs here is decimal(38, 6) but Spark's MakeInterval.inputTypes is Decimal(18, 6). All the values you insert fit either type so this works, but a reader might reasonably assume Decimal(38, 6) inputs are natively supported when in fact Spark casts down to Decimal(18, 6) before calling makeInterval. Consider using decimal(18, 6) here so the fixture matches the actual supported input type.

[andygrove]

Is spark.comet.exec.scalaUDF.codegen.enabled=false actually needed for this fixture? CometMakeInterval extends CometExpressionSerde, not CometCodegenDispatch, so I don't think codegen dispatch is in play here. If it's not load-bearing, dropping the directive would keep the fixture minimal. Same question for make_interval_ansi.sql.

[andygrove]

It might be worth expanding coverage here. A few things Spark's own IntervalExpressionsSuite / interval.sql exercise that aren't covered yet:

• Microsecond-precision seconds like Spark's docstring example make_interval(0, 1, 0, 1, 0, 0, 100.000001) asserted directly (it's currently only exercised via the column path where it can be hard to spot a per-row precision drift).
• Nulls in components other than years in the column path (currently only the years=NULL row is tested).
• Large-second cases from Spark's interval.sql: make_interval(1, 2, 3, 4, 0, 0, 123456789012.123456) and make_interval(0, 0, 0, 0, 0, 0, 1234567890123456789). If either is a known divergence (see the nanos-overflow comment on the Rust file), wrapping them in query ignore(<tracking issue>) would at least pin the behavior for future readers.
• Int.MinValue for a signed-overflow smoke test on the years column.

[andygrove]

Consider adding ANSI overflow tests for components other than years — the current fixture only exercises years = Int.MaxValue. Spark's IntervalExpressionsSuite ANSI mode block covers weeks = Int.MaxValue, and per-row overflow via hours/mins/seconds interactions. Something like:

query expect_error(overflow)
SELECT make_interval(0, 0, 2147483647)

would confirm the overflow detection path fires on non-years components too.

[andygrove]

Thanks for the rework, this is much closer. The explicit return type and the Incompatible marking both look right now, all seven of my earlier comments are addressed, and #5131 is a good tracking issue. I also checked the precision analysis behind which cases you chose to ignore. For 999999999.999999 the Float64 round-trip drifts by about 46 ns, which still floors to the same microsecond on the JVM side, so leaving that one un-ignored is correct. Nice attention to detail.

I traced the ANSI flag end to end to make sure it actually arrives. Because the serde sets the return type, create_scalar_function_expr skips the registry lookup (planner.rs:3337) and goes straight to create_comet_physical_fun, where the "make_interval" arm builds SparkMakeInterval::new(fail_on_error). That path works.

The main open question is the one in my comment on CometMakeInterval, about whether this should mix in CodegenDispatchFallback so users get something by default. The rest is scoped smaller.

One more thing on the description. Could you add microbenchmark numbers? A CometDatetimeExpressionBenchmark case comparing Spark, codegen dispatch and the native kernel would tell us whether the opt-in native path is worth the extra surface, which is the main thing I am unsure about given #5260 covers the compatible behavior.

[andygrove]

One structural thought. Since getSupportLevel is unconditionally Incompatible, the default path here is a full fallback to Spark, which is the same as the behavior today without this PR. Users only benefit if they flip allowIncompatible=true and take on the #5131 divergences.

Would you be open to mixing in CodegenDispatchFallback?

object CometMakeInterval
    extends CometExpressionSerde[MakeInterval]
    with CodegenDispatchFallback {

That routes the non-opt-in Incompatible case through the JVM codegen dispatcher (QueryPlanSerde.scala:874), so the projection stays in the Comet pipeline with exact Spark semantics by default, and your native kernel becomes the fast opt-in. It is the same shape as CometConvertTimezone and CometFromUTCTimestamp above.

This would also let #5260 and this PR land together rather than one replacing the other. I opened #5260 as the codegen-dispatch route before seeing how far this one had come. If you would rather keep them separate I am happy to close #5260 and let you carry the dispatch mixin here.

[andygrove]

The reason attributes the nanosecond overflow to seconds, but hours and minutes hit it too, and at much lower values. Spark's IntervalUtils.makeInterval accumulates microseconds while the DataFusion kernel accumulates nanoseconds, so every time component has a 1000x smaller range.

make_interval(0, 0, 0, 0, 2562048) is enough to show it. Spark computes 2562048 * 3_600_000_000 = 9_223_372_800_000_000 micros and returns a valid interval. The kernel computes 2562048 * 3_600_000_000_000 = 9_223_372_800_000_000_000 nanos, which exceeds i64::MAX, so checked_mul fails and it returns NULL, or throws under ANSI. The cutoffs are hours >= 2,562,048 and mins >= 153,722,868.

Could the reason say "time components (hours, minutes, seconds)" rather than just seconds? This string is what renders on the generated compat page, so it is the only warning a user gets. It would be good to widen #5131's description the same way.

[andygrove]

TryMakeInterval is RuntimeReplaceable and its replacement is MakeInterval(..., failOnError = false), so try_make_interval reaches this handler after ReplaceExpressions. That means this PR enables it too.

Two follow-ons. The try_make_interval row in expressions.md (line 308) still says 🔜 with the #5061 note, so it needs the same update as the make_interval row. And there is a combination neither fixture covers: failOnError = false with spark.sql.ansi.enabled = true, where try_make_interval(2147483647) must return NULL instead of throwing. Would you add a small fixture for it? It needs -- MinSparkVersion: 4.0, since try_make_interval is not registered in 3.5.

[andygrove]

Could you add an hours case alongside the seconds ones? It is the same #5131 nanosecond overflow but on a component the fixture does not touch, and at a value a real query is much more likely to produce than a 12-digit seconds decimal.

query ignore(https://github.com/apache/datafusion-comet/issues/5131)
SELECT make_interval(0, 0, 0, 0, 2562048)

[andygrove]

Would you tighten these to expect_error(ARITHMETIC_OVERFLOW)? Both engines produce that class on every supported profile. Spark 3.5 and 4.1 both route through QueryExecutionErrors.arithmeticOverflowError, and Comet's SparkError::ArithmeticOverflow renders [ARITHMETIC_OVERFLOW] interval overflow. .... The bare overflow would also match an unrelated failure, and ARITHMETIC_OVERFLOW is the more common convention in the existing fixtures.

Note the message bodies still differ. Spark says integer overflow or long overflow depending on which Math.*Exact tripped, Comet always says interval overflow. That is unavoidable given the wrapper only sees the result null mask, but a short comment in make_interval.rs noting it would save the next reader the investigation.

[andygrove]

I do not think this registration is reachable. Because the serde sets the return type, create_scalar_function_expr skips the session_ctx.udf(...) lookup (planner.rs:3337) and goes to create_comet_physical_fun, where the "make_interval" arm builds SparkMakeInterval::new(fail_on_error) directly.

If you want to keep a registry entry for safety, SparkMakeDate is the precedent and it lives in all_scalar_functions() in comet_scalar_funcs.rs. Putting a Comet wrapper here is a little misleading, since everything else in register_datafusion_spark_function is a raw upstream UDF, and this one hardcodes fail_on_error = false via Default. If it ever did get used it would silently ignore ANSI.