Rachelint commented on code in PR #12640:
URL: https://github.com/apache/datafusion/pull/12640#discussion_r1779751387
##########
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`
It is really a good suggestion for readability.
I have tried to refactor the codes following the suggestion, I thikn it
indeed clearer for me.
##########
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`
It is really a good suggestion for readability.
I have tried to refactor the codes following the suggestion, I think it
indeed clearer for me.
--
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
