andygrove opened a new issue, #6717:
URL: https://github.com/apache/datafusion-comet/issues/6717
## What / Why
#5956 adds a check to `timestamp_trunc_upstream` in
`native/spark-expr/src/kernels/temporal.rs`, ahead of the fine-unit fast path
(`MILLISECOND`, `SECOND`, `MINUTE`, and `HOUR` and `DAY` when the array has no
timezone). It scans every physical value for one that would underflow when
truncated:
```rust
if array.values().iter().any(|micros| *micros < lower) {
return timestamp_trunc_fine_boundary(array, granularity, unit);
}
```
The check is needed, but `iter().any` exits early and doesn't vectorize. In
the `timestamp_trunc` benchmark from that PR, UTC `SECOND`, `MINUTE`, `HOUR`
and `DAY` on 8192 rows without nulls went from about 7.7 µs to about 10 µs per
batch compared with the kernel without the check (release build without LTO).
That is still far faster than the chrono kernel the PR replaces, so this is
headroom rather than a regression against a release.
## Suggested fix
Take the minimum of the values buffer with a fold, or with
`arrow::compute::min` over a null-free view of the same buffer, and compare it
with `lower` once. Both vectorize. The check should keep looking at physical
values, null slots included, as it does now, because DataFusion's fine-unit
path subtracts from those too.
To measure, run `cargo bench --bench timestamp_trunc --
'timestamp_trunc/UTC/(DAY|HOUR|MINUTE|SECOND)/8192'` in `native/spark-expr`
before and after.
This depends on #5956 merging first.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
