Skip to content

perf: Concretely typed TopK StringHeap, and StringHashTable slot reuse - #24105

Draft
MassivePizza wants to merge 7 commits into
apache:mainfrom
massive-com:topk-mutable-borrow-slots
Draft

perf: Concretely typed TopK StringHeap, and StringHashTable slot reuse#24105
MassivePizza wants to merge 7 commits into
apache:mainfrom
massive-com:topk-mutable-borrow-slots

Conversation

@MassivePizza

@MassivePizza MassivePizza commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

N/A.

Continuation of #23609.

Rationale for this change

Improve perf of TopK.

What changes are included in this PR?

Eliminate overhead of casting with dyn Any for every String in topk::StringHeap.

Rearrange HashTableItem<ID> Option-ness to avoid unnecessary unwraps on lookups in topk::TopKHashTable.
Rewrite inserts to take a borrowable type, delaying clones and allowing reuse of storage slots (mostly Strings).

Are these changes tested?

Should be covered by existing tests.

Making ID an Option<ID> inside HashTableItem should be fine, since the internal HashTable keeps track of valid items (although this could be hard to verify properly). It runs with the assumption that "if everything needed an unwrap, nothing needs an unwrap".

Are there any user-facing changes?

No.

@github-actions github-actions Bot added the physical-plan Changes to the physical-plan crate label Aug 5, 2026
@MassivePizza MassivePizza changed the title perf: Concretely typed TopK StringHeap, and improved StringHashTable slot reuse perf: Concretely typed TopK StringHeap, and StringHashTable slot reuse Aug 5, 2026
@MassivePizza
MassivePizza force-pushed the topk-mutable-borrow-slots branch from 6ed1b35 to ddd9b71 Compare August 12, 2026 20:08
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.49123% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.14%. Comparing base (caa3cb4) to head (ddd9b71).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...on/physical-plan/src/aggregates/topk/hash_table.rs 95.87% 1 Missing and 3 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #24105   +/-   ##
=======================================
  Coverage   81.14%   81.14%           
=======================================
  Files        1110     1110           
  Lines      386107   386110    +3     
  Branches   386107   386110    +3     
=======================================
+ Hits       313313   313317    +4     
+ Misses      54329    54328    -1     
  Partials    18465    18465           

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

2 participants