perf: use Arrow cast for date to timestamp NTZ - #5735
Conversation
| return Ok(cast_with_options( | ||
| array_ref.as_ref(), | ||
| &DataType::Timestamp(TimeUnit::Microsecond, None), | ||
| &DEFAULT_CAST_OPTIONS, |
There was a problem hiding this comment.
Could we use safe: true for EvalMode::Try, while keeping Legacy and ANSI strict?
For day count 106751992, Spark 4.1.3 TRY_CAST returns NULL, but this head's spark_cast returns ArithmeticOverflow. I verified these separately through Spark SQL and the native entry point.
Could you add a TRY-mode test mixing valid, overflowing and null dates?
|
@peterxcli thanks for the patch! |
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Reviewed 75aade9372b3afc64794250d19cb3425d578fbd7 against 7e1984399eb887cd13109698ee55cf2ce150f849. This is a one-file change replacing the handwritten Date32-to-TIMESTAMP_NTZ builder loop with Arrow's cast to Timestamp(Microsecond, None). It also replaces unchecked multiplication with an explicit overflow error. The target is microseconds, consistent with Comet's Spark type mapping, rather than nanoseconds. The timezone-aware branch is unchanged apart from indentation.
The maintained Spark 3.5/4.0 implementations convert a date to midnight using daysToMicros(d, ZoneOffset.UTC). They therefore ignore the session timezone for NTZ and use checked microsecond arithmetic. Arrow 59.3's strict kernel matches that behavior for Legacy and ANSI casts. Valid day counts range from -106751991 to 106751991. The new tests cover both endpoints, adjacent overflowing days, both i32 extremes, nulls and session-timezone independence. Arrow's fallible array traversal checks only valid slots, so a null slot's physical payload is not treated as a date to convert.
[P2] Preserve TRY_CAST overflow-to-null semantics
The existing TRY_CAST concern remains. I independently confirmed it against the maintained Spark 3.5/4.0 sources. Both interpreted evaluation and generated code catch this overflow and return null in TRY mode. This branch always passes DataFusion's DEFAULT_CAST_OPTIONS, which has safe: false. For a batch containing [0, 106751992, null], it returns an error instead of [0, null, null] in microseconds. The non-folded scalar path reaches the same array conversion and propagates the error. Scala marks this cast compatible in TRY mode, so fallback does not protect column inputs. I have not added a duplicate inline comment.
Could the existing request be addressed with safe: true only for EvalMode::Try, retaining strict behavior for Legacy and ANSI? The generic !Ansi option would incorrectly make Legacy nullable here. A regression through spark_cast should cover mixed valid/overflow/null arrays and scalar inputs in all three modes. A column-based SQL regression would also pin native execution. Literal-only SQL is insufficient because Comet's serde folds literal casts itself. I found no additional P1/P2 issue in the authored change. Maintained Spark 3.4/4.1 source coverage remains unavailable.
The CI snapshot at 2026-09-08T03:18:30.466Z contains 65 successful, one failed and 51 skipped checks, including three later all-skipped CI runs. I verified the executed checkout c240de298fec4ec1595cfd3513a974a0752a1d29: its parents are exactly this base and head, and its tree equals the head. The Rust job passed 1,167 tests with four skipped, including both temporal tests. The Spark 3.5 expression job passed 1,314 tests, including date-to-NTZ. Those inputs do not cover overflowing dates in TRY mode. The macOS scan job failed with a JVM SIGSEGV during scan-suite execution. I did not establish its cause or attribute it to this patch. Local source and formatting checks passed. I did not run a local native/JNI/Spark build, runtime reproduction or benchmark.
Performance
The description supplies an appropriately scoped old-loop versus Arrow-kernel comparison using the same Arrow 59.3 dependency as this pair. It reports deterministic identical inputs, output equality before timing, allocation and output destruction inside the timing, warmup and 21 alternating paired samples. At 1,024 and 8,192 rows, the reported speedups range from 1.56–1.75× without nulls to 11.35–22.22× for all-null batches. These are author measurements from one macOS arm64 run, not independently reproduced results or Spark query speedups.
The source removes repeated builder appends and lets Arrow traverse the validity bitmap. It still allocates the output array, and the checked kernel should not be described as automatically vectorized. The existing query benchmark uses TIMESTAMP rather than explicitly selecting NTZ, so it does not establish this patch's end-to-end benefit. When updating the performance claim after the TRY fix, please include that mode in the paired kernel comparison because safe: true selects a different Arrow traversal. No broader query-level gain is needed to justify the limited kernel claim currently made.
Design
Using the existing Arrow kernel is a simpler implementation for the timezone-independent arithmetic. Keeping the timezone-aware path separate preserves Spark-specific DST gap and ambiguity handling. The early return also avoids building an unused output builder for NTZ. The key remaining decision is the Spark evaluation mode: the selected Arrow error policy must represent TRY versus strict behavior explicitly, rather than treating one DataFusion default as the policy for every Spark cast.
Abstraction & complexity
This change adds no new abstraction, registration or dependency. It removes a manual loop and keeps the existing helper and callers, which is an appropriate scope for the optimization. The tests extend the existing temporal suite instead of creating another framework. Passing mode-aware cast options at this boundary is sufficient for the remaining issue. Adding a second conversion implementation or an outer batch-wide error-to-null wrapper would add complexity and could erase valid rows alongside the overflowing one.
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Rechecked ca78e151ae487d8da48235288d9c9dc47560b308 against the previous review at 75aade9372b3afc64794250d19cb3425d578fbd7, with authoritative base 7e1984399eb887cd13109698ee55cf2ce150f849. The previous TRY overflow concern is addressed: safe is now true only for EvalMode::Try, while Legacy and ANSI remain strict. This matches the maintained Spark 3.5/4.0 interpreted and generated paths, which catch the checked date-to-microsecond overflow only for TRY. Arrow 59.3's nullable traversal preserves valid values and existing nulls and nulls each overflowing date individually. It ignores physical payloads in null slots. The timezone-aware branch and string conversions are unchanged by this update.
The new spark_cast regression covers mixed valid/positive-overflow/null/negative/negative-overflow input in all three modes, then checks each scalar separately. It verifies that TRY preserves the valid rows and that strict modes still error. The existing helper tests retain endpoint, i32-extreme and timezone coverage. The added SQL query uses a Parquet column, avoiding literal folding, and its plain query directive requires native operator coverage as well as equality with Spark across the four configured timezones. I found no new or remaining P1/P2 issue; no duplicate inline comment is needed. Maintained Spark 3.4/4.1 source coverage remains unavailable.
The current Rust job passed all three temporal tests: 1,190 tests passed overall, with four skipped. Its actual checkout is a2511c0bd7500452d0521ba7c03413d8a5cc9bbc, merging this head into 99d3100c60cdf5c1fae26ac7d7102953b250900f, rather than the authoritative base above. I verified identical conversion, wrapper, SQL fixture, harness and dependency-lock blobs; the complete merge tree differs, so this is supporting CI evidence, not a full build of the exact assigned pair. The fresh check at 2026-09-08T04:57:01Z had 50 successful, 15 running and seven skipped checks, with no failures. The newly completed expression-job statuses are not additional execution credit here: I have not independently inspected their SQL fixture logs or native artifact consumption. The reported local Spark 4.1.3 SQL passes are author evidence. Local formatting and diff checks passed; I did not run a local Comet/JNI/Spark build or runtime reproduction. The earlier macOS scan crash remains historical evidence with no established attribution to this patch.
Performance
The updated measurements now include TRY's nullable traversal separately from strict Arrow conversion, addressing the previous performance request. The description reports matched representable inputs, equality checks for both modes, allocation and output destruction inside the timing, warmup and rotating samples. Its reported TRY speedups range from 1.71× to 26.21× across the listed batch sizes and null fractions; strict-mode speedups range from 1.58× to 26.42×. These remain author-reported macOS arm64 kernel measurements; I have not independently reproduced the harness or timings. They support the limited kernel claim, without establishing Spark query speedups.
Design
The fix puts the Spark mode decision at the existing Arrow conversion boundary. Selecting TRY explicitly avoids the generic !Ansi policy that would also make Legacy nullable. Per-element overflow handling preserves valid rows in a mixed batch, and the separate timezone-aware conversion keeps its existing behavior. This resolves the design issue raised in the previous review without broadening the change.
Abstraction & complexity
The update adds no helper layer, dependency or second conversion implementation. It extends the existing temporal tests through the public wrapper and adds one column-based SQL regression using the established harness. The small mode-aware options value is sufficient here; no further abstraction or batch-wide error suppression is needed.
|
@parthchandra perhaps you would want to take a look at the PR? @sunchao perhaps we could wait till Parth adds his review please? |
rich7420
left a comment
There was a problem hiding this comment.
LGTM , thanks for the update
Which issue does this PR close?
Closes #5090.
Rationale for this change
Arrow already provides the date-to-timestamp conversion, so Comet can replace the per-row builder loop with its cast kernel.
What changes are included in this PR?
Convert Date32 to TIMESTAMP_NTZ with Arrow and select overflow handling by Spark evaluation mode. TRY_CAST returns null for each overflowing date; Legacy and ANSI return an error. For example, TRY_CAST on day counts
[0, 106751992, null]produces[1970-01-01 00:00:00, null, null].The timezone-aware conversion continues to use Spark's DST rules. This completes the remaining conversion in #5090; #5177 handled the other locations.
How are these changes tested?
Three native temporal tests pass, covering boundaries, nulls, session timezones, and mixed arrays and scalar inputs through
spark_castin all three modes. The column-based SQL regression passes on Spark 4.1.3 in all four session timezones. Native and JVM builds, Rust formatting, and whitespace checks pass.Commands:
From the repository root:
Benchmark
Local release-mode kernel comparison on macOS arm64 with Arrow 59.3.0 and Rust 1.96.0, thin LTO and one codegen unit. Inputs contain identical representable dates with deterministic values and irregular null placement. Output equality is checked for both Arrow modes. Timings include allocation and output destruction, with 200 ms warmup and 21 samples per implementation in rotating order. The table shows median time per batch and speedup over the old loop. Measurements were collected after the builds and tests finished. These results measure the conversion kernel only.