Minor: add partial assertion for skip aggregation probe #12640
Conversation
| // 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() |
There was a problem hiding this comment.
In partial mode, skip_aggregation_probe should be None, so mode check is not required
There was a problem hiding this comment.
Yes, these are two defensive assertions.
There was a problem hiding this 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?
There was a problem hiding this 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
There was a problem hiding this comment.
We could even use pattern matching on mode, trade duplicated function call with a clearer code path for partial and non-partial mode
match self.mode {
AggregateMode::Partial => {}
_ => {}
}There was a problem hiding this 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_aggregationandemit_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.
There was a problem hiding this comment.
Looks like an improvement to me -- thank you @Rachelint
Let's see what @jayzhan211 thinks too
| Some(Ok(batch)) => { | ||
| match (ready!(self.input.poll_next_unpin(cx)), self.mode) { | ||
| // New batch to aggregate in partial aggregation operator | ||
| (Some(Ok(batch)), AggregateMode::Partial) => { |
There was a problem hiding this comment.
BTW another way to express this rather than matching on (.., self.mode) would be to use a match clause
like
| (Some(Ok(batch)), AggregateMode::Partial) => { | |
| Some(Ok(batch)) if self.mode == AggregateMode::Partial => { |
That way you could avoid the other places which all match with (..., _)
There was a problem hiding this comment.
👍 Seems nice, fixed.
|
Thanks @Rachelint @alamb |
Which issue does this PR close?
Closes #.
Rationale for this change
I found an partial assertion check may be needed in skip aggregation probe, this pr added it.
#12332 (comment)
What changes are included in this PR?
Add an assertion to ensure and emphasize that skip aggregation probe will only be called in partial aggregation.
Are these changes tested?
Test by exist tests.
Are there any user-facing changes?
No.