You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "sunchao (via GitHub)" <gi...@apache.org> on 2026/09/19 17:54:38 UTC
[PR] fix: account for hash-join row-index storage [datafusion]
sunchao opened a new pull request, #25508:
URL: https://github.com/apache/datafusion/pull/25508
## Which issue does this PR close?
Addresses the shared accounting bug identified while reviewing [#25491](https://github.com/apache/datafusion/pull/25491#discussion_r4052388920). Split out so it can be reviewed and backported independently of prepared-build reuse.
## Rationale for this change
Hash joins reserve memory for their hash buckets, but omit the row-index vector used to link build rows with the same key. As a result, a join can allocate beyond its admitted budget and underreport its build memory. For example, 100 million build rows need an additional 400 MB for this vector that the memory pool currently does not see.
## What changes are included in this PR?
The shared hash-map constructor now reserves the bucket estimate and row-index storage together before allocating either, and includes both in the build-memory metric. This covers ordinary CollectLeft and partitioned joins, using four bytes per row for the u32 representation and eight for u64. Perfect-hash array maps keep their existing accounting.
## What is the testing strategy for this PR?
A regression gives the constructor one byte less than the complete budget and checks that it refuses the allocation without charging the pool or metric. It also verifies success at the exact limit and release on drop. The test fails against the original constructor and passes with this fix.
All 533 hash-join tests, full workspace Clippy with all targets/features and warnings denied, formatting, and whitespace checks pass on this branch. The combined prepared-build branch also passed the extended workspace run with this constructor fix (11,995 Rust tests and 521 SQL test files; eight tests ignored).
Local checks used Rust 1.98.1 and temporary source overrides for the requested public releases of Arrow 60.0.0, object_store 0.14.2, sqlparser 0.63.0, and compression-codecs 0.4.42 because the registry mirror lacked them. No dependency or lockfile changes are included. Ordinary-path benchmark comparisons for the combined branch, including the accounting fix, are reported in #25491; they are not an isolated performance claim for this fix.
## Are there any user-facing changes?
Hash joins using these maps account for their row-index storage in memory limits and build-memory metrics. A budget that previously succeeded only because this allocation was omitted can now be rejected before allocation.
--
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: account for hash-join row-index storage [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25508:
URL: https://github.com/apache/datafusion/pull/25508#issuecomment-5744302782
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25508?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 `63.63636%` with `12 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.37%. Comparing base ([`a522cd5`](https://app.codecov.io/gh/apache/datafusion/commit/a522cd5bcedabd6238a0462b881a7ef4cb5c6f66?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`5315cbe`](https://app.codecov.io/gh/apache/datafusion/commit/5315cbee14b771adcfa6a922439e4d110fd938dc?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 6 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25508?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...tafusion/physical-plan/src/joins/hash\_join/exec.rs](https://app.codecov.io/gh/apache/datafusion/pull/25508?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fjoins%2Fhash_join%2Fexec.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9qb2lucy9oYXNoX2pvaW4vZXhlYy5ycw==) | 63.63% | [7 Missing and 5 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25508?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 #25508 +/- ##
==========================================
- Coverage 82.37% 82.37% -0.01%
==========================================
Files 1138 1138
Lines 433506 434112 +606
Branches 433506 434112 +606
==========================================
+ Hits 357102 357588 +486
- Misses 54850 54886 +36
- Partials 21554 21638 +84
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25508?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: account for hash-join row-index storage [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #25508:
URL: https://github.com/apache/datafusion/pull/25508
--
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