zhuqi-lucas commented on code in PR #24559:
URL: https://github.com/apache/datafusion/pull/24559#discussion_r3831269109
##########
datafusion/functions-aggregate/benches/first_last.rs:
##########
@@ -452,6 +452,103 @@ fn create_list_of_struct_array(n: usize, list_len: usize,
null_density: f32) ->
))
}
+/// Head-to-head for the coalesce-peers rewrite: N independent primitive
+/// `first_value` accumulators (the pre-rewrite plan) vs one struct-valued
+/// accumulator carrying the same N columns (the post-rewrite plan). Uses the
+/// same worst-case ordering as [`update_bench`] so every row forces an
+/// ordering comparison in every accumulator.
+#[expect(clippy::needless_pass_by_value)]
+fn coalesce_comparison_bench(
+ c: &mut Criterion,
+ name: &str,
+ column_values: Vec<ArrayRef>,
+ struct_values: ArrayRef,
+ ord: ArrayRef,
+ num_groups: usize,
+) {
+ let n = ord.len();
+ let group_indices: Vec<usize> = (0..n).map(|i| i % num_groups).collect();
+ let worst_ord: ArrayRef = Arc::new(Int64Array::from(vec![i64::MAX; n]));
+
+ // Pre-rewrite: one accumulator per column.
+ c.bench_function(&format!("{name} separate x{}", column_values.len()), |b|
{
+ b.iter_batched(
+ || {
+ column_values
+ .iter()
+ .map(|values| {
+ let mut acc = prepare_typed_groups_accumulator(
+ true,
+ values.data_type().clone(),
+ );
+ acc.update_batch(
+ &[Arc::clone(values), Arc::clone(&worst_ord)],
+ &group_indices,
+ None,
+ num_groups,
+ )
+ .unwrap();
+ acc
+ })
+ .collect::<Vec<_>>()
+ },
+ |mut accumulators| {
+ for _ in 0..100 {
+ for (acc, values) in
accumulators.iter_mut().zip(&column_values) {
+ #[expect(clippy::unit_arg)]
+ black_box(
+ acc.update_batch(
+ &[Arc::clone(values), Arc::clone(&ord)],
Review Comment:
Thanks for the review — good catch. You're right: reusing one `ord` meant
iters 2..100 only hit compare-and-reject, so the update path wasn't measured.
Pushed a `(winner changes)` variant that feeds a strictly-decreasing `ord`
per iteration, so every row becomes a new winner and the running value is
replaced+copied every time — this exercises the update path (where the struct
plan copies one wider row vs N narrow ones) that the reused-array version
skipped. Kept the original as `(winner stable)` so both the compare-reject and
compare-replace paths are covered. Will post fresh numbers once the bot reruns.
--
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]
