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/08 13:49:43 UTC
[PR] perf: skip calendar reconstruction in hour/minute/second and dayofweek/weekday (up to 23x) [datafusion-comet]
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: 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: skip calendar reconstruction in hour/minute/second and dayofweek/weekday [datafusion-comet]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #5771:
URL: https://github.com/apache/datafusion-comet/pull/5771#issuecomment-5612275126
All three are fixed in 9b9817f8f. The density finding was correct and is the one that mattered.
**`dense` was silently all-null.** Reproduced exactly: `common::is_null` derives `round(1 / ratio)`, which is 1 for any ratio above two thirds, so 0.875 gave 8192 nulls and zero valid rows. The stride is now taken over the *valid* rows above half, which yields the intended 7168 / 1024. Ratios at or below half are untouched, and no existing caller passes anything in `(0.5, 1.0)` -- `NULL_RATIOS` is 0.0, 0.1, 1.0 -- so no other benchmark's shapes move. `assert_density` now checks the realised null count against the requested ratio before timing, so a degenerate shape fails rather than being measured.
Worth separating: the earlier -70% to -87% dense figures came from the old `extract_clock_fields.rs`, whose own selector was a genuine `i % 8 != 0`. The defect arrived with the consolidation, so it never produced a published number -- but the harness in the PR was wrong and you caught it before it did.
**`assert_matches_arithmetic` was dropped in the fold.** Correct, and the PR body's claim was false for that harness; both are fixed. Every clock shape now gets a null-aware check -- null exactly where the input is null, values inside the field's range -- and shapes where no session offset applies additionally get an exact oracle recomputed from the generator rather than from arrow, so it validates the baseline implementation as well as this one. Dictionary and offset-session controls get the null-aware half, since no arithmetic oracle applies to them. The baseline run passes all 144 checks too.
**Matched results after the repair.** 144 shapes, baseline captured on `bb9e74020` source, two head samples, absolute medians with 95% CI.
51 fast-path shapes have a baseline at or above 2 us and every one improves, by -82% to -94%:
| shape | baseline us | head us | s1 | s2 |
| --- | --- | --- | --- | --- |
| `hour/utc_session/tz/8192/no_nulls` | 92.85 [89.72, 96.97] | 5.48 [5.47, 5.48] | -89.7% | -94.1% |
| `minute/utc_session/tz/8192/no_nulls` | 113.71 [109.74, 117.73] | 7.13 [7.10, 7.16] | -89.2% | -93.5% |
| `hour/la_session/ntz/524288/no_nulls` | 3140.50 [3105.00, 3177.90] | 291.92 [290.45, 293.62] | -89.2% | -90.6% |
| `hour/utc_session/ntz/8192/no_nulls` | 55.15 [50.02, 63.27] | 5.46 [5.46, 5.47] | -82.4% | -89.6% |
Thirteen shapes were slower in both samples. The repeat does not support any of them as cost:
| shape | baseline us | head us (s2) | s1 | s2 | path |
| --- | --- | --- | --- | --- | --- |
| `minute/la_session/tz/8192/dense` | 20.97 [20.77, 21.33] | 21.13 [20.98, 21.31] | +88.1% | +0.6% | control |
| `minute/la_session/tz/8192/no_nulls` | 175.28 [171.90, 180.71] | 173.82 [173.47, 174.20] | +77.1% | +0.5% | control |
| `hour/la_session/dict1024/8192/no_nulls` | 6.52 [6.51, 6.54] | 6.61 [6.58, 6.66] | +37.0% | +1.3% | control |
| `hour/la_session/tz/8192/all_null` | 0.89 [0.89, 0.89] | 0.96 [0.96, 0.97] | +2.0% | +7.9% | control |
| `hour/la_session/ntz/524288/all_null` | 16.70 [16.68, 16.73] | 17.32 [17.16, 17.54] | +14.9% | +3.4% | fast |
On the control comparison you asked for: the large sample-1 numbers on the `minute` and `second` `la_session` shapes are interference partway through that run. `hour/la_session` runs earlier in the same process and was stable at -5.3% / -4.7% across both samples on identical code, and the sample-2 absolutes land within a microsecond of the baseline -- `minute/la_session/tz/8192/no_nulls` is 175.28 -> 173.82 us, slightly *faster*. The largest movement that reproduces is `hour/la_session/tz/8192/all_null` at +2.0% / +7.9%, which is 0.89 -> 0.96 us; for an all-null batch both sides call `unary_opt` and visit zero valid indices, so there is no algorithmic difference there to explain a cost.
You are also right that the sparse ratio moved from 12.5% to the shared 10% convention, so the tables above supersede the earlier ones entirely rather than extending them; the PR body now carries these numbers with the caveats inline.
Still not established here, and not claimed: any query-level result, and anything on x86-64.
--
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: skip calendar reconstruction in hour/minute/second and dayofweek/weekday [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao merged PR #5771:
URL: https://github.com/apache/datafusion-comet/pull/5771
--
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: skip calendar reconstruction in hour/minute/second and dayofweek/weekday [datafusion-comet]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #5771:
URL: https://github.com/apache/datafusion-comet/pull/5771#issuecomment-5602022090
Thanks for checking the arithmetic against Spark's helpers, and for the `Etc/GMT+1` / `Europe/London` boundary in particular — those are the two that would quietly break the allowlist. All three points are addressed in bb8a04858, on top of a merge of `upstream/main`.
**Which branch runs in CI.** You were right that nothing pinned it: `CometHour` takes its zone from `expr.timeZoneId`, which Spark fills from the session zone, so the branch followed whichever runner executed the suite. `hour.sql`, `minute.sql` and `second.sql` now carry `-- ConfigMatrix: spark.sql.session.timeZone=UTC,America/Los_Angeles`, and each gained a `1969-12-31 23:59:59` row in both the timestamp and the NTZ table so the Euclidean division is covered end to end. Each file now runs as two variants and both pass locally on Spark 3.5 / JDK 17.
**Two benchmark files for the same three UDFs.** Also correct, and the reason is that `benches/extract_date_part.rs` landed upstream after my branch point — I had not merged since. Merged now, the new shapes folded into that file, and `extract_clock_fields.rs` deleted. It uses the shared `common/` helpers, sweeps both session zones so each dispatch branch is actually measured (the file previously only ever constructed `America/Los_Angeles`, so the timezone-aware fast path was unreachable), adds a dense null ratio alongside `NULL_RATIOS`, adds dictionary inputs at cardinality 8 and 1024, and generates instants spanning the epoch instead of a fixed 2020 base. Upstream's existing shape ids are preserved under the `la_session` prefix, with the full `ROW_COUNTS` sweep kept there and the additions at 8192 rows so the matrix stays affordable.
**PR body and title.** Updated. The title no longer claims 23x — that figure came from a `ci`-profile run before the dense-null and dictionary fixes — and the tables now carry the post-fix numbers, with the shapes that still move the wrong way stated in the body rather than only in a thread reply, since the squash message is what survives.
One thing I did not change but that is worth someone's attention: the comment block in `hour.sql` says the native implementation is `Incompatible` for `TimestampNTZType` per #3180 and is therefore routed through the codegen dispatcher. `CometHour` has no `getSupportLevel` override — it is plain `Compatible` — so NTZ does reach the native path and that comment looks stale. I left it alone rather than widen this PR, but it will mislead the next reader of that file.
--
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: skip calendar reconstruction in hour/minute/second and dayofweek/weekday [datafusion-comet]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #5771:
URL: https://github.com/apache/datafusion-comet/pull/5771#issuecomment-5694085042
Thanks for the review!
--
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: skip calendar reconstruction in hour/minute/second and dayofweek/weekday (up to 23x) [datafusion-comet]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #5771:
URL: https://github.com/apache/datafusion-comet/pull/5771#discussion_r3964696960
##########
native/spark-expr/benches/dayofweek_weekday.rs:
##########
@@ -0,0 +1,135 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! `dayofweek` / `weekday` over `Date32`, comparing the chain Comet serializes today against a
+//! direct integer kernel on the epoch day.
+//!
+//! The `datepart_*` arms reproduce the chain the serde emitted before this change:
+//! `datepart('dow', child) + 1` for `dayofweek` and `datepart('isodow', child) - 1` for
+//! `weekday`. The `native_*` arms invoke the kernels the serde emits now. Both arms run in the
+//! same process, so this is a direct comparison rather than a cross-run baseline (the native
+//! kernels do not exist on `main`, so a saved baseline could not build them). `datepart` resolves to DataFusion's `date_part`, which for
+//! `Date32` runs `unary_opt(|d| date32_to_datetime(d).map(..))` -- a `NaiveDateTime` per row plus
+//! a recomputed null mask -- and the `+ 1` / `- 1` is a second pass over the result.
+
+use arrow::array::{Array, ArrayRef, Date32Array, Int32Array, Scalar};
+use arrow::compute::kernels::numeric::{add_wrapping, sub_wrapping};
+use arrow::compute::{date_part, DatePart};
+use arrow::datatypes::DataType;
+use arrow::datatypes::Field;
+use criterion::{criterion_group, criterion_main, Criterion, Throughput};
+use datafusion::config::ConfigOptions;
+use datafusion::logical_expr::{ColumnarValue, ScalarFunctionArgs, ScalarUDFImpl};
+use datafusion_comet_spark_expr::{SparkDayOfWeek, SparkWeekDay};
+use std::hint::black_box;
+use std::sync::Arc;
+
+const ROWS: usize = 8_192;
+const NULL_STRIDE: usize = 8;
+
+/// Epoch days spread over roughly 1970..2050, the range a date column actually holds.
+fn dates(nulls: bool) -> Date32Array {
+ (0..ROWS)
+ .map(|i| {
+ if nulls && i.is_multiple_of(NULL_STRIDE) {
+ None
+ } else {
+ Some((i as i32).wrapping_mul(3) % 29_220)
+ }
+ })
+ .collect()
+}
+
+// ---- current path -------------------------------------------------------------------------
+
+fn current_dayofweek(array: &ArrayRef) -> ArrayRef {
+ let part = date_part(array.as_ref(), DatePart::DayOfWeekSunday0).unwrap();
+ add_wrapping(&part, &Scalar::new(Int32Array::from(vec![1]))).unwrap()
+}
+
+fn current_weekday(array: &ArrayRef) -> ArrayRef {
+ let part = date_part(array.as_ref(), DatePart::DayOfWeekMonday1).unwrap();
+ sub_wrapping(&part, &Scalar::new(Int32Array::from(vec![1]))).unwrap()
+}
+
+// ---- native kernels, as the serde now emits them --------------------------------------------
+
+fn invoke(udf: &dyn ScalarUDFImpl, array: &ArrayRef) -> ArrayRef {
+ udf.invoke_with_args(ScalarFunctionArgs {
+ args: vec![ColumnarValue::Array(Arc::clone(array))],
+ arg_fields: vec![Arc::new(Field::new("d", array.data_type().clone(), true))],
+ number_rows: ROWS,
+ return_field: Arc::new(Field::new(udf.name(), DataType::Int32, true)),
+ config_options: Arc::new(ConfigOptions::default()),
+ })
+ .unwrap()
+ .to_array(ROWS)
+ .unwrap()
+}
+
+/// The benchmark is only meaningful if both arms agree, so check before timing.
+fn assert_equivalent(array: &Date32Array) {
+ let dyn_array: ArrayRef = Arc::new(array.clone());
+ for (current, native) in [
+ (
+ current_dayofweek(&dyn_array),
+ invoke(&SparkDayOfWeek::new(), &dyn_array),
+ ),
+ (
+ current_weekday(&dyn_array),
+ invoke(&SparkWeekDay::new(), &dyn_array),
+ ),
+ ] {
+ assert_eq!(
+ current.as_any().downcast_ref::<Int32Array>().unwrap(),
+ native.as_any().downcast_ref::<Int32Array>().unwrap(),
+ );
+ }
+}
+
+fn criterion_benchmark(c: &mut Criterion) {
+ for (nulls, null_tag) in [(false, "no_nulls"), (true, "sparse_nulls")] {
Review Comment:
Thanks — both mechanisms you named were real, and adding the shapes found two regressions that the previous harness structurally could not have surfaced. Fixed in 1c84bacbd.
**`unary_opt` vs `unary`.** Confirmed: `unary_opt` visits only valid indices via `try_for_each_valid_idx`, so the replaced path skipped null slots and the kernel did not. Flat all-null dayofweek measured **0.30x** — a 3.3x slowdown.
**Dictionary decode.** Also confirmed: `date_part` maps a dictionary's values and rewraps the keys, so the old chain's calendar work was proportional to cardinality. Decoding to `Date32` first made cardinality 8 measure **0.51x**.
Both fixed. `map_dates` and the clock fast path now switch on null *density*, and the weekday kernels map dictionary values and rewrap rather than decoding. Two details worth recording, since both cost a factor of ~2.5x and neither is obvious:
- The threshold is half the batch, not "any null present". Switching on the mere presence of a null dropped sparse-null dayofweek from 8.97x to 3.45x, and `unary` was still ahead at 87.5% nulls, so skipping only pays once most of the batch is null.
- `map_dates` takes a generic `F: Fn(i32) -> i32 + Copy`. As a `fn` pointer it forced an indirect call per element inside `unary` and cut no-null dayofweek from 9.0x to 3.7x.
I also found the old-path arm in the weekday benchmark had `Cast` and `Add` inverted. The serde emitted `Add(Cast(datepart(..), Int32), 1)` — the cast came first and is what unpacked the dictionary — and arrow rejects `Dictionary(Int32, Int32) + Int32` outright, so the dictionary comparison could not have run at all as written. And you were right about the generator: its endpoints put every value in 1965-1966. It now walks 1875 to 2064, crossing the epoch at row 4096.
### dayofweek / weekday, real UDF vs the complete old chain, two samples
| shape | before the fix | sample 1 | sample 2 |
| --- | --- | --- | --- |
| flat / no nulls | 8.98x | 9.04x | 8.98x |
| flat / sparse nulls (12.5%) | 8.97x | 8.65x | 8.83x |
| flat / dense nulls (87.5%) | 1.60x | 3.66x | 3.27x |
| flat / all nulls | **0.30x** | 1.17x | 1.28x |
| dict cardinality 8 | **0.51x** | 1.03-1.14x | 1.04-1.18x |
| dict cardinality 1024 | 1.19x | 1.95-2.17x | 2.06-2.26x |
One shape is still marginal: `weekday/flat/all_nulls` at 0.89x / 0.97x. It is ~1 us absolute, both arms skip every slot and do identical work, and the sibling `dayofweek/flat/all_nulls` runs the same code at 1.17x / 1.28x. I am reporting it rather than calling it noise.
### hour / minute / second, vs a `--save-baseline` capture
The baseline is taken from `bb9e74020` source, not `HEAD` — HEAD already contains the fast path, so a `git checkout HEAD` baseline measured the optimized code against itself. Negative is faster.
| shape | sample 1 | sample 2 | absolute |
| --- | --- | --- | --- |
| ntz / no nulls | -87.3% to -82.3% | -87.1% to -85.4% | 5.5-8.0 us |
| ntz / sparse nulls | -87.2% to -83.4% | -87.3% to -84.9% | 5.7-7.7 us |
| ntz / dense nulls | -71.5% to -68.5% | -69.1% to -67.9% | 2.2-2.3 us |
| utc session / no nulls | -91.7% to -90.7% | -92.8% to -90.7% | 5.6-7.6 us |
| utc session / sparse nulls | -92.0% to -89.6% | -92.0% to -89.1% | 5.5-7.8 us |
| utc session / dense nulls | -80.8% to -80.1% | -81.4% to -78.8% | 2.3-2.8 us |
Dense nulls are now a 68-81% win rather than the regression they would have been.
### What still moves the wrong way
Two groups, both reported rather than argued away:
1. **All-null clock shapes**, +9% to +64% in sample 1 and +9% to +15% in sample 2. These are 0.8-1.0 us absolute: +64% is ~0.37 us over 8192 rows, about 45 ps/row. For an all-null batch both arms call `unary_opt` and visit zero valid indices, so there is no algorithmic difference left — only allocation and codegen.
2. **Paths the fast path never enters** — `la_session` and dictionary timestamps — at +2% to +25% (sample 1) and +2% to +18% (sample 2). These cannot be caused by the change semantically; the per-batch guard is one type match plus a string compare. Earlier in this PR I measured untouched benchmark arms drifting up to 60% reproducibly when the library changes, so I treat sub-20% movement on untouched paths here as layout/codegen fallout rather than a real cost.
Every shape that takes the fast path and has meaningful cost (>=2 us) improves by 68-93%, reproducible to under 1% between samples.
731 Rust tests pass, clippy is clean under `-D warnings`, and both benchmarks keep null-aware equivalence assertions — now comparing nullability per row — that run before any timing on every shape, including the dictionary and all-null ones.
--
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: skip calendar reconstruction in hour/minute/second and dayofweek/weekday [datafusion-comet]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #5771:
URL: https://github.com/apache/datafusion-comet/pull/5771#issuecomment-5676851313
Thanks both. All three leftovers are fixed in b7e909bbf, docs and comment only — no runtime or benchmark change.
- **Audit page, clock entries.** `hour`, `minute` and `second` now point at `benches/extract_date_part.rs` instead of the deleted `extract_clock_fields.rs`, and the stale "83-96% faster" is replaced with the matched result: every fast-path shape with a baseline of at least 2 us takes 82-94% less time, with offset session zones and dictionary input on the general path and within noise on a repeat.
- **Audit page, weekday entries.** `dayofweek` and `weekday` no longer carry "8.4-10.2x". They now say about 9x on flat input with no or sparse nulls, 3-4x at 87.5% nulls, roughly 1.1x on an all-null batch or a cardinality-8 dictionary where the old path already did little work, and about 2x at cardinality 1024 — so the page matches the tables rather than the pre-fix headline.
- **`datetime.scala`.** The two Sunday-is-0 comment lines left behind by removing `val DayOfWeek` are gone, so nothing sits above `val DayOfYear` any more.
On CI: the new head re-runs the whole workflow, which should get past the expired `native-lib-linux` artifact that stopped the September 14 scan rerun before any tests ran. On the Spark 3.4 `[scans]` crash itself, I'm going on Andy's triage that it is the #5023 libhdfs SIGSEGV. As sunchao noted, the surviving artifact shows a native-thread SIGSEGV at address zero but doesn't name `hdfsThreadDestructor`, and I haven't independently confirmed the cause. If it recurs on this run, I'll attach the crash log to #5023 rather than work around it here.
I left the branch at its current merge base rather than pull in `main` again, since it is mergeable and I wanted this push to be only the doc and comment fix.
--
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: skip calendar reconstruction in hour/minute/second and dayofweek/weekday (up to 23x) [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #5771:
URL: https://github.com/apache/datafusion-comet/pull/5771#discussion_r3961137967
##########
native/spark-expr/benches/dayofweek_weekday.rs:
##########
@@ -0,0 +1,135 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! `dayofweek` / `weekday` over `Date32`, comparing the chain Comet serializes today against a
+//! direct integer kernel on the epoch day.
+//!
+//! The `datepart_*` arms reproduce the chain the serde emitted before this change:
+//! `datepart('dow', child) + 1` for `dayofweek` and `datepart('isodow', child) - 1` for
+//! `weekday`. The `native_*` arms invoke the kernels the serde emits now. Both arms run in the
+//! same process, so this is a direct comparison rather than a cross-run baseline (the native
+//! kernels do not exist on `main`, so a saved baseline could not build them). `datepart` resolves to DataFusion's `date_part`, which for
+//! `Date32` runs `unary_opt(|d| date32_to_datetime(d).map(..))` -- a `NaiveDateTime` per row plus
+//! a recomputed null mask -- and the `+ 1` / `- 1` is a second pass over the result.
+
+use arrow::array::{Array, ArrayRef, Date32Array, Int32Array, Scalar};
+use arrow::compute::kernels::numeric::{add_wrapping, sub_wrapping};
+use arrow::compute::{date_part, DatePart};
+use arrow::datatypes::DataType;
+use arrow::datatypes::Field;
+use criterion::{criterion_group, criterion_main, Criterion, Throughput};
+use datafusion::config::ConfigOptions;
+use datafusion::logical_expr::{ColumnarValue, ScalarFunctionArgs, ScalarUDFImpl};
+use datafusion_comet_spark_expr::{SparkDayOfWeek, SparkWeekDay};
+use std::hint::black_box;
+use std::sync::Arc;
+
+const ROWS: usize = 8_192;
+const NULL_STRIDE: usize = 8;
+
+/// Epoch days spread over roughly 1970..2050, the range a date column actually holds.
+fn dates(nulls: bool) -> Date32Array {
+ (0..ROWS)
+ .map(|i| {
+ if nulls && i.is_multiple_of(NULL_STRIDE) {
+ None
+ } else {
+ Some((i as i32).wrapping_mul(3) % 29_220)
+ }
+ })
+ .collect()
+}
+
+// ---- current path -------------------------------------------------------------------------
+
+fn current_dayofweek(array: &ArrayRef) -> ArrayRef {
+ let part = date_part(array.as_ref(), DatePart::DayOfWeekSunday0).unwrap();
+ add_wrapping(&part, &Scalar::new(Int32Array::from(vec![1]))).unwrap()
+}
+
+fn current_weekday(array: &ArrayRef) -> ArrayRef {
+ let part = date_part(array.as_ref(), DatePart::DayOfWeekMonday1).unwrap();
+ sub_wrapping(&part, &Scalar::new(Int32Array::from(vec![1]))).unwrap()
+}
+
+// ---- native kernels, as the serde now emits them --------------------------------------------
+
+fn invoke(udf: &dyn ScalarUDFImpl, array: &ArrayRef) -> ArrayRef {
+ udf.invoke_with_args(ScalarFunctionArgs {
+ args: vec![ColumnarValue::Array(Arc::clone(array))],
+ arg_fields: vec![Arc::new(Field::new("d", array.data_type().clone(), true))],
+ number_rows: ROWS,
+ return_field: Arc::new(Field::new(udf.name(), DataType::Int32, true)),
+ config_options: Arc::new(ConfigOptions::default()),
+ })
+ .unwrap()
+ .to_array(ROWS)
+ .unwrap()
+}
+
+/// The benchmark is only meaningful if both arms agree, so check before timing.
+fn assert_equivalent(array: &Date32Array) {
+ let dyn_array: ArrayRef = Arc::new(array.clone());
+ for (current, native) in [
+ (
+ current_dayofweek(&dyn_array),
+ invoke(&SparkDayOfWeek::new(), &dyn_array),
+ ),
+ (
+ current_weekday(&dyn_array),
+ invoke(&SparkWeekDay::new(), &dyn_array),
+ ),
+ ] {
+ assert_eq!(
+ current.as_any().downcast_ref::<Int32Array>().unwrap(),
+ native.as_any().downcast_ref::<Int32Array>().unwrap(),
+ );
+ }
+}
+
+fn criterion_benchmark(c: &mut Criterion) {
+ for (nulls, null_tag) in [(false, "no_nulls"), (true, "sparse_nulls")] {
Review Comment:
### Performance
[P2] Cover dense-null and dictionary shapes in the no-regression comparison
Could both benchmark targets include dense-null/all-null inputs and representative dictionary cases before concluding that no shape regresses? This loop only tests plain arrays with 0% or 12.5% nulls. The replacement changes a relevant cost boundary: Arrow's old `unary_opt` only evaluates valid slots, while the new `unary` evaluates every slot. The weekday UDF's dictionary decode path is also absent from the timing comparison. Please compare the actual UDF with the complete old path, including decoding where required, for low and higher dictionary cardinalities.
Please also make the [clock input generator](https://github.com/apache/datafusion-comet/blob/fd4c716c2126550a101bd071e26cf9f03b9996f9/native/spark-expr/benches/extract_clock_fields.rs#L39-L52) genuinely span negative and positive instants. Its current endpoints are `-150000000000000` and `-126053798164699` microseconds, so every generated value is in 1965–1966. Retain null-aware equivalence checks and report the matched baseline/head results for these shapes. This asks for the missing evidence required by the repository's [optimization guide](https://github.com/apache/datafusion-comet/blob/fd4c716c2126550a101bd071e26cf9f03b9996f9/docs/source/contributor-guide/optimizing_expressions.md#cover-the-shapes-that-break-optimizations), not a claim that a slowdown has been measured.
--
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