Skip to content

perf: cache dictionary arc pointer - #24418

Open
Rich-T-kid wants to merge 3 commits into
apache:mainfrom
Rich-T-kid:rich-T-kid/cache-dict-arc-ptr-impl
Open

perf: cache dictionary arc pointer#24418
Rich-T-kid wants to merge 3 commits into
apache:mainfrom
Rich-T-kid:rich-T-kid/cache-dict-arc-ptr-impl

Conversation

@Rich-T-kid

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

append_val is called once per new group on the scalar (streaming) code path. Previously it hashed a single-element slice of the dictionary values array on every call, paying create_hashes fixed overhead 65k times for a high-cardinality batch. Caching the full values array hash keyed on Arc::ptr_eq collapses that to one vectorized hash pass per batch, yielding a 2× speedup on the all-unique case with no measurable regression elsewhere.

What changes are included in this PR?

  • DictionaryGroupValuesColumn gains a cached_values: Option field
  • append_val: on a cache miss (Arc::ptr_eq fails), hashes the entire dict.values() array into val_hashes and stores the Arc; on a hit, reuses val_hashes[val_idx] directly — eliminating the per-call slice(val_idx, 1) allocation and create_hashes call

Are these changes tested?

nothing logically changed.

Are there any user-facing changes?

no

@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 @zhuqi-lucas since this was a follow up you mentioned 😄

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 81.23%. Comparing base (bbf739a) to head (4a7a664).

Files with missing lines Patch % Lines
...gregates/group_values/multi_group_by/dictionary.rs 94.44% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24418      +/-   ##
==========================================
- Coverage   81.23%   81.23%   -0.01%     
==========================================
  Files        1111     1111              
  Lines      390208   390223      +15     
  Branches   390208   390223      +15     
==========================================
+ Hits       316990   316995       +5     
- Misses      54591    54597       +6     
- Partials    18627    18631       +4     

☔ 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.

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.

cache dictionary arc ptr in groupColumn

2 participants