Skip to content

feat: add CalendarIntervalType support#4898

Open
peterxcli wants to merge 7 commits into
apache:mainfrom
peterxcli:feat/calendar-interval-type
Open

feat: add CalendarIntervalType support#4898
peterxcli wants to merge 7 commits into
apache:mainfrom
peterxcli:feat/calendar-interval-type

Conversation

@peterxcli

Copy link
Copy Markdown
Member

Which issue does this PR close?

Part of #4540.

Rationale for this change

Comet does not currently support Spark's CalendarIntervalType, causing queries carrying calendar interval columns through Comet operators to fall back to Spark.

This change adds the foundational Arrow and codegen support needed to preserve calendar intervals—months, days, and microseconds—across the Spark, Arrow, and native execution boundaries.

What changes are included in this PR?

  • Map Spark CalendarIntervalType to Arrow Interval(MonthDayNano).
  • Add CALENDAR_INTERVAL to the serialized data type protocol and native Arrow type conversion.
  • Support reading and writing calendar intervals through Arrow vectors, including microsecond-to-nanosecond conversion.
  • Add CalendarIntervalType support to Comet batch-kernel code generation for scalar and nested values.
  • Allow calendar interval columns through Comet’s data type support checks and query plan serialization.
  • Add SQL and unit-test coverage for nullable and non-null calendar interval values.

How are these changes tested?

Added coverage for:

  • Round-tripping CalendarIntervalType through the Arrow writer and CometPlainVector.
  • Round-tripping calendar intervals through the batch-kernel codegen path.
  • SQL queries containing positive, negative, mixed-component, and null calendar interval values with native local table scans enabled.

@peterxcli peterxcli changed the title Add CalendarIntervalType support feat: add CalendarIntervalType support Jul 12, 2026
@andygrove

Copy link
Copy Markdown
Member

Nice, clean approach to interval support. I traced the micros-to-nanos conversion through all four paths (both writers, both readers) and they are consistent, and the child-vector getInterval chain and slicing both look correct. Limiting the scope to pass-through with no native arithmetic feels right.

A few questions, none blocking:

  • Does an interval column survive a native Comet shuffle? The tests cover localTableScan and codegen round-trips but not an exchange, and this PR makes the type eligible for shuffle. A shuffle test, or an explicit note that shuffle is out of scope for this PR, would help.
  • The codegen adds struct-field and array-element getInterval cases, but the tests are all scalar. Could you add a nested (struct or array) interval test to cover those branches?
  • The codegen output uses Math.multiplyExact(micros, 1000L), which throws on overflow, while the ArrowWriter path relies on Spark's writer, which multiplies without the exact check. For an interval whose microseconds exceed what Arrow's int64 nanoseconds can hold, one path throws and the other wraps. Worth a short comment on the intended behavior for that (rare) case.

Also heads up: the red rust-test check is a runner disk-space flake (No space left on device during linking), not a real failure, so it just needs a re-run.

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

_: Float4Vector | _: Float8Vector | _: DecimalVector | _: VarCharVector |
_: VarBinaryVector | _: DateDayVector | _: TimeStampMicroVector |
_: TimeStampMicroTZVector =>
_: TimeStampMicroTZVector | _: IntervalMonthDayNanoVector =>

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

self reminder: move this to CometLiteral.getSupportLevel after #4715 is merged.

@peterxcli

peterxcli commented Jul 20, 2026

Copy link
Copy Markdown
Member Author

The codegen output uses Math.multiplyExact(micros, 1000L), which throws on overflow, while the ArrowWriter path relies on Spark's writer, which multiplies without the exact check. For an interval whose microseconds exceed what Arrow's int64 nanoseconds can hold, one path throws and the other wraps. Worth a short comment on the intended behavior for that (rare) case.

Seems like ArrowWriter use Math.multiplyExact, too. I think, so they're actually consistent on the overflow exception.

private[arrow] class IntervalMonthDayNanoWriter(val valueVector: IntervalMonthDayNanoVector)
extends ArrowFieldWriter {
override def setNull(): Unit = {
valueVector.setNull(count)
}
override def setValue(input: SpecializedGetters, ordinal: Int): Unit = {
val ci = input.getInterval(ordinal)
valueVector.setSafe(count, ci.months, ci.days, Math.multiplyExact(ci.microseconds, 1000L))
}
}

@andygrove andygrove 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. Thanks @peterxcli. Could you fix conflicts?

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