You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "peterxcli (via GitHub)" <gi...@apache.org> on 2026/09/06 06:22:05 UTC
[PR] refactor: use Arrow cast for date to timestamp NTZ [datafusion-comet]
peterxcli opened a new pull request, #5735:
URL: https://github.com/apache/datafusion-comet/pull/5735
## Which issue does this PR close?
Closes #5090.
## Rationale for this change
Use Arrow's existing cast kernel for the remaining hand-written temporal conversion in #5090: Date32 to TIMESTAMP_NTZ. The other three locations in the issue were already handled by #5177.
## What changes are included in this PR?
- Replace the NTZ builder loop with `cast_with_options` targeting `Timestamp(Microsecond, None)`.
- Use `DEFAULT_CAST_OPTIONS` (`safe: false`) so out-of-range dates return an error. Arrow 59 uses checked multiplication here, matching Spark's checked `daysToMicros` conversion instead of the previous loop's release-mode wrapping or debug-mode panic.
- Preserve the session-timezone path and Spark-specific DST ambiguity/gap resolution.
- Extend the existing NTZ regression test with the positive and negative microsecond boundaries and overflowing dates.
## How are these changes tested?
- `cd native && cargo test -p datafusion-comet-spark-expr --lib conversion_funcs::temporal::tests --offline`: 2 tests passed after rebasing onto upstream main. Covers the timezone path, NTZ session-timezone independence, negative dates, nulls, timezone annotation, and overflow boundaries.
- `cargo fmt --manifest-path native/Cargo.toml --all -- --check` and `git diff --check upstream/main...HEAD` passed.
### Kernel microbenchmark
Standalone release-mode comparison of the previous builder loop against the new Arrow cast, using Arrow 59.3.0 and Rust 1.96.0 on macOS arm64. Release settings: thin LTO, one codegen unit, overflow checks disabled. This isolates the conversion kernel; it does not measure Spark query performance or the full Comet expression path.
Inputs contain deterministic dates before and after the epoch, with irregular null placement. Both implementations receive identical arrays, and output equality is checked before timing every case. Input generation is outside the timed region; allocation and output destruction are included. Each case warms up for 200 ms, then collects 21 paired samples with alternating execution order and black-boxed inputs/outputs. Reported values are median time per batch; speedup is the ratio of medians. Null percentages describe the generation thresholds.
| Rows | Nulls | Previous loop | Arrow cast | Speedup |
|---:|---:|---:|---:|---:|
| 1,024 | 0% | 0.904 µs | 0.579 µs | 1.56× |
| 1,024 | 10% | 3.551 µs | 0.795 µs | 4.47× |
| 1,024 | 50% | 3.789 µs | 0.537 µs | 7.06× |
| 1,024 | 100% | 2.496 µs | 0.220 µs | 11.35× |
| 8,192 | 0% | 6.477 µs | 3.704 µs | 1.75× |
| 8,192 | 10% | 26.945 µs | 5.505 µs | 4.89× |
| 8,192 | 50% | 28.996 µs | 3.401 µs | 8.53× |
| 8,192 | 100% | 18.628 µs | 0.839 µs | 22.22× |
These are local measurements from one run, not CI performance guarantees.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: use Arrow cast for date to timestamp NTZ [datafusion-comet]
Posted by "rich7420 (via GitHub)" <gi...@apache.org>.
rich7420 commented on PR #5735:
URL: https://github.com/apache/datafusion-comet/pull/5735#issuecomment-5571615664
@peterxcli thanks for the patch!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: use Arrow cast for date to timestamp NTZ [datafusion-comet]
Posted by "coderfender (via GitHub)" <gi...@apache.org>.
coderfender commented on PR #5735:
URL: https://github.com/apache/datafusion-comet/pull/5735#issuecomment-5579525685
@parthchandra perhaps you would want to take a look at the PR? @sunchao perhaps we could wait till Parth adds his review please?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: use Arrow cast for date to timestamp NTZ [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead merged PR #5735:
URL: https://github.com/apache/datafusion-comet/pull/5735
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: use Arrow cast for date to timestamp NTZ [datafusion-comet]
Posted by "rich7420 (via GitHub)" <gi...@apache.org>.
rich7420 commented on code in PR #5735:
URL: https://github.com/apache/datafusion-comet/pull/5735#discussion_r3950323795
##########
native/spark-expr/src/conversion_funcs/temporal.rs:
##########
@@ -40,49 +42,46 @@ pub(crate) fn cast_date_to_timestamp(
cast_options: &SparkCastOptions,
target_tz: &Option<Arc<str>>,
) -> SparkResult<ArrayRef> {
+ if target_tz.is_none() {
+ // TIMESTAMP_NTZ ignores session TZ. Like Spark's daysToMicros, reject overflow.
+ return Ok(cast_with_options(
+ array_ref.as_ref(),
+ &DataType::Timestamp(TimeUnit::Microsecond, None),
+ &DEFAULT_CAST_OPTIONS,
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org