You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "saadtajwar (via GitHub)" <gi...@apache.org> on 2026/08/03 22:55:37 UTC
[PR] perf: preallocate RowsGroupColumn buffers in take_n [datafusion]
saadtajwar opened a new pull request, #24070:
URL: https://github.com/apache/datafusion/pull/24070
## Which issue does this PR close?
- Closes #23994
## 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
--
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] perf: preallocate RowsGroupColumn buffers in take_n [datafusion]
Posted by "saadtajwar (via GitHub)" <gi...@apache.org>.
saadtajwar commented on PR #24070:
URL: https://github.com/apache/datafusion/pull/24070#issuecomment-5172608648
@kosiew - ready for review!
--
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] perf: preallocate RowsGroupColumn buffers in take_n [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #24070:
URL: https://github.com/apache/datafusion/pull/24070#issuecomment-5188638838
Thanks @saadtajwar
--
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] perf: preallocate RowsGroupColumn buffers in take_n [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24070:
URL: https://github.com/apache/datafusion/pull/24070#issuecomment-5172788585
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24070?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 80.88%. Comparing base ([`882d906`](https://app.codecov.io/gh/apache/datafusion/commit/882d90658a9334c3205c7994ac15b7c25163864c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`016e86c`](https://app.codecov.io/gh/apache/datafusion/commit/016e86caf2747b1ed8fa070af578d84cf4fa8f58?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ 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
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24070?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] perf: preallocate RowsGroupColumn buffers in take_n [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #24070:
URL: https://github.com/apache/datafusion/pull/24070#discussion_r3709520619
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/row_backed.rs:
##########
@@ -425,6 +429,84 @@ mod tests {
assert_eq!(g0, 20);
}
+ /// `take_n` preallocates the retained-row buffer from the known row
Review Comment:
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.
--
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] perf: preallocate RowsGroupColumn buffers in take_n [datafusion]
Posted by "saadtajwar (via GitHub)" <gi...@apache.org>.
saadtajwar commented on code in PR #24070:
URL: https://github.com/apache/datafusion/pull/24070#discussion_r3714005934
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/row_backed.rs:
##########
@@ -425,6 +429,84 @@ mod tests {
assert_eq!(g0, 20);
}
+ /// `take_n` preallocates the retained-row buffer from the known row
Review Comment:
Thanks - pushed a change to exercise this directly! Please let me know your thoughts!
--
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] perf: preallocate RowsGroupColumn buffers in take_n [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew merged PR #24070:
URL: https://github.com/apache/datafusion/pull/24070
--
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