jayzhan211 commented on code in PR #12640:
URL: https://github.com/apache/datafusion/pull/12640#discussion_r1778034315
##########
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:
It looks good to me, and also we could do the same for other partial mode
only like `switch_to_skip_aggregation`, `should_skip_aggregation` and
`emit_early_if_necessary`
--
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
