peterxcli opened a new pull request, #5771:
URL: https://github.com/apache/datafusion-comet/pull/5771
## Which issue does this PR close?
Closes #5770.
## Rationale for this change
Two date/time paths rebuilt a `chrono` calendar datetime per row to produce
a value that is a
pure function of the epoch day or the microsecond-of-day. The reconstruction
is the expensive
part and neither field needs it.
## What changes are included in this PR?
**1. `hour` / `minute` / `second` integer fast path**
(`extract_date_part.rs`)
When no timezone offset applies, the field is arithmetic on the stored
microseconds and no
datetime has to be built. That covers `TimestampNTZ` (already local
wall-clock) and a
timezone-aware timestamp in a UTC session -- `array_with_timezone` only
re-tags such an array
(`utils.rs`, the `Timestamp(Microsecond, Some(_))` arm calls `with_timezone`
and returns), so the
stored value is the UTC instant either way.
The gate is deliberately narrow: dictionaries, non-microsecond units, and
any session timezone
outside a small zero-offset allowlist keep the existing `date_part` path,
which is always
correct. Euclidean division is load-bearing -- at UTC, `-1` us is
`1969-12-31 23:59:59.999999`,
so truncation toward zero would give the wrong field for pre-epoch instants.
**2. Native `dayofweek` / `weekday` kernels** (new
`datetime_funcs/day_of_week.rs`)
The serde emitted `datepart('dow', child) + 1` and `datepart('isodow',
child) - 1`. For `Date32`,
arrow's `date_part` runs `unary_opt(|d| date32_to_datetime(d).map(..))` -- a
`NaiveDateTime` per
row plus a recomputed null mask -- and the `+1` / `-1` walked the result a
second time in a
separate plan node. Both collapse to one modulo of the epoch day, folded
into the kernel so the
arithmetic node is gone.
The `Int32` cast the serde used to emit was already a no-op (`date_part`
returns `Int32` for
these parts and `cast.rs` short-circuits an identity cast), so it is not
part of the win.
## How are these changes tested?
New Rust unit tests:
- `day_of_week.rs`: the week either side of the epoch pinning both numbering
conventions, null
preservation, the full `i32` domain, agreement between the two numberings
across a 400-year
Gregorian cycle, dictionary input, scalar input, and the non-date
rejection path.
- `extract_date_part.rs`: the UTC fast path against the general path on the
same instant,
pre-epoch instants (where truncation would diverge), null preservation,
and a check that an
offset session timezone is *not* accelerated.
Existing coverage: the full `datafusion-comet-spark-expr` suite passes (731
tests). On the Spark
side `CometExpressionSuite` already exercises `weekday`/`dayofweek` under
`checkSparkAnswerAndOperator` with and without dictionary encoding -- which
fails if the
expression stops running natively -- and this PR adds a date-column case
covering the epoch week,
leap days, the century rules, and dates at the edges of chrono's range. Both
suites pass.
### Behaviour change worth noting
For `dayofweek`/`weekday`, the old path returned NULL for epoch days outside
chrono's range,
because `date32_to_datetime` returns `None` there. The native kernel returns
a correct weekday
across the full `i32` domain. Spark's own date range is narrower, so no
realistic query reaches
those inputs, but the output is not bit-identical on them.
## Benchmarks
Release profile, 8192-row batches, aarch64, quiet machine, two samples each.
Both benchmarks
assert the replacement is bit-identical to the path it replaces before
timing anything.
**`benches/extract_clock_fields.rs`** -- invokes the real UDFs, compared
against a
`--save-baseline main` capture on unmodified source:
| shape | sample 1 | sample 2 |
| --- | --- | --- |
| `hour` / `minute` / `second`, NTZ, no nulls | -87.5% / -92.3% / -91.7% |
-87.5% / -92.3% / -91.7% |
| `hour` / `minute` / `second`, UTC session, no nulls | -96.4% / -95.2% /
-93.2% | -95.8% / -95.0% / -93.3% |
| NTZ, sparse nulls | -87.6% / -83.7% / -84.6% | -87.3% / -83.1% / -83.9% |
| UTC session, sparse nulls | -91.6% / -89.2% / -89.8% | -90.9% / -88.1% /
-90.0% |
| `America/Los_Angeles` (keeps the general path) | -0.9% to -21.4% | -2.2%
to -23.1% |
No shape regressed. The offset-timezone path is untouched code and drifts
slightly faster.
**`benches/dayofweek_weekday.rs`** -- both arms run in the same process (a
cross-run baseline is
impossible: the native kernels do not exist on `main`):
| shape | old chain | native kernel | sample 1 | sample 2 |
| --- | --- | --- | --- | --- |
| `dayofweek`, no nulls | 39.4 us | 4.06 us | 9.71x | 9.30x |
| `dayofweek`, sparse nulls | 32.7 us | 3.47 us | 9.42x | 8.35x |
| `weekday`, no nulls | 40.6 us | 3.99 us | 10.17x | 8.96x |
| `weekday`, sparse nulls | 46.8 us | 7.42 us | 6.30x | 8.67x |
Two samples are reported because on this hardware a library change shifts
codegen enough to move
*unrelated* bench arms by up to 60% reproducibly, so single cross-run deltas
below roughly +/-20%
on these benches are not trustworthy. The figures above are either far
outside that band or are
same-process comparisons.
A third target from the issue -- a year-only Gregorian split for
`iceberg_years` -- is
deliberately **not** included. It improved that transform by 30-54%, but
reproducibly slowed
`iceberg_days` sparse-null shapes by 16-24% through codegen layout, which
the no-regression gate
in `optimizing_expressions.md` does not allow. Details and numbers are in
https://github.com/apache/datafusion-comet/issues/5770#issuecomment-5586112182.
--
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]
