Rachelint commented on code in PR #12640:
URL: https://github.com/apache/datafusion/pull/12640#discussion_r1777945500
##########
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:
Yes, these are two defensive assertions.
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`?
```rust
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: [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]
