Fix ArrowBytesViewMap retained capacity accounting - #24257
Open
kosiew wants to merge 4 commits into
Open
Conversation
…pdate docs, and add regression test for flush-path capacity and duplicate-insert
…nual element sizing - Completed counts of Vec descriptors and Buffer capacities. - Added test to enforce slack handling, flush partial retained buffers, and verify duplicate retention size. - Avoided empty‑size assertion by acknowledging HashTable preallocation.
…ion covering views slack, full completed Vec allocation, and buffer slack
… capacity unit test, and update ArrowBytesViewMap::views to use DFHeapSize
github-actions
Bot
added
physical-expr
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24257 +/- ##
========================================
Coverage 81.01% 81.01%
========================================
Files 1106 1106
Lines 383974 384106 +132
Branches 383974 384106 +132
========================================
+ Hits 311085 311197 +112
- Misses 54554 54567 +13
- Partials 18335 18342 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kosiew
marked this pull request as ready for review
August 12, 2026 07:37
neilconway
reviewed
Aug 12, 2026
neilconway
left a comment
neilconway
left a comment
Contributor
There was a problem hiding this comment.
While we're fixing this, I wonder if it is correct to initialize map_size to 0 when the initial map capacity is 512?
| let completed_size: usize = self.completed.iter().map(|b| b.len()).sum(); | ||
| // All fields below own their allocations. Count retained capacity rather | ||
| // than used length because this value drives memory accounting. | ||
| let views_size = self.views.heap_size(&mut DFHeapSizeCtx::default()); |
Contributor
There was a problem hiding this comment.
allocated_size should be equivalent here, I think (plus cheaper to compute).
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
ArrowBytesViewMap::size()currently undercounts retained heap memory by accounting for used lengths rather than allocated capacities in some of its owned storage.In particular, it does not account for unused capacity in
views, the backing allocation ofcompleted, or unused capacity retained by completed Arrow buffers. Sincesize()is used for memory accounting, it should report the heap allocations owned by the map while continuing to excludeselfand external input-array buffers.What changes are included in this PR?
ArrowBytesViewMap::size()to account for the allocated capacity ofviews.completedvector and the retained capacity of each completedBuffer.size()excludes bothselfand input-array buffers.DFHeapSizesupport foru128, allowingVec<u128>capacity to be accounted for through the existing heap-size infrastructure.u128andVec<u128>heap-size accounting and retained-capacity accounting inArrowBytesViewMap.Are these changes tested?
Yes. The patch adds:
test_vec_u128, which verifies that aVec<u128>is accounted for using its allocated capacity.u128assertion in the existing zero-heap-size test.test_size_counts_retained_buffer_capacities, which verifies thatArrowBytesViewMap::size()accounts for unusedviewscapacity, thecompletedvector allocation, and retained completed-buffer capacity. It also verifies that reinserting duplicate values does not report additional owned storage.Are there any user-facing changes?
No direct user-facing or API behavior changes. This corrects internal memory accounting for
ArrowBytesViewMap.LLM-generated code disclosure
This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed.