peterxcli opened a new issue, #5770:
URL: https://github.com/apache/datafusion-comet/issues/5770
### 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` (`6368c4e61`), 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.
--
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]
