Repository navigation
bench: add nested-type (List/Struct/Map) cases to first_value/last_value benchmark - #24075
Conversation
…mark Adds List, Struct, Map and a composite List<Struct> value column to the first_value / last_value benchmark, each with the same coverage as the primitive cases: first_value/last_value update + merge, plus first_value evaluate, at 0% and 90% nulls. prepare_typed_groups_accumulator now mirrors the planner: it uses the native GroupsAccumulator when the value type is supported and otherwise falls back to a GroupsAccumulatorAdapter around one per-group Accumulator. So the same benchmark case runs the fallback on a build without native nested support and the native path on a build that has it -- which lets a before/after benchmark run surface the speedup for the nested GroupsAccumulator work (e.g. first_value struct update ~112ms -> ~24ms at 1024 groups).
There was a problem hiding this comment.
Pull request overview
This PR extends the first_last Criterion benchmark in datafusion-functions-aggregate to cover nested value types (List/Struct/Map and a composite List), and updates the benchmark harness to exercise either the native GroupsAccumulator implementation or the adapter-based fallback path, depending on type support.
Changes:
- Add nested-type benchmark cases for
first_value/last_valueupdate + merge, plusfirst_valueevaluate, at two null-density settings. - Generalize the benchmark accumulator construction to be value-type aware and introduce an explicit adapter fallback factory.
- Add nested array generators (
StructArray,ListArray,MapArray,List<Struct>) for benchmark inputs.
Suppressed comments (2)
datafusion/functions-aggregate/benches/first_last.rs:450
- This label uses
nulls={pct}%, but in the nested generatorspctcontrols inner/payload null density (fields/elements/values) rather than top-level nulls. Renaming avoids misleading benchmark output.
&format!("{fn_label} merge_bench {type_label} nulls={pct}%"),
datafusion/functions-aggregate/benches/first_last.rs:461
- This label uses
nulls={pct}%, but in the nested generatorspctcontrols inner/payload null density (fields/elements/values) rather than top-level nulls. Renaming avoids misleading benchmark output.
&format!("first_value evaluate_bench {type_label} nulls={pct}%, all"),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…l nulls
- prepare_typed_groups_accumulator now decides native vs adapter via
groups_accumulator_supported() instead of catching create_groups_accumulator
errors, so genuine construction failures stay loud rather than silently
benchmarking the fallback.
- Nested generators now produce top-level null values (via a shared
top_level_nulls helper derived from arrow's null generator), so nulls={pct}%
matches the primitive semantics (the value itself is null) instead of only
nulling inner fields/elements/values.
53f34f3 to
ec11fef
Compare
There was a problem hiding this comment.
@zhuqi-lucas
Thanks for the quick benchmark!
I took a look at the changes and didn't find any blocking issues or suggestions. The touched code is benchmark-only, and the explicit native vs planner fallback construction makes sense here because it provides a clear comparison between #23628 and the current main branch.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24075 +/- ##
==========================================
- Coverage 80.89% 80.89% -0.01%
==========================================
Files 1102 1102
Lines 376085 376093 +8
Branches 376085 376093 +8
==========================================
- Hits 304230 304229 -1
- Misses 53747 53755 +8
- Partials 18108 18109 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thank you @kosiew for review, merged now. |
…lue benchmark (apache#24075) ## Which issue does this PR close? Groundwork for benchmarking apache#23628 (native `GroupsAccumulator` for nested value types in `first_value` / `last_value`). ## Rationale for this change The `first_last` benchmark only covers primitive value types today. apache#23628 adds a native `GroupsAccumulator` for **nested** value types (`List`, `Struct`, `Map`), which previously fell back to one per-group `Accumulator` via `GroupsAccumulatorAdapter`. To measure that work we need nested-type cases in the benchmark. Landing this first (with the fallback path on current `main`) means that once apache#23628 is in flight, a `run benchmark first_last` diff shows the fallback → native speedup per type automatically. ## What changes are included in this PR? - Adds `List<Int64>`, `Struct<i64,utf8,f64>`, `Map<utf8,i64>` and a composite `List<Struct<i64,utf8>>` value column, each with the same coverage as the primitive cases: `first_value`/`last_value` update + merge, plus `first_value` evaluate, at 0% and 90% nulls. - `prepare_typed_groups_accumulator` now mirrors the planner: it uses the native `GroupsAccumulator` when the value type is supported and otherwise falls back to a `GroupsAccumulatorAdapter` around one per-group `Accumulator`. The same benchmark case therefore runs the fallback on a build without native nested support and the native path on one that has it. ## Are these changes tested? Benchmark-only. Runs locally on `main` (all nested cases exercise the fallback path). As a preview of the intended signal, `first_value struct update` goes ~112ms (fallback) → ~24ms (native, with apache#23628) at 1024 groups. ## Are there any user-facing changes? No — benchmark only.
Which issue does this PR close?
Groundwork for benchmarking #23628 (native
GroupsAccumulatorfor nested value types infirst_value/last_value).Rationale for this change
The
first_lastbenchmark only covers primitive value types today. #23628 adds a nativeGroupsAccumulatorfor nested value types (List,Struct,Map), which previously fell back to one per-groupAccumulatorviaGroupsAccumulatorAdapter. To measure that work we need nested-type cases in the benchmark.Landing this first (with the fallback path on current
main) means that once #23628 is in flight, arun benchmark first_lastdiff shows the fallback → native speedup per type automatically.What changes are included in this PR?
List<Int64>,Struct<i64,utf8,f64>,Map<utf8,i64>and a compositeList<Struct<i64,utf8>>value column, each with the same coverage as the primitive cases:first_value/last_valueupdate + merge, plusfirst_valueevaluate, at 0% and 90% nulls.prepare_typed_groups_accumulatornow mirrors the planner: it uses the nativeGroupsAccumulatorwhen the value type is supported and otherwise falls back to aGroupsAccumulatorAdapteraround one per-groupAccumulator. The same benchmark case therefore runs the fallback on a build without native nested support and the native path on one that has it.Are these changes tested?
Benchmark-only. Runs locally on
main(all nested cases exercise the fallback path). As a preview of the intended signal,first_value struct updategoes ~112ms (fallback) → ~24ms (native, with #23628) at 1024 groups.Are there any user-facing changes?
No — benchmark only.