2010YOUY01 commented on code in PR #24858:
URL: https://github.com/apache/datafusion/pull/24858#discussion_r3965498749
##########
datafusion/functions-aggregate-common/src/aggregate/groups_accumulator.rs:
##########
@@ -576,4 +611,136 @@ mod tests {
assert!(!accumulator.supports_state_preserving());
Ok(())
}
+
+ /// Accumulator whose `size()` is constant, so that the only thing that can
Review Comment:
I suggest removing this test, and potentially improving the coverage in the
e2e test instead (`datafusion/core/tests/memory_limit/mod.rs` in this PR).
Ideally, tests should be against end behavior rather than implementation
details. "Some aggregate queries should count the scratch space memory" is an
optimization detail, so testing it is already quite hard -- even doing it in an
e2e test is a bit of a compromise.
Here we're testing it via low-level APIs: the test is coupled to internals,
so I think it's really hard to maintain, and hard to re-discover what the test
goal was in the future.
##########
datafusion/functions-aggregate-common/src/aggregate/groups_accumulator.rs:
##########
@@ -102,6 +102,17 @@ pub struct GroupsAccumulatorAdapter {
/// bottleneck in earlier implementations when there were many
/// distinct groups.
allocation_bytes: usize,
+
+ /// The portion of [`Self::allocation_bytes`] that is the scratch
+ /// [`AccumulatorState::indices`] capacity held by [`Self::states`].
+ ///
+ /// The scratch vectors are cleared, but not deallocated, at the end of
every
+ /// batch, so their capacity is retained for the lifetime of the group. The
+ /// pre/post deltas taken around [`Accumulator`] work therefore see the
same
+ /// capacity on both sides and can never charge it. This field records what
+ /// has already been charged so each batch charges only the growth since
the
+ /// previous one.
Review Comment:
I think only the top comment is enough, the remaining part has already been
explained well in the implementation.
```suggestion
```
--
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]
