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: [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]
