peterxcli commented on issue #5770: URL: https://github.com/apache/datafusion-comet/issues/5770#issuecomment-5586112182
Benchmarked all three targets. Summary of what held up and what did not. **Targets 1 and 2 confirmed, and larger than the issue estimated.** Measured on the real code paths (the `SparkHour`/`SparkMinute`/`SparkSecond` UDFs, and the new `spark_dayofweek` / `spark_weekday` kernels against the `datepart` + arithmetic chain they replace), release profile, 8192-row batches, aarch64, two samples each: | path | change | | --- | --- | | `hour`/`minute`/`second`, `TimestampNTZ` | **-83% to -92%** | | `hour`/`minute`/`second`, UTC session | **-88% to -96%** | | `dayofweek` / `weekday` | **8.4x to 10.2x** | No regression on the offset-timezone path (`America/Los_Angeles`), which keeps the existing `date_part` route. **Target 3 is withdrawn.** The year-only split is a genuine win on its own transform -- `iceberg_years` improved 30-54%, reproducible across two samples, far more than the 1.1-1.2x the standalone microbenchmark predicted. But it also made two shapes it does not touch semantically *reproducibly slower*: | shape | sample 1 | sample 2 | | --- | --- | --- | | `iceberg_days/date/sparse_nulls` | +17.7% | +24.2% | | `iceberg_days/timestamp/sparse_nulls` | +11.1% | +16.5% | `iceberg_days` uses `div_floor` and never touches the calendar split, so this is codegen/layout fallout from adding a function to that module, not a semantic change. The baseline for that comparison was captured with only `temporal.rs` reverted, so the change is the sole difference. Per the no-regression gate in `optimizing_expressions.md`, a 30-54% win on one transform does not buy a 16-24% loss on another, so it is not in the PR. Anyone picking this up should isolate the layout effect first (inlining attributes, or moving the helper) rather than assume the microbench number. **A methodology note for anyone re-running these.** Passing a kernel to `PrimitiveArray::unary` through a `black_box`ed function pointer blocks inlining and vectorization and understates the kernel by 2-3x. It also inverted a conclusion: a "biased epoch" variant of `civil_from_days` (plain `/` and `%` instead of `div_euclid`/`rem_euclid`) looked like a 1.2x win while both arms were equally pessimized, and measured 0.65-0.82x -- a clear regression -- once they inlined. Pass the kernel as a generic `F: Fn(..) -> i32 + Copy` and `black_box` the array instead. Also worth knowing: on this hardware a library change shifts codegen enough to move *unrelated* bench arms by up to 60% reproducibly, so cross-run criterion deltas below roughly +/-20% on these benches are not trustworthy. The numbers above are either far outside that band or were taken as same-process comparisons. -- 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]
