fix: account for Arrow bytes map hash table allocations - #24229
Open
Boulea7 wants to merge 1 commit into
Open
Conversation
Initialize ArrowBytesMap and ArrowBytesViewMap accounting from the hash table allocation, and resynchronize it after table growth. Add regression coverage for construction, resize, and take. Fixes apache#21248 Generated-by: OpenAI Codex
Boulea7
force-pushed
the
fix/bytes-map-allocation-accounting
branch
from
August 10, 2026 16:07
34c109e to
30a98a8
Compare
alamb
reviewed
Aug 10, 2026
|
|
||
| // `insert_accounted` estimates growth from capacity. Keep `map_size` | ||
| // consistent with the exact allocation recorded by `new` after a resize. | ||
| if map.capacity() != capacity { |
Contributor
There was a problem hiding this comment.
I think I missed how does map_size get report in the overall size? I expected to see a change in size() as well 🤔
|
|
||
| #[test] | ||
| fn test_size_includes_hash_table_allocation() { | ||
| fn allocated_size(map: &ArrowBytesMap<i32, ()>) -> usize { |
Contributor
There was a problem hiding this comment.
This isn't testing any public API 🤔
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
2 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?
Rationale for this change
ArrowBytesMap::size()andArrowBytesViewMap::size()undercount memory because their hash tables are preallocated, whilemap_sizestarts at zero. This makes aggregate memory accounting inaccurate before the first resize and can also leave it out of sync after table growth.What changes are included in this PR?
map_sizefrom the hash table'sallocation_size()in both constructors.map_sizefromallocation_size()so exact allocation accounting is not mixed with incremental capacity estimates.take(), and the reset map returned bytake().Are these changes tested?
Yes.
cargo test -p datafusion-physical-expr-common— 77 unit tests and 8 doc tests passed.cargo fmt --all -- --check— passed.cargo clippy --all-targets --all-features -- -D warnings— passed../dev/rust_lint.sh— passed, including the workspace lint, formatting, documentation, license, typo, and workflow checks.I also ran the repository's extended workspace test command. It completed with 1,084 passed, 2 failed, and 1 ignored. Both failures were process RSS ceiling checks: the sort check used about 207 MB against a 190.7 MB limit, and the sort-merge join check initially used about 153.9 MB against a 152.6 MB limit. The sort-merge join check passed when rerun in isolation; an independent rerun reproduced only the sort RSS failure at about 206 MB. These checks exercise process-level integer sort and sort-merge join memory limits and do not execute the modified Arrow byte map paths, so the remaining failure appears environment-dependent.
Are there any user-facing changes?
There are no API changes. Memory estimates reported by these two byte maps now include the hash table allocation at construction and remain synchronized after growth and
take().AI-assisted contribution disclosure
This contribution was assisted by OpenAI Codex. I manually reviewed the final diff end-to-end, including the constructor, growth, and
take()paths, and I understand and can explain the implementation. There are no known unresolved assumptions or unreviewed generated sections. The commit message recordsGenerated-by: OpenAI Codex.