Rachelint commented on code in PR #12640:
URL: https://github.com/apache/datafusion/pull/12640#discussion_r1777957474
##########
datafusion/physical-plan/src/aggregates/row_hash.rs:
##########
@@ -1004,9 +1004,13 @@ impl GroupedHashAggregateStream {
/// Updates skip aggregation probe state.
fn update_skip_aggregation_probe(&mut self, input_rows: usize) {
if let Some(probe) = self.skip_aggregation_probe.as_mut() {
- // Skip aggregation probe is not supported if stream has any
spills,
- // currently spilling is not supported for Partial aggregation
- assert!(self.spill_state.spills.is_empty());
+ // Skip aggregation probe is only supported in Partial aggregation.
+ // And it is not supported if stream has any spills even in
Partial aggregation.
+ // Although currently spilling is actually not supported in
Partial aggregation,
+ // it is possible to be supported in future, so we also add an
assertion for it.
+ assert!(
+ self.mode == AggregateMode::Partial &&
self.spill_state.spills.is_empty()
Review Comment:
I want to make that update_skip_aggregation_probe will be only called in
partial more clear by someway.
Maybe we can just let the update_skip_aggregation_probe will only be called
in partial?
if self.mode == AggregateMode::Partial {
self.update_skip_aggregation_probe(input_rows);
}
And we make self.skip_aggregation_probe.as_mut().unwarp in
update_skip_aggregation_probe?
--
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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
