kosiew commented on code in PR #24692:
URL: https://github.com/apache/datafusion/pull/24692#discussion_r3880303340
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -4011,21 +4011,32 @@ mod tests {
Arc::clone(&input_schema),
)?);
- let stream = partial_aggregate.execute_typed(0, &task_ctx)?;
+ let stream = partial_aggregate.execute_typed(0, &task_ctx);
// ensure that we really got the version we wanted
- match version {
+ let stream = match version {
0 => {
- assert!(matches!(stream, StreamType::AggregateStream(_)));
+ // the ungrouped stream charges its accumulators'
construction-time
Review Comment:
Could we add a focused regression test with an accumulator that allocates
retained state in its constructor, such as a `Vec`, and whose size does not
change during `update_batch`? The current test verifies that the existing
median accumulator's nonzero `size_of::<Self>()` cannot fit in a 1-byte pool,
but it doesn't quite reproduce the case this change is intended to fix. A
constructor-allocated buffer would directly exercise the Spark bloom-filter
scenario and verify that admission fails before the input is polled.
--
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]
