Skip to content

introduce new dictionary benchmarks - #24416

Open
Rich-T-kid wants to merge 4 commits into
apache:mainfrom
Rich-T-kid:rich-T-kid/cache-dictionary-arc-ptr
Open

introduce new dictionary benchmarks#24416
Rich-T-kid wants to merge 4 commits into
apache:mainfrom
Rich-T-kid:rich-T-kid/cache-dictionary-arc-ptr

Conversation

@Rich-T-kid

@Rich-T-kid Rich-T-kid commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

The existing benchmarks only cover the vectorized (GroupOrdering::None) intern/emit path. This adds coverage for the scalar append_val/equal_to path and the rolling EmitTo::First (take-n) emit path.

What changes are included in this PR?

Two new benchmarks in benches/dictionary_group_values.rs:

  • bench_scalar_append_equal — uses GroupOrdering::Full to exercise the scalar GroupValuesColumn:: code path
  • bench_take_n — uses EmitTo::First(size/2) in a loop over N_BATCHES iterations to exercise the partial/rolling emit path

Are these changes tested?

n/a

Are there any user-facing changes?

n/a

@github-actions github-actions Bot added the physical-plan Changes to the physical-plan crate label Aug 16, 2026
@Rich-T-kid

Copy link
Copy Markdown
Contributor Author

cc @saadtajwar @kumarUjjawal 🚀

@codecov-commenter

codecov-commenter commented Aug 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.25%. Comparing base (bbf739a) to head (e76e389).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24416      +/-   ##
==========================================
+ Coverage   81.23%   81.25%   +0.02%     
==========================================
  Files        1111     1112       +1     
  Lines      390208   391578    +1370     
  Branches   390208   391578    +1370     
==========================================
+ Hits       316990   318186    +1196     
- Misses      54591    54685      +94     
- Partials    18627    18707      +80     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@saadtajwar saadtajwar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice! Will definitely make sure to use this for #24089

@kumarUjjawal kumarUjjawal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @Rich-T-kid

I left two comments. Please let me know what you think.

Comment thread datafusion/physical-plan/benches/dictionary_group_values.rs Outdated
Comment thread datafusion/physical-plan/benches/dictionary_group_values.rs Outdated
@Rich-T-kid
Rich-T-kid force-pushed the rich-T-kid/cache-dictionary-arc-ptr branch from 200910d to e76e389 Compare August 18, 2026 04:27
@Rich-T-kid

Copy link
Copy Markdown
Contributor Author

Thank you @Rich-T-kid

I left two comments. Please let me know what you think.

@kumarUjjawal thank you for the review, e76e389 should address your comments 🚀

@kumarUjjawal kumarUjjawal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for iteration.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants