[SPARK-58005][SQL] Add an opt-in lossless Arrow struct representation for CalendarInterval - #57088
[SPARK-58005][SQL] Add an opt-in lossless Arrow struct representation for CalendarInterval#57088viirya wants to merge 2 commits into
Conversation
… for CalendarInterval The default Arrow mapping for CalendarInterval (IntervalMonthDayNano) multiplies the microseconds component by 1000 into Arrow's int64 nanosecond field, so any |microseconds| > Long.MaxValue / 1000 (roughly +/-292 years) cannot be represented, while Spark permits the full Long range. The mapping must stay as-is for interchange, but internal Arrow storage needs the full domain. Following SPARK-57975's pattern, this extends the opt-in lossless encoding (the flag is renamed from losslessTimestampNanos to losslessInternalTypes since it now covers both kinds of types) to map CalendarInterval to a struct of (months: int32, days: int32, microseconds: int64) -- the type's own field layout, mirroring the default in-memory cache's CALENDAR_INTERVAL ColumnType -- with no unit conversion and hence no overflow. The struct is tagged through child field metadata and is self-describing on read via fromArrowField, ArrowWriter, and ArrowColumnVector, including nested inside arrays, structs, and maps. Additionally, IntervalMonthDayNanoWriter now translates the Math.multiplyExact overflow into the structured DATETIME_OVERFLOW at the conversion site (the same pattern as the nanosecond timestamp writers), so interchange paths that keep the standard encoding raise a clear error instead of a raw "long overflow" ArithmeticException, and the translation cannot re-label unrelated arithmetic failures from lazily-evaluated upstream input. Co-authored-by: Claude Code
sunchao
left a comment
There was a problem hiding this comment.
Summary
Prior state and problem
Spark's CalendarInterval permits the full Long microsecond domain, while Arrow's standard IntervalMonthDayNano representation packs the sub-day component into an int64 nanosecond value. That conversion cannot represent intervals beyond roughly +/-292 years and previously exposed a raw ArithmeticException on overflow.
Design approach
This adds an opt-in internal struct encoding of (months: int32, days: int32, microseconds: int64), tags it so the logical CalendarIntervalType can be recovered on read, and adds dedicated writer/vector-reader handling. The standard Arrow interchange representation remains the default. The default writer's overflow is also translated at the exact multiplication site into structured DATETIME_OVERFLOW.
Correctness / compatibility analysis
I reviewed the schema generation and recovery, writer dispatch, vector readback, nested array/struct/map paths, null handling, metadata preservation, full positive/negative component boundaries, and default interchange behavior. The physical representation is coherent end to end, and existing callers continue to use the standard representation because the new flag defaults to false.
The losslessTimestampNanos to losslessInternalTypes rename does not change JVM descriptors or Scala default-getter signatures, ArrowUtils is private to SQL, and no stale in-tree named caller remains. I found no blocking production issue in this PR.
Key design decisions
Keeping the lossless encoding opt-in is the right compatibility boundary: external Arrow consumers retain the standard interval type, while closed internal storage can preserve Spark's full value domain. Making the struct self-describing through reserved child metadata also lets schema recovery, writer selection, and vector access agree without a separate read-side mode flag.
Implementation sketch
ArrowUtils creates and recognizes the tagged three-child struct; ArrowWriter serializes the three raw CalendarInterval components without unit conversion; and ArrowColumnVector reconstructs the scalar logical value from those children. The existing IntervalMonthDayNanoWriter keeps the standard layout and now scopes overflow translation to Math.multiplyExact.
Behavioral changes worth calling out
Default Arrow callers retain Interval(MONTH_DAY_NANO). Their only user-visible change is a structured DATETIME_OVERFLOW for out-of-range interval values instead of an opaque raw arithmetic overflow. Internal callers that explicitly enable losslessInternalTypes receive the new struct representation.
Suggested improvements
I left two non-blocking inline P3 comments: one to directly test the upstream-exception pass-through invariant, and one to validate the exact tagged struct shape before selecting the scalar interval path.
[P2] Adjacent integration risk in #56334 (not a blocker for this PR): when the Arrow cache enables losslessInternalTypes, its current zero-copy path can reuse a standard scalar IntervalMonthDayNanoVector or nanosecond timestamp vector while attaching the new lossless struct schema. The serialized RecordBatch then lacks the struct child nodes that VectorLoader expects on cache read. #56334 should require exact input-field/target-field equality for zero-copy and row-convert mismatched vectors. #57088 is safe independently because all current production callers leave the flag disabled.
+1, approving.
| // int64 nanosecond field. The overflow must surface as the structured DATETIME_OVERFLOW | ||
| // (not a raw ArithmeticException), and the translation must be scoped to the conversion: | ||
| // an unrelated (Spark)ArithmeticException raised by upstream evaluation must pass through | ||
| // unchanged, which the writer guarantees by catching only around Math.multiplyExact. |
There was a problem hiding this comment.
[P3] Could we add the regression case for the upstream-exception half of this invariant? This test currently checks only the Math.multiplyExact overflow. If a future refactor widened the try to include input.getInterval, an upstream DIVIDE_BY_ZERO could be relabeled as DATETIME_OVERFLOW while this test stayed green. A custom InternalRow whose getInterval throws a SparkArithmeticException, followed by an assertion that the original exception escapes unchanged, would pin the behavior.
There was a problem hiding this comment.
Added. The test now also drives a row whose getInterval throws a DIVIDE_BY_ZERO SparkArithmeticException (standing in for lazily-evaluated upstream input) and asserts the very same instance escapes unchanged, so a future refactor that widened the try past the Math.multiplyExact expression fails this test instead of silently relabeling upstream errors.
| .containsAll(Seq("months", "days", "microseconds").asJava) && | ||
| field.getChildren.asScala.exists { child => | ||
| child.getName == "months" && | ||
| child.getMetadata.getOrDefault(calendarIntervalStructKey, "false") == "true" |
There was a problem hiding this comment.
[P3] Could this recognize only the exact canonical shape: exactly three children in (months, days, microseconds) order with int32, int32, and int64 types and the expected nullability? A tagged reordered schema such as (days, months, microseconds) passes this predicate today. CalendarIntervalStructWriter writes children positionally while ArrowColumnVector reads them by name, so that shape silently swaps months and days. The current producer is canonical; this is defensive hardening for corrupt or future-evolved tagged schemas.
There was a problem hiding this comment.
Hardened, and good catch on the positional-write/by-name-read asymmetry. Both recognizers (isCalendarIntervalStructField and, since it had the identical looseness, isTimestampNanosStructField from SPARK-57975) now accept only the exact canonical shape: child count, order, names, integer widths, and non-nullability, via a shared helper. Anything tagged but non-canonical falls back to the generic struct handling, which is order-faithful. Added negative tests for reordered, wrong-width, extra-child, and missing-child tagged schemas for both types.
… scoped overflow catch Address review: the struct writers fill children positionally while ArrowColumnVector's accessors read them by name, so a permissive recognizer match on a tagged-but-reordered schema would silently swap component values. Both recognizers (isCalendarIntervalStructField and isTimestampNanosStructField, which had the identical looseness) now accept only the exact canonical shape -- child count, order, names, integer widths, and non-nullability -- and anything non-canonical falls back to the order-faithful generic struct handling, with negative tests for reordered, wrong-width, extra-child, and missing-child tagged schemas. Also pin the other half of the conversion-site catch invariant: the overflow test now drives a row whose getInterval throws an upstream DIVIDE_BY_ZERO SparkArithmeticException and asserts the same instance escapes unchanged, so a future refactor widening the try past Math.multiplyExact fails the test instead of silently relabeling upstream errors as DATETIME_OVERFLOW. Co-authored-by: Claude Code
… for CalendarInterval ### What changes were proposed in this pull request? This extends the opt-in lossless Arrow encoding introduced by SPARK-57975 (#57053) to `CalendarIntervalType`, and hardens the default interval writer's overflow error: - **Lossless struct encoding**: with the opt-in flag, a `CalendarInterval` column maps to an Arrow struct of `(months: int32, days: int32, microseconds: int64)` -- the type's own field layout, mirroring the default in-memory cache's `CALENDAR_INTERVAL` `ColumnType`. The components are stored as-is with no unit conversion, so the full `Long` microsecond domain round-trips. The struct is tagged through child-field metadata (the geometry/variant pattern) and is self-describing on read: `fromArrowField` recovers `CalendarIntervalType`, `ArrowWriter` selects a dedicated struct writer, and `ArrowColumnVector` serves `getInterval` from the child vectors, including nested inside arrays, structs, and maps. - **Flag rename**: the parameter is renamed from `losslessTimestampNanos` to `losslessInternalTypes`, since it now selects the lossless encoding for both kinds of types whose standard Arrow encoding cannot cover their full Spark value domain. `ArrowUtils` is `private[sql]`, so the rename has no compatibility impact; the only intended caller (the Arrow-based Dataset cache, #56334) wants both types, and the flag expresses one intent: internal storage wants fidelity. - **Structured error at the conversion site**: `IntervalMonthDayNanoWriter` now catches the `Math.multiplyExact(microseconds, 1000L)` overflow exactly at the conversion and raises the structured `DATETIME_OVERFLOW` (new `QueryExecutionErrors.calendarIntervalArrowNanosOverflowError`, the same pattern as `TimestampNTZNanosWriter`'s `timestampNanosEpochNanosOverflowError`) instead of letting a raw `ArithmeticException: long overflow` escape. Because the catch is scoped to the single conversion expression, it cannot re-label unrelated arithmetic failures (e.g. an ANSI `DIVIDE_BY_ZERO` raised by lazily-evaluated upstream input), which was a live mis-attribution risk with any wider catch (see #56334 (comment)). The default `Interval(MONTH_DAY_NANO)` mapping and every existing caller are unchanged. ### Why are the changes needed? Spark permits the full `Long` microsecond range in `CalendarInterval`, but Arrow's `IntervalMonthDayNano` stores the sub-day component as int64 nanoseconds, so any `|microseconds| > Long.MaxValue / 1000` (roughly +/-292 years) is structurally unrepresentable in the standard encoding -- the default in-memory cache serializer stores the three components raw and has no such limit. As with the nanosecond timestamps in SPARK-57975, the interchange mapping must keep the standard encoding for external consumers, so internal storage (the Arrow-based Dataset cache proposed in #56334) needs a per-call-site lossless alternative; with it, the cache can delete its schema-wide overflow-translation wrapper entirely. Raised in #56334 (comment) and #56334 (comment). ### Does this PR introduce _any_ user-facing change? The lossless encoding itself is opt-in via an internal API parameter and changes nothing by default. One user-visible improvement on the existing paths: writing an out-of-range `CalendarInterval` through Arrow (e.g. `toPandas`, Arrow UDFs) now fails with the structured `DATETIME_OVERFLOW` condition naming the value and the limit, instead of an opaque `java.lang.ArithmeticException: long overflow`. ### How was this patch tested? New tests: - `ArrowUtilsSuite` "calendar interval lossless struct": schema shape (struct of int32/int32/int64, non-null children), round-trip, nested array/struct/map coverage, user-metadata preservation, no misfire on an untagged struct with the same child names, and the default `Interval(MONTH_DAY_NANO)` mapping staying unchanged when the flag is off. - `ArrowWriterSuite` "calendar interval overflow raises DATETIME_OVERFLOW at the conversion site": the default writer raises the structured condition for `microseconds = Long.MaxValue / 1000 + 1`. - `ArrowWriterSuite` "calendar interval lossless struct round-trip covers the full value domain": write-and-read-back through `ArrowWriter` + `ArrowColumnVector` for values including `Long.MaxValue` / `Long.MinValue` microseconds and full-range months/days (all far outside the default mapping's limit) plus nulls. - `ArrowWriterSuite` "calendar interval lossless struct round-trip inside nested types": the same extreme values inside `array<...>`, `struct<...>`, and `map<int, ...>`. Existing regression suites pass: `ArrowUtilsSuite`, `ArrowWriterSuite`, `ArrowConvertersSuite`, `ColumnVectorSuite`, `ColumnarBatchSuite`. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code This pull request and its description were written by Claude Code. Closes #57088 from viirya/interval-arrow-lossless. Authored-by: Liang-Chi Hsieh <viirya@gmail.com> Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com> (cherry picked from commit 5ca6b10) Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com>
|
Thanks @dongjoon-hyun @sunchao |
What changes were proposed in this pull request?
This extends the opt-in lossless Arrow encoding introduced by SPARK-57975 (#57053) to
CalendarIntervalType, and hardens the default interval writer's overflow error:CalendarIntervalcolumn maps to an Arrow struct of(months: int32, days: int32, microseconds: int64)-- the type's own field layout, mirroring the default in-memory cache'sCALENDAR_INTERVALColumnType. The components are stored as-is with no unit conversion, so the fullLongmicrosecond domain round-trips. The struct is tagged through child-field metadata (the geometry/variant pattern) and is self-describing on read:fromArrowFieldrecoversCalendarIntervalType,ArrowWriterselects a dedicated struct writer, andArrowColumnVectorservesgetIntervalfrom the child vectors, including nested inside arrays, structs, and maps.losslessTimestampNanostolosslessInternalTypes, since it now selects the lossless encoding for both kinds of types whose standard Arrow encoding cannot cover their full Spark value domain.ArrowUtilsisprivate[sql], so the rename has no compatibility impact; the only intended caller (the Arrow-based Dataset cache, [SPARK-57268][SQL] Add Apache Arrow as a native cache format for in-memory Dataset caching #56334) wants both types, and the flag expresses one intent: internal storage wants fidelity.IntervalMonthDayNanoWriternow catches theMath.multiplyExact(microseconds, 1000L)overflow exactly at the conversion and raises the structuredDATETIME_OVERFLOW(newQueryExecutionErrors.calendarIntervalArrowNanosOverflowError, the same pattern asTimestampNTZNanosWriter'stimestampNanosEpochNanosOverflowError) instead of letting a rawArithmeticException: long overflowescape. Because the catch is scoped to the single conversion expression, it cannot re-label unrelated arithmetic failures (e.g. an ANSIDIVIDE_BY_ZEROraised by lazily-evaluated upstream input), which was a live mis-attribution risk with any wider catch (see [SPARK-57268][SQL] Add Apache Arrow as a native cache format for in-memory Dataset caching #56334 (comment)).The default
Interval(MONTH_DAY_NANO)mapping and every existing caller are unchanged.Why are the changes needed?
Spark permits the full
Longmicrosecond range inCalendarInterval, but Arrow'sIntervalMonthDayNanostores the sub-day component as int64 nanoseconds, so any|microseconds| > Long.MaxValue / 1000(roughly +/-292 years) is structurally unrepresentable in the standard encoding -- the default in-memory cache serializer stores the three components raw and has no such limit. As with the nanosecond timestamps in SPARK-57975, the interchange mapping must keep the standard encoding for external consumers, so internal storage (the Arrow-based Dataset cache proposed in #56334) needs a per-call-site lossless alternative; with it, the cache can delete its schema-wide overflow-translation wrapper entirely. Raised in #56334 (comment) and #56334 (comment).Does this PR introduce any user-facing change?
The lossless encoding itself is opt-in via an internal API parameter and changes nothing by default. One user-visible improvement on the existing paths: writing an out-of-range
CalendarIntervalthrough Arrow (e.g.toPandas, Arrow UDFs) now fails with the structuredDATETIME_OVERFLOWcondition naming the value and the limit, instead of an opaquejava.lang.ArithmeticException: long overflow.How was this patch tested?
New tests:
ArrowUtilsSuite"calendar interval lossless struct": schema shape (struct of int32/int32/int64, non-null children), round-trip, nested array/struct/map coverage, user-metadata preservation, no misfire on an untagged struct with the same child names, and the defaultInterval(MONTH_DAY_NANO)mapping staying unchanged when the flag is off.ArrowWriterSuite"calendar interval overflow raises DATETIME_OVERFLOW at the conversion site": the default writer raises the structured condition formicroseconds = Long.MaxValue / 1000 + 1.ArrowWriterSuite"calendar interval lossless struct round-trip covers the full value domain": write-and-read-back throughArrowWriter+ArrowColumnVectorfor values includingLong.MaxValue/Long.MinValuemicroseconds and full-range months/days (all far outside the default mapping's limit) plus nulls.ArrowWriterSuite"calendar interval lossless struct round-trip inside nested types": the same extreme values insidearray<...>,struct<...>, andmap<int, ...>.Existing regression suites pass:
ArrowUtilsSuite,ArrowWriterSuite,ArrowConvertersSuite,ColumnVectorSuite,ColumnarBatchSuite.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code
This pull request and its description were written by Claude Code.