apache / apache/datafusion-comet
perf: skip calendar reconstruction in datetime extraction (hour/minute/second, dayofweek/weekday)
- Dominant language
- Scala
- Stars
- 1.3k
- Forks
- 373
- Avg merge
- 2d 4h
- Merged PRs (30d)
- 198
Description
### What is the problem the feature request solves?
Comet's date/time extraction paths reconstruct a `chrono` calendar datetime per row to produce
values that are a pure function of the epoch day or the microsecond-of-day. The reconstruction
is the expensive part and it is not needed for these fields.
Measured on `main` (`bb9e74020`), 8192-row batches, aarch64 (Apple Silicon), Criterion, quiet
machine. Two independent harnesses agree: an in-tree bench calling the real Comet UDFs
(`ci` profile), and a standalone arrow-only bench built under comet's `[profile.release]`
settings. Every bench asserts the replacement is bit-identical to the current path before it
times anything.
| path | current | integer kernel | speedup |
| --- | --- | --- | --- |
| `hour`/`minute`/`second`, `Timestamp` + UTC session | 93.4-106.6 us | 4.4-7.1 us | **14.1-23.4x** |
| `hour`/`minute`/`second`, `TimestampNTZ` | 48.3-61.2 us | 4.4-7.8 us | **7.0-12.6x** |
| `dayofweek` / `weekday` on `Date32` | 44.9-49.3 us | 3.2-3.5 us | **12.9-14.1x** |
| `iceberg_years`, year-only split | 17.2-18.5 us | 14.6-14.7 us | 1.18-1.25x |
(The release-profile harness, which omits Comet's UDF wrapper, gives the same ordering with
smaller ratios: 9.8-16.1x, 6.2-9.6x, 9.4-10.3x, 1.12x.)
**1. `hour` / `minute` / `second` -- the largest win**
`native/spark-expr/src/datetime_funcs/extract_date_part.rs` already separates `TimestampNTZ`
from timezone-aware timestamps, then calls arrow's `date_part`, which builds a datetime per row.
For NTZ, and for timezone-aware timestamps in a UTC session, the clock fields are arithmetic on
the stored microseconds:
```rust
const MICROS_PER_DAY: i64 = 86_400_000_000;
fn hour(micros: i64) -> i32 { (micros.rem_euclid(MICROS_PER_DAY) / 3_600_000_000) as i32 }
fn minute(micros: i64) -> i32 { micros.div_euclid(60_000_000).rem_euclid(60) as i32 }
fn second(micros: i64) -> i32 { micros.div_euclid(1_000_000).rem_euclid(60) as i32 }
```
Euclidean division is load-bearing: at UTC, `-1` microsecond is `1969-12-31 23:59:59.999999`,
and truncation toward zero gives the wrong second.
Worth calling out: **a UTC-tagged timestamp costs about twice an NTZ one** through `date_part`
(103.5 vs 61.2 us for `hour`) even though UTC applies no offset -- the tag alone forces the
timezone-aware datetime construction. A UTC fast path erases that entirely. For reference, a
real offset zone (`America/Los_Angeles`) costs 179-199 us and is *not* replaceable by this.
**2. `dayofweek` / `weekday` on `Date32`**
`CometDayOfWeek` and `CometWeekDay` in `spark/src/main/scala/org/apache/comet/serde/datetime.scala`
emit `datepart('dow', child) + 1` and `datepart('isodow', child) - 1`. `datepart` resolves to
DataFusion's `date_part`, which for `Date32` runs
```rust
// arrow-arith/src/temporal.rs:405
Ok(self.unary_opt(|d| date32_to_datetime(d).map(map_func)))
```
-- a `NaiveDateTime` per row plus a recomputed null mask -- and the `+ 1` / `- 1` is a second
pass allocating another `Int32Array`. Both collapse to one modulo:
```rust
/// Spark: Sunday = 1, ..., Saturday = 7
fn spark_dayofweek(days: i32) -> i32 { ((i64::from(days) + 4).rem_euclid(7) + 1) as i32 }
/// Spark: Monday = 0, ..., Sunday = 6
fn spark_weekday(days: i32) -> i32 { (i64::from(days) + 3).rem_euclid(7) as i32 }
```
Epoch day 0 is 1970-01-01, a Thursday: `5` and `3`. Widening to `i64` avoids overflow at
`i32::MAX`. This one also deletes a plan node, since the `+1`/`-1` folds into the kernel.
The `Int32` cast in the serde is already free -- `date_part` returns `Int32` for these parts in
DF 55 and `cast.rs:230` short-circuits an identity cast -- so it is not part of the win.
**3. `iceberg_funcs::temporal::civil_from_days` -- measured, and mostly not worth it**
The shipped Hinnant split vectorizes well once inlined (17.2 us for 8192 rows). Two variants
were benchmarked:
- **Year-only** (skip the `/153` and the month remap for `iceberg_years`, using
`day_of_year >= 306` as the Jan/Feb test): **1.18-1.25x**. Real but small.
- **Biased epoch** (add `146_097 * 14_700` so the era split uses plain `/` and `%` instead of
`div_euclid`/`rem_euclid`): **0.65-0.82x -- a regression**, consistently across all four
groups and both harnesses. LLVM handles the Euclidean correction for a constant divisor
better than the extra bias arithmetic costs. Recorded so nobody tries it again.
So this is the weakest of the three targets, not the best starting point.
### Describe the potential solution
In value order:
1. An integer fast path in `extract_date_part.rs` for `TimestampNTZ` and for `Timestamp` with a
UTC session timezone, dispatched **once per batch**, falling through to the existing
timezone-aware path otherwise.
2. Native `spark_dayofweek` / `spark_weekday` kernels over `Date32`, folding the `+1` / `-1` in
so the serde stops emitting a separate math node.
3. Optionally the year-only split for `iceberg_years`. Low value; skip if it complicates the
shared helper.
Constraints that should not move:
- Preserve null bitmaps, dictionary handling and sliced-array behaviour. The dictionary return
type in `extract_date_part.rs` is part of the contract.
- Keep NTZ vs timezone-aware semantics as they are. A UTC fast path is not licence to assume a
fixed offset for a batch spanning a DST transition.
- `dayofweek` (Sunday=1..7) and `weekday` (Monday=0..6) use different numbering;
`second(timestamp)` is integer seconds, distinct from fractional-second extraction; ISO
week/year is week-based-year arithmetic, not weekday arithmetic.
- If this is generalized to `year` / `month` / `dayofmonth`, note DF 55's `date_part` implements
`preimage` (`datafusion-functions-55.0.0/src/datetime/date_part.rs:263`), rewriting a year
predicate into a date range. It only fires for `YEAR`, so `dow`/`isodow` lose nothing, but
replacing `year` with a Comet UDF would give up that pruning.
- Full `i32` epoch-day range must keep working -- the Iceberg transform covers `i32::MIN`/`MAX`
days (years -5877641 to 5881580), outside chrono's range.
Acceptance:
- Kernel equivalence over a full 400-year cycle, negative epochs, `i32` boundaries, plus null /
dictionary / sliced arrays.
- Differential Spark tests with Comet on and off across session timezones, NTZ, non-hour
offsets and DST transitions, asserting the native expression actually runs so a fallback
cannot hide a broken kernel.
- A query-level measurement, which this issue does **not** have (see below).
### Additional context
Prompted by the fast-date / fast-day-of-week / fast-time-of-day articles at
https://www.benjoffe.com/, though none of that code is used here -- the kernels above are plain
Euclidean arithmetic. No third-party fast-calendar implementation was ported: doing so means
transcribing constants that cannot be checked against the source, and those implementations use
narrow intermediates in places, so a naive port can overflow near `i32::MIN`.
**Scope of the evidence.** These are kernel microbenchmarks on 8192-row batches, not a Spark
query. They show the expression is 7-23x cheaper; they do **not** show any end-to-end query is
faster, which depends on what fraction of query time these expressions occupy. A
plan-inspected, consumption-forced query benchmark
(`SELECT sum(hour(ts)), sum(minute(ts)) ...`) is still needed, on x86-64 as well -- all numbers
here are aarch64.
One methodology note for anyone re-running this: passing the kernel to
`PrimitiveArray::unary` through a `black_box`ed function pointer blocks inlining and
vectorization, and understates the kernel by roughly 2-3x. It also inverted the target 3
conclusion -- the biased-division variant looked like a 1.2x win when both arms were
pessimized, and is a 0.75x regression once they inline. Pass the kernel as a generic
`F: Fn(..) -> i32 + Copy` and `black_box` the array instead.
Contributor guide
Research direction
Start in native/spark-expr/src/datetime_funcs/extract_date_part.rs and inspect the CometDayOfWeek and CometWeekDay definitions in spark/src/main/scala/org/apache/comet/serde/datetime.scala. Compare the existing paths with arrow-arith/src/temporal.rs:405, then validate null, dictionary, sliced, negative-epoch, boundary, timezone, and DST cases. Done means equivalent kernels, passing differential Spark tests, and a query-level measurement.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust, scala
- Domain
- backend, performance
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100