Boulea7 opened a new pull request, #24229:
URL: https://github.com/apache/datafusion/pull/24229

   ## Which issue does this PR close?
   
   - Closes #21248.
   
   ## Rationale for this change
   
   `ArrowBytesMap::size()` and `ArrowBytesViewMap::size()` undercount memory 
because their hash tables are preallocated, while `map_size` starts 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?
   
   - Initialize `map_size` from the hash table's `allocation_size()` in both 
constructors.
   - After an insertion changes the table capacity, resynchronize `map_size` 
from `allocation_size()` so exact allocation accounting is not mixed with 
incremental capacity estimates.
   - Use the same insertion path throughout both map implementations.
   - Add regression coverage for construction, capacity growth, populated 
`take()`, and the reset map returned by `take()`.
   
   ## 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.
   - Workspace `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 
records `Generated-by: OpenAI Codex`.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to