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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
