bench: add FixedSizeBinary coverage to multi_group_by benchmark#23650
Open
alamb wants to merge 1 commit into
Open
bench: add FixedSizeBinary coverage to multi_group_by benchmark#23650alamb wants to merge 1 commit into
alamb wants to merge 1 commit into
Conversation
Rich-T-kid
approved these changes
Jul 16, 2026
Rich-T-kid
left a comment
Rich-T-kid
left a comment
Contributor
There was a problem hiding this comment.
LGTM
nit: may want to varyFSB_WIDTH between benchmarks but this could also bloat the benchmarks
Adds a `fixed_size_binary` experiment to the multi_group_by benchmark that groups on a `(FixedSizeBinary(16), Int32)` key, comparing the vectorized columnar path (`GroupValuesColumn`) against the row-based fallback (`GroupValuesRows`) at 1,000 and 1,000,000 distinct groups. The distinct FixedSizeBinary values come from arrow-rs's `create_fsb_array` benchmark generator; rows cycle through that pool to control cardinality (as `generate_batches` does for Int32). Enabling `bench_util` requires the `test_utils` feature, so the bench entry now declares it. This gives coverage for the `FixedSizeBinaryGroupValueBuilder`: before FixedSizeBinary support, such a schema fell back to `GroupValuesRows`, so the `vectorized` vs `row_based` comparison measures the columnar speedup directly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alamb
force-pushed
the
bench-fsb-multi-group-by
branch
from
36fa31d to
da9f529
Compare
Contributor
Author
Thank you @Rich-T-kid -- I agree that the varying FSB_WIDTH would bloat things -- I think using FSB 16 (what is used for uuids) is probably a good enough coverage until we have a real usecase for a diffferent size |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #23650 +/- ##
=======================================
Coverage ? 80.65%
=======================================
Files ? 1086
Lines ? 366396
Branches ? 366396
=======================================
Hits ? 295508
Misses ? 53269
Partials ? 17619 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
The point of a specialized FixedSizeBinary group values is performance but we have no performance benchmark for it.
What changes are included in this PR?
Adds a
fixed_size_binaryexperiment todatafusion/physical-plan/benches/multi_group_by.rsRun with:
Are these changes tested?
This is benchmark-only. The benchmark compiles on
mainand runs end-to-endon top of #23646. No product code changes.
Are there any user-facing changes?
No.