Conversation
sunchao
left a comment
There was a problem hiding this comment.
Correctness
This changes the scalar-format implementations of Spark trunc(date, format) and date_trunc(format, timestamp) to call DataFusion 55.1's physical date_trunc. Spark's aliases are normalized before dispatch. Date32 values use a timestamp cast on each side of the upstream call. Coarse values outside the guarded nanosecond range retain the previous calendar implementation. Per-row formats and dictionary-key handling keep their existing paths.
I compared the implementation with TruncDate, TruncTimestamp, TruncInstant, and DateTimeUtils on the maintained Spark 3.5 and 4.0 branches. The accepted aliases, Monday-based weeks, month/quarter/year boundaries, pre-epoch rounding, and existing null/invalid-format fallback agree within the reviewed scope. These expressions have no changed ANSI-specific behavior. The explicit Date32 return type in the serde also avoids resolving the Spark function through DataFusion's different signature. The non-UTC incompatibility gate remains in place. The added DST tests exercise that opt-in path and do not establish general non-UTC compatibility.
The lower guard includes room for truncation to move the result before the input, which is necessary near the nanosecond boundary. The added tests cover aliases, nulls, dictionaries, leap dates, pre-epoch values, the lower boundary, year 3333, and DST overlaps/gaps. I found no confirmed new correctness finding in the paths reviewed.
Validation
At ed65ff23, a standalone probe compiled the exact upstream UTC integer-calendar functions and Comet Date32 guards and passed 1,056 calendar outputs plus 66 guard checks. Its oracle follows the maintained Spark UTC calendar rules. This does not execute Arrow casts, dictionary/null assembly, the timezone kernel, JNI, or Spark. Maintained Spark 3.4, 4.1, and 4.2 sources were unavailable, so I am not claiming those version comparisons.
The author reports native and Spark test passes. The hosted evidence is still limited: at 2026-09-15 18:33 UTC, CI, the Delta gate, CodeQL, and title workflows were action_required with zero jobs. Only labeling ran successfully. The merge preview has parents fad62309 and ed65ff23 and the same tree as the reviewed head, but no product CI execution is established.
Performance
The Date32 change adds a filtered array and two full-array casts around the upstream kernel even when every non-null value is in the ordinary modern range. Mixed-range batches perform another output merge. Timestamp fine-granularity paths can use upstream arithmetic directly, while coarse mixed-range timestamp batches construct two masked inputs and merge their results.
The inline P2 asks for matched measurements before accepting the Date32 tradeoff. There is an existing native Date32 benchmark for all four supported granularities, but the PR contains no results. I have not measured a slowdown. The correctness probe and the reported SQL tests do not establish performance. Any timestamp performance claim should also cover the default UTC path because the existing timestamp benchmark uses Los Angeles.
Design
Calling the upstream physical function behind Spark-specific alias and compatibility checks gives the migration a clear boundary. Keeping the existing implementation for the wider date range avoids exposing DataFusion 55.1's nanosecond restriction directly to Spark users. The unchanged serde restrictions, timestamp type annotation, and explicit Date32 return type preserve the planner contract.
The main design decision still needing evidence is routing Date32 through timestamps. Retaining the existing Date32 kernel is a concrete simpler alternative if the extra conversions erase the upstream benefit. That decision should follow the requested measurements. No generated compatibility documentation needs a manual edit for this implementation-only change.
Abstraction & complexity
The shared upstream invocation helper centralizes argument fields, return type, and error conversion. Separate alias tables make the Spark vocabulary explicit, and the range predicate documents why coarse truncation needs a margin. The legacy helpers still serve the row-format and wide-range paths, so they are not dead duplication.
The split-and-merge logic adds per-batch work and a second implementation to maintain. The new boundary and mixed-range tests support that responsibility. Beyond the measured dispatch decision requested above, I found no additional abstraction change that clears the bar for a separate finding.
| let timestamps = cast( | ||
| &upstream_input, | ||
| &DataType::Timestamp(TimeUnit::Microsecond, None), | ||
| )?; | ||
| let truncated = datafusion_date_trunc(timestamps, granularity)?; | ||
| let truncated = cast(truncated.as_ref(), &DataType::Date32)?; |
There was a problem hiding this comment.
Performance
[P2] Could you include matched parent/head microbenchmark results before switching the default scalar Date32 path? Even an ordinary all-modern batch now builds a filtered Date32 array, casts the whole batch to microseconds, runs the upstream kernel, and casts the result back to Date32. The previous implementation produced its output in one pass, and mixed modern/year-3333 batches add another merge pass. The existing native/spark-expr/benches/date_trunc.rs already exercises YEAR/QUARTER/MONTH/WEEK over 10,000 modern dates. Please compare that benchmark on the parent and this head, add a representative mixed-range/null case, and use the results to justify this dispatch or retain the existing Date32 kernel where the conversions cause a material regression. The correctness tests do not establish the performance tradeoff, and there are no benchmark results in the PR yet.
…aths Reuse the upstream physical date_trunc for scalar formats while keeping Comet per-row format, dictionary, and session-timezone handling, with a fallback outside DataFusion's TimestampNanosecond range.
ed65ff2 to
029dafd
Compare
sunchao
left a comment
There was a problem hiding this comment.
Re-reviewed 0e85f5a9 against 23f5b63b and the previously reviewed ed65ff23. The prior P2 Date32 performance concern is addressed. trunc(date, fmt) now retains the original single-pass Date32 calculation, removing the temporary filtered array and both casts. The benchmark adds the requested mixed modern/year-3333/null fixture. The description reports regressions for the discarded cast implementation, which I have not independently rerun.
I verified that the restored Date32 loop is equivalent to the current base and that its format lookup, calendar helpers, dictionary handling, and per-row format paths are unchanged. The timestamp implementation, range guards, serde restrictions, and UDF wrappers are unchanged from the previous review. The other incremental changes come from the updated base, including the stronger routing assertions. No new findings.
Validation was source comparison against the maintained Spark 3.5/4.0 branches and checksum verification of the locked DataFusion 55.1 source. I did not run Spark/JNI suites or benchmarks for this follow-up, and the maintained Spark 3.4/4.1 branches remain unavailable. CI and CodeQL are still action_required with zero jobs. Only the label job has passed.
sunchao
left a comment
There was a problem hiding this comment.
Rechecked b6851020 against 58ab5f61 and the previously approved 0e85f5a9. This mainline merge leaves the authored additions and deletions unchanged across all nine PR files. The temporal kernel, SQL tests, Scala suite and mixed-range benchmark are byte-identical. The P2 Date32 concern remains addressed by the original single-pass Date32 calculation. Timestamp guards, serde restrictions and native routing remain intact. No new or remaining verified P1/P2 finding. The existing approval stands.
Validation was source comparison against maintained Spark 3.5/4.0, locked DataFusion source evidence, and deterministic benchmark-fixture checks. No local Spark/JNI suite or benchmark was run. Maintained Spark 3.4/4.1 sources remain unavailable. Current-head CI and CodeQL remain action_required with zero jobs. Only labeling passed. The advertised merge tree equals the head, but no product CI execution of that tree was verified.
Which issue does this PR close?
Part of #5103 (temporal truncation only).
Rationale for this change
#5103 asked Comet to stop maintaining a parallel truncation kernel where DataFusion already has a compatible physical
date_trunc.This PR routes compatible scalar-format
date_trunc(fmt, timestamp)paths throughdatafusion-functions55.1.0.trunc(date, fmt)retains Comet's existing direct Date32 kernel because matched parent/head benchmarks showed that routing Date32 through a Date32 → Timestamp →date_trunc→ Date32 cast sandwich regressed performance by 181%–308%.This is not a full replacement of Comet's truncation kernels. For timestamp granularities that DataFusion evaluates through nanoseconds, Spark-legal values outside the TimestampNanosecond range (approximately 1678–2262, for example year 3333) continue to use the existing Comet calendar fallback.
Per-row format handling, dictionary unwrap, and session-timezone pre-shift behavior remain unchanged.
What changes are included in this PR?
YEAR/YYYY/YY,QUARTER,MONTH/MON/MM) and Monday-basedWEEKhandling in Comet's direct day-based kernel.date_trunc.date_truncafter the existing session-timezone pre-shift.date_truncremainsIncompatible(date_trunc incorrect results in non-UTC timezone #2649). DST SQL coverage usesallowIncompatible=true; shuffle may still insert the existing DataFusion / DataFusion-Spark functions whose Arrow return type drifts from Spark catalyst's declared type #4515 schema-align cast.How are these changes tested?
Spark compatibility and DST-boundary tests:
trunc_date.sql,trunc_timestamp.sql).America/Los_Angeles,America/New_York, andAmerica/Sao_Paulo.CometTemporalExpressionSuite:Date32 benchmark results
The initial PR head routed Date32 through DataFusion using a Date32 → Timestamp →
date_trunc→ Date32 cast sandwich. The following results compare that implementation with the parent using identical benchmark inputs.Because every measured Date32 shape regressed materially, the final implementation retains Comet's existing single-pass Date32 kernel. DataFusion routing is limited to compatible timestamp paths.
Local runs: