Skip to content

Rich t kid/introduce dict benchmarks#21860

Merged
kumarUjjawal merged 6 commits intoapache:mainfrom
Rich-T-kid:rich-t-kid/Introduce-dict-benchmarks
May 7, 2026
Merged

Rich t kid/introduce dict benchmarks#21860
kumarUjjawal merged 6 commits intoapache:mainfrom
Rich-T-kid:rich-t-kid/Introduce-dict-benchmarks

Conversation

@Rich-T-kid
Copy link
Copy Markdown
Contributor

@Rich-T-kid Rich-T-kid commented Apr 26, 2026

Which issue does this PR close?

This PR provides the benchmarks mentioned in #7647 & #9017

Rationale for this change

Currently the benchmark suite doesn't have any dictionary-encoded tables with aggregations performed on them. This makes it difficult to prove performance improvements, for example, a separate PR I'm working on (#21765) is hard to validate because the existing benchmarks don't exercise this path. This PR attempts to close that gap.

What changes are included in this PR?

Adds a new dict benchmark to dfbench that measures group-by performance on dictionary-encoded columns across varying cardinality (5/10/25%), null rates (0/15%), and value types (Utf8 and List), covering both single and multi-column group-by scenarios.

Are these changes tested?

--

Are there any user-facing changes?

no

@Rich-T-kid Rich-T-kid force-pushed the rich-t-kid/Introduce-dict-benchmarks branch from 9c07966 to c465861 Compare April 26, 2026 18:30
@Rich-T-kid Rich-T-kid force-pushed the rich-t-kid/Introduce-dict-benchmarks branch from c465861 to 7347a93 Compare April 26, 2026 18:38
@Rich-T-kid
Copy link
Copy Markdown
Contributor Author

@alamb these are the benchmarks for #21765, once their merged I can run a comparison between the current implementation and my approach

Comment thread benchmarks/src/dict.rs
Comment thread benchmarks/src/dict.rs Outdated
Comment thread benchmarks/bench.sh
@Rich-T-kid Rich-T-kid requested a review from kumarUjjawal May 5, 2026 21:36
Copy link
Copy Markdown
Contributor

@kumarUjjawal kumarUjjawal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's something wrong with github so I am not able to post comment on the line number but basically at line 372:

Is this check needed? `schema` is created from the same `query`, and `make_record_batch` always adds `dict_col2` when `query.col2` is `Some`. So this condition looks unreachable?

@Rich-T-kid
Copy link
Copy Markdown
Contributor Author

Yea I agree. Removed it

@Rich-T-kid Rich-T-kid requested a review from kumarUjjawal May 6, 2026 15:58
@Rich-T-kid Rich-T-kid force-pushed the rich-t-kid/Introduce-dict-benchmarks branch from f9e5ee5 to 5befe3f Compare May 6, 2026 16:31
@Rich-T-kid
Copy link
Copy Markdown
Contributor Author

@kumarUjjawal linting error broke the CI, just pushed up a fix

Copy link
Copy Markdown
Contributor

@kumarUjjawal kumarUjjawal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@kumarUjjawal kumarUjjawal added this pull request to the merge queue May 7, 2026
@kumarUjjawal
Copy link
Copy Markdown
Contributor

Thank you @Rich-T-kid

Merged via the queue into apache:main with commit 6b27d2d May 7, 2026
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants