Fix OneSideHashJoiner memory size accounting - #24744
Merged
Merged
Conversation
…uble counting - Updated `OneSideHashJoiner::size()` to be self‑inclusive, eliminating inline double counts. - Refined counting logic to be based on `Vec` capacity. - Utilized `hashbrown::HashSet::allocation_size()` which includes control bytes. - Added tests covering empty and grown `visited_rows` scenarios. - No transformer changes.
- The test incrementally inserts elements into `hashes_buffer` via the hashmap to verify that the map expands correctly as it fills. - It asserts that the internal capacities increase as expected and checks the exact size delta after each insertion to ensure no gaps or overruns. - The test accounts for a preexisting zero‑capacity map estimate baseline, guaranteeing correct behavior even when the map starts with no allocated capacity.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24744 +/- ##
=======================================
Coverage 81.47% 81.47%
=======================================
Files 1122 1122
Lines 403582 403675 +93
Branches 403582 403675 +93
=======================================
+ Hits 328821 328905 +84
Misses 55512 55512
- Partials 19249 19258 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kosiew
marked this pull request as ready for review
August 28, 2026 07:14
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?
sizefunctions #23393Rationale for this change
OneSideHashJoiner::size()can overcount memory by adding inline fields that are already included in the joiner descriptor, while its previousHashSet<usize>estimate only accounted for element payload capacity and omitted hashbrown control/metadata allocation.This change defines the component-level accounting contract more explicitly so the joiner descriptor is counted once and directly owned container allocations are included using their actual allocation-aware helpers where available.
What changes are included in this PR?
OneSideHashJoiner::size()contract as including the joiner descriptor, directly owned container allocations, and input arrays, while excluding allocations referenced by sharedonexpressions.size_of_val(self).onvector's owned backing allocation using its capacity.PruningJoinHashMapaccounting so its descriptor is not counted twice.visited_rowspayload-only capacity calculation withallocation_size(), which accounts for the hashbrown allocation layout, including control bytes.visited_rowsstates and for the ownedon, hash map, and hashes-buffer allocations.Are these changes tested?
Yes. This PR adds the focused unit test:
one_side_hash_joiner_size_counts_descriptor_and_hash_set_allocation_onceThe test checks the explicit expected size formula for an empty joiner, verifies
visited_rowsallocation accounting across capacity growth, and verifies that owned container allocation changes are reflected without double-counting descriptors.Are there any user-facing changes?
No public API or query-result behavior changes are introduced.
This changes internal memory-size accounting for
OneSideHashJoiner, making its reported size more consistent with the documented ownership contract.LLM-generated code disclosure
This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed.