Skip to content

fix: Not all rows are accounted in RowCursorStream - #25372

Open
EmilyMatt wants to merge 1 commit into
apache:mainfrom
EmilyMatt:account-cursor-mem
Open

EmilyMatt wants to merge 1 commit into
apache:mainfrom
EmilyMatt:account-cursor-mem

Conversation

@EmilyMatt

Copy link
Copy Markdown
Contributor

Currently the behaviour is to only keep the current batch reserved per cursor, meaning we don't actually reserve all the rows we are keeping owned.
This leads to a lot of invisible memory, that is held, but not accounted by the pool.
(This scales fairly linearly with the number of partitions, in a regular round-robin case, for example.)
While the mem overhead was greatly reduced by @ariel-miculas 's #23619 and #23802
It is still a very problematic area in terms of memory visibility.
(Round-robin is an extreme case by nature)

This PR does not reduce the mem usage or anything, it just makes the memory visible and keeps everything that needs to be reserved - reserved.

I believe in order to ensure robustness, once this is merged another PR will need to be made to cap the merge reservation to the peak of the ExternalSorter reservation (due to some behaviours I've seen with fair-type pools, which treat spillable and non-spillable operators differently)

@github-actions github-actions Bot added the physical-plan Changes to the physical-plan crate label Sep 16, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.06897% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.94%. Comparing base (86ba5e0) to head (9b992dd).

Files with missing lines Patch % Lines
datafusion/physical-plan/src/sorts/stream.rs 81.81% 9 Missing and 17 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25372      +/-   ##
==========================================
- Coverage   81.94%   81.94%   -0.01%     
==========================================
  Files        1135     1135              
  Lines      428278   428403     +125     
  Branches   428278   428403     +125     
==========================================
+ Hits       350947   351049     +102     
- Misses      56375    56383       +8     
- Partials    20956    20971      +15     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ariel-miculas

Copy link
Copy Markdown
Contributor

Currently the behaviour is to only keep the current batch reserved per cursor, meaning we don't actually reserve all the rows we are keeping owned.
This leads to a lot of invisible memory, that is held, but not accounted by the pool.

I think this should be explained in more detail.
Specifically, from what I can tell, the existing issue is that we release the memory reservation when the cursor for a partition is dropped, ignoring the fact that ReusableRows keeps the memory alive.

Other than that, the cursors Vec should keep the memory reservations alive for each input partition, and when the stream is polled, the previous reservation would be dropped and a new reservation created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants