[Minor] add non topk benchmarks for utf8/utf8view string aggregates#21073
[Minor] add non topk benchmarks for utf8/utf8view string aggregates#21073buraksenn wants to merge 3 commits intoapache:mainfrom
Conversation
| let ctx = rt | ||
| .block_on(create_context(partitions, samples, asc, use_topk, use_view)) | ||
| .unwrap(); | ||
| c.bench_function(&name, |b| { |
There was a problem hiding this comment.
These new TopK-disabled string cases expand the matrix, but they still only go through run_string()/aggregate_string(), which currently checks row-count and whether the physical plan contains lim=[...]. That means this PR does not actually verify correctness across Utf8 and Utf8View group keys, even though that is part of the motivation. Can we strengthen the string benchmark path with an expected-result assertion (or add a dedicated helper/test that compares Utf8 vs Utf8View output for both TopK modes) so the new variants catch ordering/value regressions instead of only plan-shape changes?
There was a problem hiding this comment.
I wrongly assumed we wanted to only check performance but I understand. I've added a assert to check results of each
| let dir = if asc { "asc" } else { "desc" }; | ||
| let topk_label = if use_topk { "TopK" } else { "no TopK" }; | ||
| let name = format!("distinct {total_rows} rows {dir} [{topk_label}]"); | ||
| let ctx = rt.block_on(async { |
There was a problem hiding this comment.
This refactor now rebuilds the DISTINCT input/context once per (use_topk, asc) pair, even though asc only affects the query text and the old version shared one context per TopK mode. It is outside the timed loop, so not a benchmark-result bug, but it does add a lot of setup work for 10M rows. Could we hoist context creation by use_topk again, or extract a small helper that caches the two contexts?
There was a problem hiding this comment.
I understand I think I've resolved this by using and cloning the session (shallow copy afais) but can you check if possible
| "top k={limit} aggregate {rows} worst-case rows [Utf8View]", | ||
| ), | ||
| ]; | ||
| for &(asc, use_topk, use_view, run_asc, name_tpl) in numeric_cases { |
There was a problem hiding this comment.
Could this tuple be simplified a bit? run_asc currently mirrors asc in every entry, so carrying both booleans makes the benchmark matrix harder to audit and easier to desynchronize later. Passing asc straight through (or switching to a small case struct with named fields) would make the intent clearer.
There was a problem hiding this comment.
Makes sense, I've got rid of run_asc and reordered cases. I can also introduce a struct if requested
|
Thanks @kosiew for the detailed review |
buraksenn
changed the title
2 participants
Which issue does this PR close?
Rationale for this change
Details are in #19713 but main idea is to compare non-topk and topk test results so that we can compare performances
What changes are included in this PR?
Added non topk benchmark tests.
Are these changes tested?
Only test changes
Are there any user-facing changes?
No