feat: support interval types and make_ym_interval / make_dt_interval - #4541
Conversation
…[skip ci] Implements the type-support prerequisite from issue apache#4540: add Spark YearMonthIntervalType and DayTimeIntervalType as physical types that round-trip through Comet's Arrow FFI, and route the make_ym_interval / make_dt_interval constructors through the JVM codegen dispatcher so they execute natively and match Spark exactly. Type plumbing: - proto: add YEAR_MONTH_INTERVAL (18) and DAY_TIME_INTERVAL (19) to DataTypeId. - native serde.rs: map them to Arrow Interval(YearMonth) and Duration(Microsecond) respectively. DayTime stores microseconds in an int64, which matches Duration(Microsecond) rather than the lossy Interval(DayTime) {days, millis}. - Utils.toArrowType / fromArrowType: same mapping on the JVM side. - QueryPlanSerde.serializeDataType: emit the new type ids. - CometBatchKernelCodegen: accept the two interval types in isSupportedDataType and resolve IntervalYearVector / DurationVector; emit primitive set() writes. Expressions: - make_ym_interval -> CometMakeYMInterval, make_dt_interval -> CometMakeDTInterval, both via CometCodegenDispatch, registered in temporalExpressions. CalendarIntervalType and interval arithmetic remain follow-ups under apache#4540. Tests: SQL file tests for both constructors assert answer parity and native execution (checkSparkAnswerAndOperator), covering column and literal inputs, defaults, negatives, and nulls.
# Conflicts: # spark/src/main/scala/org/apache/comet/codegen/CometBatchKernelCodegenOutput.scala
mbutrovich
left a comment
There was a problem hiding this comment.
Mostly just a test request, this is looking good @andygrove!
| case _: YearMonthIntervalType => new ArrowType.Interval(IntervalUnit.YEAR_MONTH) | ||
| // Spark stores DayTimeIntervalType as microseconds in an int64, matching Arrow | ||
| // Duration(Microsecond) rather than the lossy Interval(DayTime) {days, millis} layout. | ||
| case _: DayTimeIntervalType => new ArrowType.Duration(TimeUnit.MICROSECOND) |
There was a problem hiding this comment.
Verified correct, no change needed for this PR. The representation choices match Spark's internal storage exactly: YearMonthIntervalType is int32 months, which maps cleanly to Interval(YearMonth); DayTimeIntervalType is an int64 microsecond count, which round-trips faithfully through Duration(Microsecond). The comment correctly notes that Interval(DayTime)'s {days, millis} layout would lose microsecond precision, so avoiding it is right.
One forward-looking note, not a blocker. Both mappings drop Spark's interval field qualifiers. YearMonthIntervalType(YEAR, YEAR) and YearMonthIntervalType(YEAR, MONTH) both serialize to the same Interval(YearMonth), and on the way back fromArrowType produces the default YearMonthIntervalType() (YEAR TO MONTH); same story for DayTimeIntervalType start/end fields. This is harmless here because the only expressions wired are make_ym_interval and make_dt_interval, both of which Spark types with the default fields (YearMonthIntervalType(), DayTimeIntervalType()), and interval columns are not scanned yet. But once #4540 adds interval scans or field-qualified interval handling, this normalization will change a column's declared type. Worth a tracking note so it is not lost.
| @@ -0,0 +1,35 @@ | |||
| -- Licensed to the Apache Software Foundation (ASF) under one | |||
There was a problem hiding this comment.
The coverage of column, literal, default, negative, and null inputs is good, and using checkSparkAnswerAndOperator (the default query mode) correctly asserts native execution and exercises the interval read-back path.
One gap worth filling: overflow. make_ym_interval computes years * 12 + months through IntervalUtils.makeYearMonthInterval, which uses exact arithmetic and throws ARITHMETIC_OVERFLOW unconditionally (not ANSI-gated); make_dt_interval overflows its int64 microsecond total similarly. Since these route through the codegen dispatcher (Spark's own doGenCode), an overflow case is a good way to confirm the dispatched path propagates Spark's exception identically rather than diverging. Something like:
query expect_error(ARITHMETIC_OVERFLOW)
SELECT make_ym_interval(178956971, 0)Please confirm the exact error token the suite matches. The value 178956971 * 12 exceeds Int.MaxValue, so both engines should raise the same overflow.
| case yi: ArrowType.Interval if yi.getUnit == IntervalUnit.YEAR_MONTH => | ||
| YearMonthIntervalType() | ||
| case di: ArrowType.Interval if di.getUnit == IntervalUnit.DAY_TIME => DayTimeIntervalType() | ||
| case d: ArrowType.Duration if d.getUnit == TimeUnit.MICROSECOND => DayTimeIntervalType() |
There was a problem hiding this comment.
Observation, no change requested. fromArrowType now maps both Interval(DayTime) (line 110) and Duration(Microsecond) (line 111) to DayTimeIntervalType, while toArrowType only ever emits Duration(Microsecond). So the Interval(DayTime) reverse case is dead for Comet-produced data and is presumably kept for externally-produced Arrow. That is fine and consistent with the precision rationale, just flagging the asymmetry in case it was unintentional.
Confirm the codegen-dispatch path propagates Spark's arithmetic-overflow exception identically. The expect_error pattern uses the lowercase word overflow so it matches every Spark version: 4.x raises INTERVAL_ARITHMETIC_OVERFLOW while 3.x raises a raw ArithmeticException.
# Conflicts: # spark/src/main/scala/org/apache/comet/serde/datetime.scala
|
Merged. Thanks @mbutrovich |
…pache#4541) * feat: support interval types and make_ym_interval / make_dt_interval [skip ci] Implements the type-support prerequisite from issue apache#4540: add Spark YearMonthIntervalType and DayTimeIntervalType as physical types that round-trip through Comet's Arrow FFI, and route the make_ym_interval / make_dt_interval constructors through the JVM codegen dispatcher so they execute natively and match Spark exactly. Type plumbing: - proto: add YEAR_MONTH_INTERVAL (18) and DAY_TIME_INTERVAL (19) to DataTypeId. - native serde.rs: map them to Arrow Interval(YearMonth) and Duration(Microsecond) respectively. DayTime stores microseconds in an int64, which matches Duration(Microsecond) rather than the lossy Interval(DayTime) {days, millis}. - Utils.toArrowType / fromArrowType: same mapping on the JVM side. - QueryPlanSerde.serializeDataType: emit the new type ids. - CometBatchKernelCodegen: accept the two interval types in isSupportedDataType and resolve IntervalYearVector / DurationVector; emit primitive set() writes. Expressions: - make_ym_interval -> CometMakeYMInterval, make_dt_interval -> CometMakeDTInterval, both via CometCodegenDispatch, registered in temporalExpressions. CalendarIntervalType and interval arithmetic remain follow-ups under apache#4540. Tests: SQL file tests for both constructors assert answer parity and native execution (checkSparkAnswerAndOperator), covering column and literal inputs, defaults, negatives, and nulls. * test: add overflow cases for make_ym_interval and make_dt_interval Confirm the codegen-dispatch path propagates Spark's arithmetic-overflow exception identically. The expect_error pattern uses the lowercase word overflow so it matches every Spark version: 4.x raises INTERVAL_ARITHMETIC_OVERFLOW while 3.x raises a raw ArithmeticException.
PR #4541 enabled the YearMonthIntervalType and DayTimeIntervalType constructors via codegen dispatch. Update the expression support list to reflect that make_dt_interval and make_ym_interval are now supported.
peterxcli
left a comment
There was a problem hiding this comment.
@andygrove I notice that shuffle and array or nested support for YearMonthInterval and DayTimeInterval hasn't done yet.
26/07/20 17:26:13 WARN CometExecRule: Comet cannot execute some parts of this plan natively (set spark.comet.explainFallback.enabled=false to disable this logging):
CollectLimit
+- Project
+- Exchange [COMET: unsupported shuffle data type YearMonthIntervalType(0,1) for input i#11, Comet columnar shuffle not enabled]
+- CometProject
+- CometNativeScan parquet+- LocalTableScan [COMET: Unsupported array element of type YearMonthIntervalType(0,1)]
Which issue does this PR close?
Part of #4540.
Rationale for this change
Comet had no support for Spark's interval data types, so every expression producing or consuming an interval forced a fallback to Spark, and the JVM codegen dispatcher (
CometCodegenDispatch) could not cover them either becauseCometBatchKernelCodegen.isSupportedDataTyperejected interval output. This PR adds the foundational type support and uses the existing codegen-dispatch mechanism to make the interval constructors run natively while matching Spark exactly.What changes are included in this PR?
Type support for
YearMonthIntervalTypeandDayTimeIntervalType:native/proto/src/proto/types.proto: addYEAR_MONTH_INTERVAL(18) andDAY_TIME_INTERVAL(19) toDataTypeId.native/core/src/execution/serde.rs: map them to ArrowInterval(YearMonth)andDuration(Microsecond).Utils.toArrowType/fromArrowType: same mapping on the JVM side.QueryPlanSerde.serializeDataType: emit the new type ids.CometBatchKernelCodegen: accept both interval types inisSupportedDataType, resolveIntervalYearVector/DurationVector, and emit primitiveset()writes.Representation note: Spark stores
DayTimeIntervalTypeas microseconds in anint64, which round-trips faithfully through ArrowDuration(Microsecond). ArrowInterval(DayTime)uses a{days, millis}layout that would lose microsecond precision, so it is intentionally not used.Expressions (via
CometCodegenDispatch, registered intemporalExpressions):make_ym_interval->CometMakeYMIntervalmake_dt_interval->CometMakeDTIntervalOut of scope (follow-ups tracked in #4540):
CalendarIntervalType, and interval arithmetic /multiply_*_interval/extractof interval fields.How are these changes tested?
SQL file tests under
spark/src/test/resources/sql-tests/expressions/datetime/(make_ym_interval.sql,make_dt_interval.sql) run byCometSqlFileTestSuite. Thequerymode usescheckSparkAnswerAndOperator, asserting both answer parity with Spark and native execution (a fallback fails the test). Cases cover column and literal inputs, default arguments, negatives, and nulls. The full SQL suite shows no new regressions.