chore: Bench agg (welford) stats - #5988
Conversation
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Reviewed 72890359 against base 20faa234.
This adds a separate Criterion target for grouped statistical aggregation: variance, covariance, correlation, floating-point average, exact/approximate percentile, and approximate distinct count. The two-file contribution changes benchmark registration and the harness. The aggregate implementations and production planner are unchanged.
The harness constructs Comet's UDFs directly with the expected Float64 arguments, including the percentage literal for exact percentile. DataFusion uses Comet's grouped accumulators where supported and its adapter around Comet's per-group accumulator for approximate percentile. Criterion awaits the async execution, and the harness drains the output stream and surfaces execution errors.
I compared the relevant Welford state/update and finalization behavior with the maintained Spark 3.5 and 4.0 branches. Those implementations maintain counts, means and moment/covariance state, with sample/population denominators and correlation normalization applied at final evaluation. This harness stops at AggregateMode::Partial, so it exercises updates and state emission without verifying those final results or merging states. Its finite, non-null Float64 data also leaves null handling, empty/singleton groups, overflow/NaN boundaries and legacy divide-by-zero modes outside the workload. It does not exercise Spark planning or fallback. Maintained 3.4/4.1 sources were unavailable. No compatibility claim is made for those versions.
At reviewed merge eb02e70eda3cf138114bc1ea339792dc659f7445, Rust CI passed all-target Clippy, cargo check --benches, and 1,477 Rust tests (five skipped). That establishes benchmark compilation, not a successful timed Criterion run. I did not run the benchmark or Spark/JVM tests locally. At the final refresh, all reported checks were complete: 24 passed and 14 skipped. No blocking correctness finding in this contribution.
Performance
Each iteration processes ten copies of an 8,192-row batch, grouped by 1,024 string keys: 80 rows and eight distinct input pairs per group. The two numerical columns are perfectly negatively correlated but have nonzero variance in every group, so the statistical update work is exercised.
The measurement includes plan/context construction, string grouping, accumulator updates, partial-state materialization and output disposal. Input generation and runtime construction are outside the loop. In particular, HLL precision 14 emits about 12.8 MiB of raw register values across these groups, before array/key overhead, so its result includes substantial state-output cost. These are useful grouped-operator measurements, but they should not be interpreted as isolated Welford kernel timings or Spark-versus-Comet speedups. No runtime performance result was produced during this review.
Design
The in-memory scan, explicit UDF binding and fully consumed aggregate stream make the workload straightforward to follow. Existing aggregate.rs already has direct statistical-accumulator benchmarks that call update_batch and evaluate. The incremental coverage here is grouped partial execution. That distinction also explains why these cases do not measure percentile finalization, HLL estimation, or the sample/population final denominator differences. The benchmark remains a bounded baseline rather than broad input-distribution coverage.
Abstraction & complexity
The small UDF-construction helpers and shared aggregate builder keep the cases consistent without changing runtime abstractions. The structure follows the existing operator benchmarks, and the limited duplication does not justify a new shared benchmark framework. No blocking abstraction or complexity issue found.
sunchao
left a comment
There was a problem hiding this comment.
Re-reviewed 5a40e5ec against 72890359 and base 58ab5f61. This revision only merges main. The benchmark source, aggregate implementations and dependency lockfile are unchanged. Its Cargo registration remains intact alongside the two benchmark targets added on main. No new or remaining verified P1/P2 findings.
Current Rust CI passed all-target Clippy, cargo check --benches, and 1,481 tests with five skipped. The job checked out a4106559, whose parents are this base/head and whose tree equals the head. This confirms compilation and test coverage, not a timed Criterion run. At September 17, 09:29 UTC, the current-head checks were complete: 23 passed and 14 were skipped. The PR Benchmark Check was skipped, so no timed benchmark result is credited.
The previous measurement qualifications still apply: this measures grouped partial updates and state output on fixed non-null data, including plan construction and grouping costs. It does not compare final numerical results, exercise merge/final evaluation, or establish a kernel or whole-query speedup. I rechecked the fixture shape and maintained Spark 3.5/4.0 sources. Maintained 3.4/4.1 sources remain unavailable. No local product build or benchmark was run.
Which issue does this PR close?
Part of #5396
Adds Comet-kernel benches for the agg_funcs accumulators not covered by aggregate.rs: variance (samp/pop), covariance (samp/pop), correlation, avg, approx_percentile, percentile, approx_count_distinct (HLL++, p10/p14)
Closes #.
Rationale for this change
What changes are included in this PR?
How are these changes tested?