Skip to content

perf: preallocate RowsGroupColumn buffers in take_n - #24070

Open
saadtajwar wants to merge 2 commits into
apache:mainfrom
saadtajwar:saadtajwar/preallocate-rowsgroupcolumn-buffers
Open

perf: preallocate RowsGroupColumn buffers in take_n#24070
saadtajwar wants to merge 2 commits into
apache:mainfrom
saadtajwar:saadtajwar/preallocate-rowsgroupcolumn-buffers

Conversation

@saadtajwar

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Currently RowsGroupColumn's take_n method knows the remaining number of rows and bytes after taking the first n group rows, but when invoking empty_rows passes in 0 for the row capacity and data capacity parameters instead of the known remainders, potentially causing unnecessary copying and allocating

What changes are included in this PR?

Using the known remaining number of rows and bytes in the invocation to empty_rows

Are these changes tested?

Yes

Are there any user-facing changes?

No

@saadtajwar

Copy link
Copy Markdown
Contributor Author

@kosiew - ready for review!

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

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.88%. Comparing base (882d906) to head (016e86c).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24070      +/-   ##
==========================================
- Coverage   80.88%   80.88%   -0.01%     
==========================================
  Files        1102     1102              
  Lines      375813   375872      +59     
  Branches   375813   375872      +59     
==========================================
+ Hits       303971   304015      +44     
- Misses      53741    53748       +7     
- Partials    18101    18109       +8     

☔ 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.

@kosiew kosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saadtajwar
Looks good overall. I left one non-blocking suggestion to make the preallocation behavior more directly exercised by the test.

assert_eq!(g0, 20);
}

/// `take_n` preallocates the retained-row buffer from the known row

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test does a good job of confirming that rebuilding works correctly, but I do not think it distinguishes the new preallocation logic from the previous empty_rows(0, 0) path. The outer-null FixedSizeList has a shorter row encoding, but that row is emitted rather than retained. The retained inner-null and non-null Int32 rows appear to have the same encoded length.

Could we change or add retained rows with different encoded sizes, perhaps using FixedSizeList<Utf8> values with unequal payload lengths? That would exercise the lengths().skip(n).sum() preallocation calculation more directly.

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.

Preallocate RowsGroupColumn buffers during take_n rebuilds

3 participants