You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "kosiew (via GitHub)" <gi...@apache.org> on 2026/10/06 08:31:50 UTC
[PR] Fix GroupOrdering memory descriptor double-counting [datafusion]
kosiew opened a new pull request, #26074:
URL: https://github.com/apache/datafusion/pull/26074
## Which issue does this PR close?
- Part of #23393
## Rationale for this change
`GroupOrdering::size()` currently counts both the outer enum descriptor and the inline active variant descriptor. Because the active variant is stored inside `GroupOrdering`, this double-counts inline memory for partial and full ordering states.
This change makes the `GroupOrdering` enum the single owner of the descriptor charge. Partial ordering continues to include retained `order_indices` capacity and state-owned allocations, while full ordering adds no additional descriptor charge.
## What changes are included in this PR?
- Make `GroupOrdering::size()` charge `size_of::<GroupOrdering>()` exactly once for `None`, `Partial`, and `Full`.
- Rename `GroupOrderingPartial::size()` to `heap_size()` and make it report only:
- retained `order_indices` allocation
- allocations owned by the partial ordering state
- Remove `GroupOrderingFull::size()`, since full ordering has no additional heap allocation to add beyond the outer enum descriptor.
- Document the ownership boundary in `GroupOrdering::size()` and `GroupOrderingPartial::heap_size()`.
- Add deterministic size-accounting tests for none, full, and partial ordering states.
## Are these changes tested?
Yes. This PR adds:
- `test_size_none`, which verifies that `GroupOrdering::None` reports exactly `size_of::<GroupOrdering>()`.
- `test_size_full`, which verifies that full ordering reports exactly one `GroupOrdering` descriptor before and after `new_groups`, `remove_groups`, `input_done`, and `reset`.
- `test_size_partial_retained_allocations`, which verifies that:
- grown-and-truncated `order_indices` capacity remains charged
- a variable-width `ScalarValue::Utf8` sort key is included in the reported size
- replacing the sort key charges the replacement rather than retaining the old key
- `input_done` and `reset` drop state-owned key allocations while retaining the `order_indices` capacity
- repopulating the state causes the key allocation to be charged again
The tests use deterministic `size_of`, vector capacity, and `ScalarValue::size()` expectations rather than allocator-observed byte counts.
## Are there any user-facing changes?
No public API changes are introduced.
This changes internal memory accounting reported by `GroupOrdering::size()` so that inline descriptor memory is no longer double-counted. Grouping and aggregation behavior is otherwise unchanged by this patch.
## LLM-generated code disclosure
This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed.
--
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 GroupOrdering memory descriptor double-counting [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead merged PR #26074:
URL: https://github.com/apache/datafusion/pull/26074
--
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 GroupOrdering memory descriptor double-counting [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #26074:
URL: https://github.com/apache/datafusion/pull/26074#discussion_r4198007007
##########
datafusion/physical-plan/src/aggregates/order/partial.rs:
##########
@@ -267,9 +266,10 @@ impl GroupOrderingPartial {
Ok(())
}
- /// Return the size of memory allocated by this structure
- pub(crate) fn size(&self) -> usize {
- size_of::<Self>() + self.order_indices.allocated_size() + self.state.size()
+ /// Returns retained heap allocations, excluding the inline descriptor
+ /// already counted by [`super::GroupOrdering::size`].
+ pub(crate) fn heap_size(&self) -> usize {
+ self.order_indices.allocated_size() + self.state.size()
Review Comment:
Not introduced here, so a follow-up under #23393 is fine. `order_indices` is charged by capacity, but `State::size` charges `sort_key` by length. `get_row_at_idx` collects into `Result<Vec<_>>`, and a std-only stand-in with a 64-byte element ends at `len = 1`, `capacity = 4`. So a one-column key likely holds 3 spare `ScalarValue` slots (192 bytes) that `heap_size` does not report, and the `in_progress` expectation in the new test pins that length-based value. `Vec<ScalarValue>` already implements `DFHeapSize` (capacity based), which `CountGroupsAccumulator::size` uses the same way. `ScalarValue::size_of_vec` minus the `Vec` header, as in `nth_value.rs`, is the other existing option.
##########
datafusion/physical-plan/src/aggregates/order/mod.rs:
##########
@@ -164,7 +166,75 @@ mod tests {
use std::sync::Arc;
- use arrow::array::Int32Array;
+ use arrow::array::{Int32Array, StringArray};
+ use datafusion_common::ScalarValue;
+
+ #[test]
+ fn test_size_none() {
+ assert_eq!(GroupOrdering::None.size(), size_of::<GroupOrdering>());
+ }
+
+ #[test]
+ fn test_size_full() -> Result<()> {
+ let mut ordering = GroupOrdering::try_new(&InputOrderMode::Sorted)?;
+ let expected = size_of::<GroupOrdering>();
+ assert_eq!(ordering.size(), expected);
+
+ ordering.new_groups(&[], &[0, 1, 2], 3)?;
+ assert_eq!(ordering.size(), expected);
+ ordering.remove_groups(2);
+ assert_eq!(ordering.size(), expected);
+ ordering.input_done();
+ assert_eq!(ordering.size(), expected);
+ ordering.reset();
+ assert_eq!(ordering.size(), expected);
+ Ok(())
+ }
Review Comment:
One test is enough here: assert that `None` and a `Sorted` ordering both report exactly `size_of::<GroupOrdering>()`. `None | Full(_)` never reads the variant, so the `new_groups`, `remove_groups`, `input_done` and `reset` steps cannot change the result. The first `Full` assertion already catches the old code, and `test_size_none` passed before this PR too.
##########
datafusion/physical-plan/src/aggregates/order/mod.rs:
##########
@@ -164,7 +166,75 @@ mod tests {
use std::sync::Arc;
- use arrow::array::Int32Array;
+ use arrow::array::{Int32Array, StringArray};
+ use datafusion_common::ScalarValue;
+
+ #[test]
+ fn test_size_none() {
+ assert_eq!(GroupOrdering::None.size(), size_of::<GroupOrdering>());
+ }
+
+ #[test]
+ fn test_size_full() -> Result<()> {
+ let mut ordering = GroupOrdering::try_new(&InputOrderMode::Sorted)?;
+ let expected = size_of::<GroupOrdering>();
+ assert_eq!(ordering.size(), expected);
+
+ ordering.new_groups(&[], &[0, 1, 2], 3)?;
+ assert_eq!(ordering.size(), expected);
+ ordering.remove_groups(2);
+ assert_eq!(ordering.size(), expected);
+ ordering.input_done();
+ assert_eq!(ordering.size(), expected);
+ ordering.reset();
+ assert_eq!(ordering.size(), expected);
+ Ok(())
+ }
+
+ #[test]
+ fn test_size_partial_retained_allocations() -> Result<()> {
+ // Grow then truncate: unused order-index capacity must remain charged.
+ let mut order_indices = vec![0];
+ order_indices.extend(1..32);
+ order_indices.truncate(1);
+ assert!(order_indices.capacity() > order_indices.len());
+ let expected =
+ size_of::<GroupOrdering>() + order_indices.capacity() * size_of::<usize>();
+ let mut ordering =
+ GroupOrdering::Partial(GroupOrderingPartial::try_new(order_indices)?);
+ assert_eq!(ordering.size(), expected);
+
+ // A variable-width key checks both the scalar descriptor and its payload.
+ let key = "retained sort key";
+ let batch_group_values: Vec<ArrayRef> =
+ vec![Arc::new(StringArray::from(vec!["a", key]))];
+ ordering.new_groups(&batch_group_values, &[0, 1], 2)?;
+ let in_progress = expected + ScalarValue::Utf8(Some(key.to_owned())).size();
+ assert_eq!(ordering.size(), in_progress);
+ ordering.remove_groups(1);
+ assert_eq!(ordering.size(), in_progress);
+
+ // Replacing the key charges only the new payload, not the previous key.
+ let replacement_key = "updated sort key with a longer payload";
+ let replacement_group_values: Vec<ArrayRef> =
+ vec![Arc::new(StringArray::from(vec![replacement_key]))];
+ ordering.new_groups(&replacement_group_values, &[1], 2)?;
+ let replaced =
+ expected + ScalarValue::Utf8(Some(replacement_key.to_owned())).size();
+ assert!(replaced > in_progress);
+ assert_eq!(ordering.size(), replaced);
+
+ // Completing or resetting drops the key, but retains order-index capacity.
+ ordering.input_done();
+ assert_eq!(ordering.size(), expected);
+ ordering.reset();
+ assert_eq!(ordering.size(), expected);
+ ordering.new_groups(&batch_group_values, &[0, 1], 2)?;
+ assert_eq!(ordering.size(), in_progress);
+ ordering.reset();
+ assert_eq!(ordering.size(), expected);
Review Comment:
`heap_size` is a pure function of the current state, so these steps cannot fail independently of the earlier ones:
- `remove_groups` leaves `sort_key` untouched.
- The key replacement block hits the same `InProgress` arm as the first `new_groups` call. Replacement itself is already covered by `test_group_ordering_partial` in `partial.rs`.
- `reset` and the re-populate steps repeat the `Start` and `InProgress` assertions made above.
The spare-capacity assert, one `new_groups` call with the `Utf8` key and the `input_done` assert cover every accounting term this PR touches at about half the length.
--
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 GroupOrdering memory descriptor double-counting [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #26074:
URL: https://github.com/apache/datafusion/pull/26074#discussion_r4204353077
##########
datafusion/physical-plan/src/aggregates/order/partial.rs:
##########
@@ -267,9 +266,10 @@ impl GroupOrderingPartial {
Ok(())
}
- /// Return the size of memory allocated by this structure
- pub(crate) fn size(&self) -> usize {
- size_of::<Self>() + self.order_indices.allocated_size() + self.state.size()
+ /// Returns retained heap allocations, excluding the inline descriptor
+ /// already counted by [`super::GroupOrdering::size`].
+ pub(crate) fn heap_size(&self) -> usize {
+ self.order_indices.allocated_size() + self.state.size()
Review Comment:
#26099
--
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 GroupOrdering memory descriptor double-counting [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #26074:
URL: https://github.com/apache/datafusion/pull/26074#discussion_r4203938158
##########
datafusion/physical-plan/src/aggregates/order/mod.rs:
##########
@@ -164,7 +166,75 @@ mod tests {
use std::sync::Arc;
- use arrow::array::Int32Array;
+ use arrow::array::{Int32Array, StringArray};
+ use datafusion_common::ScalarValue;
+
+ #[test]
+ fn test_size_none() {
+ assert_eq!(GroupOrdering::None.size(), size_of::<GroupOrdering>());
+ }
+
+ #[test]
+ fn test_size_full() -> Result<()> {
+ let mut ordering = GroupOrdering::try_new(&InputOrderMode::Sorted)?;
+ let expected = size_of::<GroupOrdering>();
+ assert_eq!(ordering.size(), expected);
+
+ ordering.new_groups(&[], &[0, 1, 2], 3)?;
+ assert_eq!(ordering.size(), expected);
+ ordering.remove_groups(2);
+ assert_eq!(ordering.size(), expected);
+ ordering.input_done();
+ assert_eq!(ordering.size(), expected);
+ ordering.reset();
+ assert_eq!(ordering.size(), expected);
+ Ok(())
+ }
+
+ #[test]
+ fn test_size_partial_retained_allocations() -> Result<()> {
+ // Grow then truncate: unused order-index capacity must remain charged.
+ let mut order_indices = vec![0];
+ order_indices.extend(1..32);
+ order_indices.truncate(1);
+ assert!(order_indices.capacity() > order_indices.len());
+ let expected =
+ size_of::<GroupOrdering>() + order_indices.capacity() * size_of::<usize>();
+ let mut ordering =
+ GroupOrdering::Partial(GroupOrderingPartial::try_new(order_indices)?);
+ assert_eq!(ordering.size(), expected);
+
+ // A variable-width key checks both the scalar descriptor and its payload.
+ let key = "retained sort key";
+ let batch_group_values: Vec<ArrayRef> =
+ vec![Arc::new(StringArray::from(vec!["a", key]))];
+ ordering.new_groups(&batch_group_values, &[0, 1], 2)?;
+ let in_progress = expected + ScalarValue::Utf8(Some(key.to_owned())).size();
+ assert_eq!(ordering.size(), in_progress);
+ ordering.remove_groups(1);
+ assert_eq!(ordering.size(), in_progress);
+
+ // Replacing the key charges only the new payload, not the previous key.
+ let replacement_key = "updated sort key with a longer payload";
+ let replacement_group_values: Vec<ArrayRef> =
+ vec![Arc::new(StringArray::from(vec![replacement_key]))];
+ ordering.new_groups(&replacement_group_values, &[1], 2)?;
+ let replaced =
+ expected + ScalarValue::Utf8(Some(replacement_key.to_owned())).size();
+ assert!(replaced > in_progress);
+ assert_eq!(ordering.size(), replaced);
+
+ // Completing or resetting drops the key, but retains order-index capacity.
+ ordering.input_done();
+ assert_eq!(ordering.size(), expected);
+ ordering.reset();
+ assert_eq!(ordering.size(), expected);
+ ordering.new_groups(&batch_group_values, &[0, 1], 2)?;
+ assert_eq!(ordering.size(), in_progress);
+ ordering.reset();
+ assert_eq!(ordering.size(), expected);
Review Comment:
I’ll keep only the spare-capacity setup, initial assertion, one `Utf8` key update, and `input_done` assertion.
--
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 GroupOrdering memory descriptor double-counting [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #26074:
URL: https://github.com/apache/datafusion/pull/26074#issuecomment-6032496963
@comphead
> `State::size` naming, spare-capacity setup, and inline-only arm documentation.
`State::size` also excludes its inline descriptor; I’ll rename it `heap_size` for consistency. `GroupOrdering::try_new` clones the indices, so this setup tests retained-capacity accounting through the direct partial constructor, not that factory. I’ll keep that guard using `Vec::with_capacity(32)` plus `push(0)`, drop the redundant capacity assertion, and comment that `None | Full(_)` holds only inline state.
--
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 GroupOrdering memory descriptor double-counting [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #26074:
URL: https://github.com/apache/datafusion/pull/26074#issuecomment-6012853419
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/26074?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 `89.47368%` with `6 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.70%. Comparing base ([`d55153a`](https://app.codecov.io/gh/apache/datafusion/commit/d55153abd51279107f7f4be22955aa6571318ea0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`4090508`](https://app.codecov.io/gh/apache/datafusion/commit/4090508092fd4f13c2a261124eb2b60e8304f0d8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/26074?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/aggregates/order/mod.rs](https://app.codecov.io/gh/apache/datafusion/pull/26074?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Faggregates%2Forder%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9hZ2dyZWdhdGVzL29yZGVyL21vZC5ycw==) | 89.09% | [0 Missing and 6 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/26074?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 #26074 +/- ##
==========================================
- Coverage 82.70% 82.70% -0.01%
==========================================
Files 1147 1147
Lines 447631 447680 +49
Branches 447631 447680 +49
==========================================
+ Hits 370222 370262 +40
+ Misses 54921 54919 -2
- Partials 22488 22499 +11
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/26074?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 GroupOrdering memory descriptor double-counting [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #26074:
URL: https://github.com/apache/datafusion/pull/26074#discussion_r4203905306
##########
datafusion/physical-plan/src/aggregates/order/mod.rs:
##########
@@ -164,7 +166,75 @@ mod tests {
use std::sync::Arc;
- use arrow::array::Int32Array;
+ use arrow::array::{Int32Array, StringArray};
+ use datafusion_common::ScalarValue;
+
+ #[test]
+ fn test_size_none() {
+ assert_eq!(GroupOrdering::None.size(), size_of::<GroupOrdering>());
+ }
+
+ #[test]
+ fn test_size_full() -> Result<()> {
+ let mut ordering = GroupOrdering::try_new(&InputOrderMode::Sorted)?;
+ let expected = size_of::<GroupOrdering>();
+ assert_eq!(ordering.size(), expected);
+
+ ordering.new_groups(&[], &[0, 1, 2], 3)?;
+ assert_eq!(ordering.size(), expected);
+ ordering.remove_groups(2);
+ assert_eq!(ordering.size(), expected);
+ ordering.input_done();
+ assert_eq!(ordering.size(), expected);
+ ordering.reset();
+ assert_eq!(ordering.size(), expected);
+ Ok(())
+ }
Review Comment:
I’ll combine the `None` and `Sorted` assertions into one descriptor-size test; the `Sorted` assertion catches the original double charge.
--
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