gabotechs commented on code in PR #24486:
URL: https://github.com/apache/datafusion/pull/24486#discussion_r3851218481
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -4488,10 +4460,10 @@ mod tests {
Ok(())
}
- /// Spilling behavior is not implemented for partial-reduce stream yet, so
fall
- /// back to the existing `GroupedHashAggregateStream`
+ /// Partial-reduce hash aggregation returns `ResourcesExhausted` when its
Review Comment:
👍 Agree with this. Partial reduction is not needed for correctness, it's
purely for performance, so if it accumulates too much memory worth of a hash
table, it might be better to flush it to the output, and even just fallback to
a bypass that does not aggregate anything.
Whether we should be re-aggregating or just bypassing the input to the
output directly, my guess is that whatever is simpler and introduces less code
is probably the best initial approach.
--
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]
