Repository navigation
chore: add benches for remaining agg functions - #6647
Conversation
sunchao
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem: Grouped benchmarks lacked coverage for
mode,max_by,min_by,regr_slope, andlistagg. - Design approach: Adds five Criterion cases using grouped
AggregateMode::Partialover 81,920 rows and 1,024 groups. - Correctness / compatibility analysis: Constructors and argument order match the production planner. Checked relevant Spark sources across all five supported profiles. No introduced P1/P2 issues found within this review.
- Key design decisions: Data generation and runtime creation occur outside timing. Plan construction, aggregation, and partial-state output are timed, consistent with neighboring benchmarks.
- Implementation sketch: Registers
aggregate_misc, builds in-memory execution plans through a small helper, and drains each result stream. - Behavioral changes worth calling out: Adds benchmark coverage only. These cases measure partial aggregation, not final-result evaluation or Spark-versus-Comet performance.
- Suggested improvements: None meeting the P1/P2 reporting bar.
Reviewed full SHA 8115f6726b9025f25d6b8e75c6cda2d8f0f1a307 against base 33f21da181c14df7fa3e6a3104c3d7215810147e, using the complete merge-base PR diff: two files, 223 additions. The PR is not a draft. Existing reviews, comments, and threads were empty. Routed skills: review-comet-pr and review-comet-expression-pr.
Validation: Targeted cargo check --locked --offline, Clippy with -D warnings, and cargo test --locked --offline -p datafusion-comet-spark-expr --bench aggregate_misc passed. All five benchmark smoke cases succeeded. No project source changes were made.
Exact-head CI: 17 checks passed, two remain running, and 14 were skipped. Native-library build and Rust tests are still running. The initial title-format failure has a later passing run. Benchmark-specific, macOS, Spark SQL, and Iceberg jobs were skipped. Validation limits: no optimized performance measurements or end-to-end Spark suites were run locally, and CI is not yet complete.
Which issue does this PR close?
Part of Closes #5396
Closes #.
Rationale for this change
What changes are included in this PR?
How are these changes tested?