Repository navigation
feat: add CalendarIntervalType support - #4898
Conversation
|
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 A few questions, none blocking:
Also heads up: the red This review was prepared with the assistance of an LLM (Claude). |
…o feat/calendar-interval-type
| _: Float4Vector | _: Float8Vector | _: DecimalVector | _: VarCharVector | | ||
| _: VarBinaryVector | _: DateDayVector | _: TimeStampMicroVector | | ||
| _: TimeStampMicroTZVector => | ||
| _: TimeStampMicroTZVector | _: IntervalMonthDayNanoVector => |
There was a problem hiding this comment.
self reminder: move this to CometLiteral.getSupportLevel after #4715 is merged.
Seems like |
andygrove
left a comment
There was a problem hiding this comment.
LGTM. Thanks @peterxcli. Could you fix conflicts?
|
@andygrove Thanks for the review! |
CalendarIntervalType support landed in apache#4898, so MakeInterval can now go through the JVM codegen dispatcher alongside TimestampAdd and TimestampDiff instead of falling the projection back to Spark. Also broadens the datetime test coverage: - timestampadd gains MILLISECOND and DAYOFYEAR, a TIMESTAMP_NTZ column (a separately compiled kernel, since TimestampAdd.dataType follows the input), DST spring-forward, fall-back and nonexistent-local-hour cases, and a DATETIME_OVERFLOW expect_error case. - timestampdiff gains MICROSECOND, MILLISECOND and QUARTER, NTZ inputs, and the matching DST cases. - The grammar routes DATEADD/DATE_ADD and DATEDIFF/DATE_DIFF/TIMEDIFF to TimestampAdd/TimestampDiff when the first argument is a datetimeUnit keyword. Those alias spellings are now covered, split by the version that introduced them: dateadd/datediff inline, date_add/date_diff in date_add_unit_alias.sql (Spark 3.5+), timediff in timediff.sql (4.0+). - make_interval and make_interval_ansi fixtures, the latter covering the ANSI overflow path where MakeInterval.failOnError is true. Corrects the date_add, date_diff, dateadd and datediff rows in the expression guide, which claimed Native without noting that this is true only of the two-argument form, and adds rows for the unit spellings.
* Add CalendarIntervalType Arrow support * Support interval vectors in UDF and shuffle codegen * remove stale test * spotless apply
…val expressions Restore CalendarIntervalType in QueryPlanSerde.supportedDataType (added deliberately by apache#4898; removing it narrowed hash, scalar-subquery, and nested-type gates out of scope for this PR) and revert CometSink to main, since its isTypeSupported OR existed only to patch sinks around that removal. CometLiteral now only adds YearMonthIntervalType alongside the existing DayTimeIntervalType arm. Move MultiplyYMInterval next to MultiplyDTInterval in the expression map's interval cluster. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Untyped constructors such as map(), map('a', NULL) and array() leave
NullType children in their output type, and the codegen dispatch gate
rejected any output type containing NullType, so the whole operator
fell back to Spark.
Make the type gate asymmetric: canHandle now accepts NullType (top-level
or nested in array/struct/map) for the output type while still rejecting
it for BoundReference inputs, since CometScalaUDFCodegen.specFor cannot
build an ArrowColumnSpec for a NullVector. The output emitter maps
NullType to NullVector and writes it with setNull only.
Also update the Scala/Java UDF guide: NullType arguments remain
unsupported, NullType return types are now supported; CalendarIntervalType
is removed from the unsupported list since it has been supported since
apache#4898.
Closes apache#5525
Assisted-by: Claude Code (claude-fable-5)
Untyped constructors such as map(), map('a', NULL) and array() leave
NullType children in their output type, and the codegen dispatch gate
rejected any output type containing NullType, so the whole operator
fell back to Spark.
Make the type gate asymmetric: canHandle now accepts NullType (top-level
or nested in array/struct/map) for the output type while still rejecting
it for BoundReference inputs, since CometScalaUDFCodegen.specFor cannot
build an ArrowColumnSpec for a NullVector. The output emitter maps
NullType to NullVector and writes it with setNull only.
Also update the Scala/Java UDF guide: NullType arguments remain
unsupported, NullType return types are now supported; CalendarIntervalType
is removed from the unsupported list since it has been supported since
apache#4898.
Closes apache#5525
Assisted-by: Claude Code (claude-fable-5)
Untyped constructors such as map(), map('a', NULL) and array() leave
NullType children in their output type, and the codegen dispatch gate
rejected any output type containing NullType, so the whole operator
fell back to Spark.
Make the type gate asymmetric: canHandle now accepts NullType (top-level
or nested in array/struct/map) for the output type while still rejecting
it for BoundReference inputs, since CometScalaUDFCodegen.specFor cannot
build an ArrowColumnSpec for a NullVector. The output emitter maps
NullType to NullVector and writes it with setNull only.
Also update the Scala/Java UDF guide: NullType arguments remain
unsupported, NullType return types are now supported; CalendarIntervalType
is removed from the unsupported list since it has been supported since
apache#4898.
Closes apache#5525
Assisted-by: Claude Code (claude-fable-5)
Untyped constructors such as map(), map('a', NULL) and array() leave
NullType children in their output type, and the codegen dispatch gate
rejected any output type containing NullType, so the whole operator
fell back to Spark.
Make the type gate asymmetric: canHandle now accepts NullType (top-level
or nested in array/struct/map) for the output type while still rejecting
it for BoundReference inputs, since CometScalaUDFCodegen.specFor cannot
build an ArrowColumnSpec for a NullVector. The output emitter maps
NullType to NullVector and writes it with setNull only.
Also update the Scala/Java UDF guide: NullType arguments remain
unsupported, NullType return types are now supported; CalendarIntervalType
is removed from the unsupported list since it has been supported since
apache#4898.
Closes apache#5525
Assisted-by: Claude Code (claude-fable-5)
Untyped constructors such as map(), map('a', NULL) and array() leave
NullType children in their output type, and the codegen dispatch gate
rejected any output type containing NullType, so the whole operator
fell back to Spark.
Make the type gate asymmetric: canHandle now accepts NullType (top-level
or nested in array/struct/map) for the output type while still rejecting
it for BoundReference inputs, since CometScalaUDFCodegen.specFor cannot
build an ArrowColumnSpec for a NullVector. The output emitter maps
NullType to NullVector and writes it with setNull only.
Also update the Scala/Java UDF guide: NullType arguments remain
unsupported, NullType return types are now supported; CalendarIntervalType
is removed from the unsupported list since it has been supported since
apache#4898.
Closes apache#5525
Assisted-by: Claude Code (claude-fable-5)
Untyped constructors such as map(), map('a', NULL) and array() leave
NullType children in their output type, and the codegen dispatch gate
rejected any output type containing NullType, so the whole operator
fell back to Spark.
Make the type gate asymmetric: canHandle now accepts NullType (top-level
or nested in array/struct/map) for the output type while still rejecting
it for BoundReference inputs, since CometScalaUDFCodegen.specFor cannot
build an ArrowColumnSpec for a NullVector. The output emitter maps
NullType to NullVector and writes it with setNull only.
Also update the Scala/Java UDF guide: NullType arguments remain
unsupported, NullType return types are now supported; CalendarIntervalType
is removed from the unsupported list since it has been supported since
apache#4898.
Closes apache#5525
Assisted-by: Claude Code (claude-fable-5)
…spatch (apache#4715) * Support multiply_ym_interval with YearMonth interval codegen dispatch * Fix markdown formatting for interval docs * Fix markdown formatting for interval docs * review by andy * fix md format * restructure the CometLiteral.getSupportLevel * Update datatypes.md * review suggestion * fix style * minor change * md prettier * Use DataTypeSupport for sink type checks * address review: keep CalendarIntervalType support intact, group interval expressions Restore CalendarIntervalType in QueryPlanSerde.supportedDataType (added deliberately by apache#4898; removing it narrowed hash, scalar-subquery, and nested-type gates out of scope for this PR) and revert CometSink to main, since its isTypeSupported OR existed only to patch sinks around that removal. CometLiteral now only adds YearMonthIntervalType alongside the existing DayTimeIntervalType arm. Move MultiplyYMInterval next to MultiplyDTInterval in the expression map's interval cluster. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: admit year-month intervals in the list-literal gate Merging upstream/main brought in `listLiteralElementSupported`, which `CometLiteralSuite` pins against `makeListLiteral`'s arms in both directions. This branch had already added a `YearMonthIntervalType` arm to the encoder, so after the merge the two sides disagreed and the pin failed. Widen the gate rather than drop the encoder arm: `serializeDataType` already maps the type and recurses through `ArrayType`, and `literal_to_array_ref` already reads the ints back as an `IntervalYearMonthArray`. An `array<interval year to month>` literal is now serialized directly instead of sending the whole projection back to Spark with "Unsupported data type ArrayType(YearMonthIntervalType(0,1),true)". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: expect native execution for a null year-month interval in abs.sql `abs.sql` pinned `SELECT abs(CAST(NULL AS INTERVAL YEAR TO MONTH))` as a fallback because CometLiteral did not admit YearMonthIntervalType literals, noting that it "flips to a failure when it is fixed". This branch adds that literal support, so the query now runs natively and the pinned fallback failed in every [expressions] CI job (Spark 3.4, 3.5, 4.0, 4.1, 4.2). Assert the answer and native operators instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: keep the element type for empty nested list literal children When every child of a nested list literal level is empty or null, the decoder built an empty array of the child list type itself instead of its element type, adding a list level. Concatenating that with a populated sibling failed, e.g. array(array(CAST(array() AS ARRAY<INTERVAL YEAR TO MONTH>)), array(array(INTERVAL '1' MONTH))), which year-month interval literals now reach. Build the empty values array with the element type and cover both a unit case and the constant-folded query. * fix: give empty nested list literal levels the field types of populated ones Spark folds array(array(array()), array(array(array(INTERVAL '1' MONTH)))) into a literal whose arrays all declare non-nullable elements. literal_to_array_ref rebuilds every populated level with nullable fields, but built an all-empty level from the declared element type, so the two siblings differed in nested field nullability and Arrow refused to concatenate them, failing native planning. Build the empty values through the same recursion a populated child takes, so both always agree. Cover it with a decoder unit test and a constant-folded CometLiteralSuite query, each with an interval and an int leaf. * test: say the nested empty-array fixture query is not constant-folded CometSqlFileTestSuite excludes ConstantFolding, so only the inner array() in that query is a literal and the outer arrays run as native make_array. The folded form is covered by CometLiteralSuite. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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?
CalendarIntervalTypeto ArrowInterval(MonthDayNano).CALENDAR_INTERVALto the serialized data type protocol and native Arrow type conversion.CalendarIntervalTypesupport to Comet batch-kernel code generation for scalar and nested values.How are these changes tested?
Added coverage for:
CalendarIntervalTypethrough the Arrow writer andCometPlainVector.