Skip to content

feat: support make_interval via codegen dispatch - #5260

Open
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:feat/make-interval-codegen-dispatch
Open

feat: support make_interval via codegen dispatch#5260
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:feat/make-interval-codegen-dispatch

Conversation

@andygrove

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes #3099. Part of the interval support epic #5061.

Split out of #5030, which originally carried this alongside timestampadd and timestampdiff. Those two are unrelated to make_interval beyond sharing the codegen-dispatch mechanism, so they now travel separately.

Note there is an alternative native implementation in #5039, which wires make_interval to DataFusion's SparkMakeInterval. The two approaches are mutually exclusive; see the trade-off below.

Rationale for this change

MakeInterval has no Comet handler today, so any query using make_interval falls the entire operator back to Spark. It is a regular expression, not RuntimeReplaceable, so it reaches serde directly.

This PR routes it through the JVM codegen dispatcher rather than adding a native implementation. The dispatcher runs Spark's own generated code inside the native Comet pipeline, which keeps the operator native while guaranteeing bit-for-bit Spark compatibility. For make_interval specifically this covers three behaviors that are easy to diverge on natively:

  • the seconds argument is a Decimal(18, 6) scaled to microseconds, with its own overflow check;
  • failOnError (defaulting to SQLConf.ansiEnabled) chooses between raising ARITHMETIC_OVERFLOW and returning NULL, and it must be the same exception Spark raises;
  • the years * 12 and weeks * 7 products overflow int independently of one another.

make_interval produces CalendarIntervalType, which Comet's columnar layer gained support for in #4898. With that in place the dispatcher can carry its output.

The trade-off against #5039: codegen dispatch gives exact Spark semantics for free but runs JVM code per batch, so it is slower than a native kernel. A native implementation is the better end state if it can match Spark on the decimal scaling and both ANSI paths. This PR is the low-risk option and can be superseded.

What changes are included in this PR?

  • CometMakeInterval codegen-dispatch serde in datetime.scala, registered in QueryPlanSerde's temporalExpressions map.
  • make_interval.sql and make_interval_ansi.sql Comet SQL file tests.
  • The make_interval row in the Supported Spark Expressions guide.

How are these changes tested?

New Comet SQL file tests run each query through both Spark and Comet, verify the results match, and verify Comet executes the expression through the dispatcher rather than falling back.

make_interval.sql (non-ANSI, so failOnError is false) covers:

  • all seven arguments as columns, and each of the shorter arities filled in by MakeInterval's auxiliary constructors;
  • mixed literal/column arguments and all-literal arguments (the suite disables constant folding, so these still reach Comet);
  • negative and zero components, and values that carry across units (25 months, 80 minutes, 299.889987 seconds);
  • NULL propagation from nullable columns. It has to come from columns rather than NULL literals: MakeInterval is NullIntolerant, so NullPropagation rewrites any call with a literal NULL argument into a null interval literal that never reaches Comet. That literal currently fails natively, tracked separately by Null CalendarInterval literal throws native exception instead of evaluating to null #5058.
  • overflow returning NULL when years * 12 exceeds the int range.

make_interval_ansi.sql pins spark.sql.ansi.enabled=true so failOnError is true, and asserts the overflow raises the same error Spark does for both the years * 12 and weeks * 7 products. Each error query sits alongside valid-input queries in the same file so the case cannot pass vacuously through a fallback.

Verified green on Spark 3.5 and 4.1 locally, along with the pre-existing calendar_interval.sql fixture and the explain comet test in CometExpressionSuite, both of which use make_interval.

MakeInterval has no Comet handler today, so any query using make_interval
falls the whole operator back to Spark. Route it through the JVM codegen
dispatcher, which runs Spark's own generated code inside the native Comet
pipeline and so matches Spark bit-for-bit across all supported versions.

make_interval produces CalendarIntervalType, which Comet's columnar layer
gained support for in apache#4898; that is what makes carrying the dispatcher's
output possible now.

Adds make_interval.sql and make_interval_ansi.sql fixtures, the latter
covering the ANSI overflow path where MakeInterval.failOnError is true and
overflow raises ARITHMETIC_OVERFLOW instead of returning NULL.
@peterxcli

peterxcli commented Aug 4, 2026

Copy link
Copy Markdown
Member

The trade-off against #5039: ... . A native implementation is the better end state .... . This PR is the low-risk option and can be superseded.

Just to clarify — is this planned for 1.0.0 so we can deliver the native implementation without schedule pressure?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could also add test cases which is from the review of native make_interval impl: #5039 (comment)

Spark's own sql-tests/inputs/interval.sql exercises exactly this range:

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other reference: #5039 (comment)

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() would at least pin the behavior for future readers.
  • Int.MinValue for a signed-overflow smoke test on the years column.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto as `make_interval.sql.
ref: #5039 (comment)

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)

@andygrove

Copy link
Copy Markdown
Member Author

The trade-off against #5039: ... . A native implementation is the better end state .... . This PR is the low-risk option and can be superseded.

Just to clarify — is this planned for 1.0.0 so we can deliver the native implementation without schedule pressure?

Hi @peterxcli This work was part of a PR that was created before your PR. We can review both and see what makes sense (maybe even hybrid). Let me know when your PR is ready for another review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Support Spark expression: make_interval

2 participants