You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "mohitgurav20 (via GitHub)" <gi...@apache.org> on 2026/09/25 12:30:11 UTC
[PR] fix(physical-plan): replace approximate `map_size` accounting with exact `HashTable::allocation_size()` in `GroupValuesRows` [datafusion]
mohitgurav20 opened a new pull request, #25750:
URL: https://github.com/apache/datafusion/pull/25750
## Which issue does this PR close?
Closes #25735
Related to #25188
## Rationale for this change
### Background
`GroupValuesRows` is the general-purpose **fallback** `GroupValues`
implementation used for nested types (`Struct`, `List`, `Map`,
`RunEndEncoded`), multi-column schemas that cannot be handled by
`GroupValuesColumn`, and any schema containing unsupported column types.
Its `size()` method is the authoritative byte count fed into DataFusion's
query-execution **memory pool**, which uses it to decide when to apply spill
pressure. Under-reporting here directly weakens memory-spill signals for
exactly the schemas — nested and multi-column fallback — that are the most
expensive at runtime.
### Root Cause
Before this PR, `GroupValuesRows` tracked its hash-table footprint through an
incrementally maintained `map_size: usize` side-channel, updated in two places:
**During insertion** via `HashTableAllocExt::insert_accounted` in `proxy.rs`:
```rust
if self.len() == self.capacity() {
let bump_elements = self.capacity().max(16);
let bump_size = bump_elements * size_of::<T>(); // ← entry slots only
*accounting = (*accounting).checked_add(bump_size).expect("overflow");
self.reserve(bump_elements, &hasher);
}
self.insert_unique(hash, x, hasher);
--
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(physical-plan): replace approximate `map_size` accounting with exact `HashTable::allocation_size()` in `GroupValuesRows` [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25750:
URL: https://github.com/apache/datafusion/pull/25750#issuecomment-5866137898
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25750?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `94.53125%` with `7 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.50%. Comparing base ([`5719930`](https://app.codecov.io/gh/apache/datafusion/commit/5719930666fb590b318cd286cbb00cc473efe244?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`7218df1`](https://app.codecov.io/gh/apache/datafusion/commit/7218df1a6164502a0017b0004687e178e3955a57?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 25 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25750?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...n/physical-plan/src/aggregates/group\_values/row.rs](https://app.codecov.io/gh/apache/datafusion/pull/25750?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Faggregates%2Fgroup_values%2Frow.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9hZ2dyZWdhdGVzL2dyb3VwX3ZhbHVlcy9yb3cucnM=) | 94.53% | [0 Missing and 7 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25750?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25750 +/- ##
========================================
Coverage 82.49% 82.50%
========================================
Files 1140 1140
Lines 438499 438622 +123
Branches 438499 438622 +123
========================================
+ Hits 361754 361877 +123
+ Misses 54873 54863 -10
- Partials 21872 21882 +10
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25750?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(physical-plan): replace approximate `map_size` accounting with exact `HashTable::allocation_size()` in `GroupValuesRows` [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #25750:
URL: https://github.com/apache/datafusion/pull/25750#issuecomment-5865813440
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).
<details>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion-physical-plan v55.1.0 (current)
Built [ 41.250s] (current)
Parsing datafusion-physical-plan v55.1.0 (current)
Parsed [ 0.171s] (current)
Building datafusion-physical-plan v55.1.0 (baseline)
Built [ 39.946s] (baseline)
Parsing datafusion-physical-plan v55.1.0 (baseline)
Parsed [ 0.171s] (baseline)
Checking datafusion-physical-plan v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.665s] 223 checks: 221 pass, 2 fail, 0 warn, 31 skip
--- failure inherent_method_missing: pub method removed or renamed ---
Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/inherent_method_missing.ron
Failed in:
HashJoinExecBuilder::with_prepared_build, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/9fa9d7f769bc167db92f848fba75ca1f8d98ff53/datafusion/physical-plan/src/joins/hash_join/exec/prepared.rs:313
HashJoinExec::prepare_build, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/9fa9d7f769bc167db92f848fba75ca1f8d98ff53/datafusion/physical-plan/src/joins/hash_join/exec/prepared.rs:154
--- failure struct_missing: pub struct removed or renamed ---
Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/struct_missing.ron
Failed in:
struct datafusion_physical_plan::joins::PreparedHashJoinBuild, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/9fa9d7f769bc167db92f848fba75ca1f8d98ff53/datafusion/physical-plan/src/joins/hash_join/exec/prepared.rs:40
Summary semver requires new major version: 2 major and 0 minor checks failed
Finished [ 83.617s] datafusion-physical-plan
```
</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(physical-plan): replace approximate `map_size` accounting with exact `HashTable::allocation_size()` in `GroupValuesRows` [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew merged PR #25750:
URL: https://github.com/apache/datafusion/pull/25750
--
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(physical-plan): replace approximate `map_size` accounting with exact `HashTable::allocation_size()` in `GroupValuesRows` [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #25750:
URL: https://github.com/apache/datafusion/pull/25750#issuecomment-5865705043
@mohitgurav20
Thank you for working on this.
Can you also include these sections in the PR description?
- What changes are included in this PR?
- Are these changes tested?
- Are there any user-facing changes?
--
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(physical-plan): replace approximate `map_size` accounting with exact `HashTable::allocation_size()` in `GroupValuesRows` [datafusion]
Posted by "mohitgurav20 (via GitHub)" <gi...@apache.org>.
mohitgurav20 commented on PR #25750:
URL: https://github.com/apache/datafusion/pull/25750#issuecomment-5866394596
"Hi @kosiew, thank you for reviewing! I've updated the PR description with the answers to all of your questions. Let me know if everything looks good or if there's anything else you'd like me to change. Have a great day!"
--
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(physical-plan): replace approximate `map_size` accounting with exact `HashTable::allocation_size()` in `GroupValuesRows` [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #25750:
URL: https://github.com/apache/datafusion/pull/25750#issuecomment-5945702236
🚀
@mohitgurav20
Thank you for your contribution.
--
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