chore: add_benches hash function aggregators - #5730
Conversation
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Reviewed 7278f549 against base f97fb451. This adds benchmark coverage for the Comet hash and aggregate kernels tracked in #5396. The four changed files add two Criterion targets and extend the existing aggregate target. They do not change production expressions, planning, fallback or error behavior. I found no P1/P2 issue in this scope.
The Murmur3 and xxHash64 cases use their required Int32 and Int64 seeds of 42, respectively, with Int64, UTF-8 and Float64 array inputs. The statistical cases select the intended population/sample variants, Float64 inputs and null-on-divide-by-zero mode. The median, approximate-percentile accuracy of 10,000 and HLL relative standard deviation of 0.05 match the corresponding kernel parameters. I checked the applicable seed, null and statistical contracts against the maintained Spark 3.5 and 4.0 sources. Maintained 3.4 and 4.1 sources were unavailable, so those versions are not independently qualified. Boundary values, overflow, special floating-point values and alternative modes are outside the new input matrix.
The benchmark job passed cargo check --benches, and the Rust job passed 1,167 tests with four skipped. Both checked out merge 809246eb, whose parents are the exact reviewed base and head. It uses DataFusion 55, while the unmerged head locks 54.1. The benchmark sources, relevant target registrations and scalar accumulator implementations match the executed merge. These checks establish compilation and existing native-test results, not execution of the new Criterion cases. No local benchmark run or timing result is claimed. The macOS Spark 4.0 scan job failed with a JVM SIGSEGV, so complete CI is not green. The log does not establish a defect in these benchmark additions.
Performance
The 11 aggregate configurations and two hash targets each cover three batch sizes and three null ratios, adding 117 cases. Input arrays are generated outside the timed iterations. The hash calls eagerly produce arrays, and each aggregate iteration creates fresh state, calls update_batch, evaluates the result and passes it through black_box. These calls bypass SQL constant folding and perform actual kernel work.
The aggregate measurements include UDF, schema, field and accumulator construction and destruction. Hash measurements include output allocation. This is a useful measure of processing one batch with fresh state, but it cannot isolate update-loop cost, especially for all-null inputs. The paired columns also share null positions, and HLL inputs cap distinct values at 4,096. There is no new Spark/DataFusion comparison arm or reported speedup. These cases therefore provide regression inputs without establishing relative engine performance or an end-to-end improvement.
Design
Keeping the hash entry points in separate targets and extending the existing aggregate target fits the repository's Criterion organization. Driving the scalar accumulator directly gives these cases a clear scope and avoids introducing execution-plan setup into every aggregate benchmark. Fresh accumulators prevent state from accumulating across iterations and keep percentile evaluation independent between samples. Grouped aggregation, merging partial states and multi-batch execution remain separate coverage areas.
Abstraction & complexity
The shared accumulator driver centralizes the required argument construction and update/evaluate lifecycle. The one- and two-argument helpers remove repeated matrix setup while keeping the concrete kernel, types and parameters visible at each registration. The current factories ignore the unused expression list, so it does not omit an argument consumed by these kernels. The added helpers are confined to benchmarks and do not introduce a production abstraction. I found no actionable simplification needed before merging.
Which issue does this PR close?
Part of #5396
Rationale for this change
What changes are included in this PR?
benches for hash functions and aggregated functions (custom comet kernels only). I added new agg benches to existing bench file to keep things simple and consistent with the repo approach
How are these changes tested?
Benches only