Skip to content

feat: support timestampadd and timestampdiff via codegen dispatch#5030

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

feat: support timestampadd and timestampdiff via codegen dispatch#5030
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:feat/datetime-codegen-dispatch

Conversation

@andygrove

Copy link
Copy Markdown
Member

Which issue does this PR close?

There is no dedicated issue. This is part of the ongoing effort to provide native support for expressions that currently force a full fallback to Spark. Related to the expression coverage epic #240.

Rationale for this change

timestampadd and timestampdiff have no Comet handler today, so any query using them falls the entire operator back to Spark. Both are regular expressions (present since Spark 3.3), not RuntimeReplaceable, so they reach serde directly.

Rather than a native implementation, this PR routes them through the JVM codegen dispatcher. 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 across all supported versions. This avoids the datetime edge-case divergences (timezone and calendar handling) that a chrono-based native implementation would be prone to.

What changes are included in this PR?

  • CometTimestampAdd and CometTimestampDiff codegen-dispatch serdes in datetime.scala, registered in QueryPlanSerde's temporalExpressions map.
  • Comet SQL file tests timestampadd.sql and timestampdiff.sql covering all time units, negative quantities, month-end and leap-day rollover, whole-unit truncation, and NULL inputs.
  • Support status for both functions added to the Supported Spark Expressions guide.

make_interval was considered but left for a follow-up: its output type is CalendarIntervalType, which Comet's columnar layer does not yet support (the dispatcher reports unsupported output type CalendarIntervalType and falls back). It can be enabled once CalendarIntervalType support lands (#4898).

How are these changes tested?

New Comet SQL file tests run each query through both Spark and Comet and verify the results match and that Comet executes the expression natively (through the dispatcher) rather than falling back. Coverage includes every supported time unit, positive and negative quantities, month-end and leap-day boundaries, whole-unit truncation toward zero, and NULL inputs.

Route TimestampAdd and TimestampDiff through the JVM codegen dispatcher so
they run inside the native Comet pipeline instead of forcing a full operator
fallback to Spark. The dispatcher executes Spark's own generated code, so
results match Spark exactly across all supported versions and avoid the
datetime incompatibilities that a chrono-based native implementation would
risk.

make_interval was considered but is left for a follow-up: its output type
CalendarIntervalType is not yet supported by Comet's columnar layer, so the
dispatcher cannot produce it.
@andygrove andygrove added this to the 1.0.0 milestone Jul 24, 2026
@peterxcli

peterxcli commented Jul 25, 2026

Copy link
Copy Markdown
Member

make_interval was considered but left for a follow-up: its output type is CalendarIntervalType, which Comet's columnar layer does not yet support (the dispatcher reports unsupported output type CalendarIntervalType and falls back). It can be enabled once CalendarIntervalType support lands (#4898).

I think you're refer to #3099? I saw the native SparkMakeInterval implementation is ready and registered as comet udf, what we need to do is adding a new catalyst mapping classOf[MakeInterval] -> CometMakeInterval and CometMakeInterval scala implementation to convert expr to make_interval proto?

@peterxcli peterxcli left a comment

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.

LGTM, only need to merge upstream and resolve conflict in expressions.md.

ai review: non-blocker review:

  1. add MILLISECOND to both tests and MICROSECOND to timestampdiff.
  2. Add one TIMESTAMP_NTZ or DST-boundary case; current tests only exercise timestamp-LTZ away from DST transitions.

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.

2 participants