You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "kosiew (via GitHub)" <gi...@apache.org> on 2026/08/11 09:46:57 UTC
[PR] Fix ArrowBytesViewMap retained capacity accounting [datafusion]
kosiew opened a new pull request, #24257:
URL: https://github.com/apache/datafusion/pull/24257
## Which issue does this PR close?
* Part of #23393
## Rationale 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 of `completed`, or unused capacity retained by completed Arrow buffers. Since `size()` is used for memory accounting, it should report the heap allocations owned by the map while continuing to exclude `self` and external input-array buffers.
## What changes are included in this PR?
* Updates `ArrowBytesViewMap::size()` to account for the allocated capacity of `views`.
* Accounts for the backing allocation of the `completed` vector and the retained capacity of each completed `Buffer`.
* Uses allocation-based accounting for the in-progress buffer.
* Clarifies that `size()` excludes both `self` and input-array buffers.
* Adds `DFHeapSize` support for `u128`, allowing `Vec<u128>` capacity to be accounted for through the existing heap-size infrastructure.
* Adds tests covering `u128` and `Vec<u128>` heap-size accounting and retained-capacity accounting in `ArrowBytesViewMap`.
## Are these changes tested?
Yes. The patch adds:
* `test_vec_u128`, which verifies that a `Vec<u128>` is accounted for using its allocated capacity.
* An additional `u128` assertion in the existing zero-heap-size test.
* `test_size_counts_retained_buffer_capacities`, which verifies that `ArrowBytesViewMap::size()` accounts for unused `views` capacity, the `completed` vector 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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix ArrowBytesViewMap retained capacity accounting [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #24257:
URL: https://github.com/apache/datafusion/pull/24257#discussion_r3775188078
##########
datafusion/physical-expr-common/src/binary_view_map.rs:
##########
@@ -469,11 +469,14 @@ where
}
/// Return the total size, in bytes, of memory used to store the data in
- /// this set, not including `self`
+ /// this set, not including `self` or input-array buffers.
pub fn size(&self) -> usize {
- let views_size = self.views.len() * size_of::<u128>();
- let in_progress_size = self.in_progress.capacity();
- 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());
Review Comment:
Amended to use `allocated_size`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix ArrowBytesViewMap retained capacity accounting [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #24257:
URL: https://github.com/apache/datafusion/pull/24257#issuecomment-5291303120
@neilconway, @comphead
Thanks for your review, feedback.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix ArrowBytesViewMap retained capacity accounting [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #24257:
URL: https://github.com/apache/datafusion/pull/24257#discussion_r3775188078
##########
datafusion/physical-expr-common/src/binary_view_map.rs:
##########
@@ -469,11 +469,14 @@ where
}
/// Return the total size, in bytes, of memory used to store the data in
- /// this set, not including `self`
+ /// this set, not including `self` or input-array buffers.
pub fn size(&self) -> usize {
- let views_size = self.views.len() * size_of::<u128>();
- let in_progress_size = self.in_progress.capacity();
- 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());
Review Comment:
Amended to use `allocated_size`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix ArrowBytesViewMap retained capacity accounting [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24257:
URL: https://github.com/apache/datafusion/pull/24257#issuecomment-5251943485
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24257?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 81.01%. Comparing base ([`a05388e`](https://app.codecov.io/gh/apache/datafusion/commit/a05388e82374ed2943595980b0539b6c1fa629a0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`5ce5c21`](https://app.codecov.io/gh/apache/datafusion/commit/5ce5c21a6a8a509f45e656dc87fe1a7bd9965e1c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 3 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ 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
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24257?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix ArrowBytesViewMap retained capacity accounting [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew merged PR #24257:
URL: https://github.com/apache/datafusion/pull/24257
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix ArrowBytesViewMap retained capacity accounting [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #24257:
URL: https://github.com/apache/datafusion/pull/24257#discussion_r3775188078
##########
datafusion/physical-expr-common/src/binary_view_map.rs:
##########
@@ -469,11 +469,14 @@ where
}
/// Return the total size, in bytes, of memory used to store the data in
- /// this set, not including `self`
+ /// this set, not including `self` or input-array buffers.
pub fn size(&self) -> usize {
- let views_size = self.views.len() * size_of::<u128>();
- let in_progress_size = self.in_progress.capacity();
- 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());
Review Comment:
Will amend to use `allocated_size`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix ArrowBytesViewMap retained capacity accounting [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #24257:
URL: https://github.com/apache/datafusion/pull/24257#discussion_r3766821677
##########
datafusion/physical-expr-common/src/binary_view_map.rs:
##########
@@ -469,11 +469,14 @@ where
}
/// Return the total size, in bytes, of memory used to store the data in
- /// this set, not including `self`
+ /// this set, not including `self` or input-array buffers.
pub fn size(&self) -> usize {
- let views_size = self.views.len() * size_of::<u128>();
- let in_progress_size = self.in_progress.capacity();
- 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());
Review Comment:
`allocated_size` should be equivalent here, I think (plus cheaper to compute).
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org