xanderbailey commented on code in PR #18712:
URL: https://github.com/apache/datafusion/pull/18712#discussion_r2529937637
##########
datafusion/physical-plan/src/aggregates/row_hash.rs:
##########
@@ -709,9 +709,21 @@ impl Stream for GroupedHashAggregateStream {
break 'reading_input;
}
- self.emit_early_if_necessary()?;
+ // Check if we should switch to skip aggregation
mode
+ // It's important that we do this before we early
emit since we've
+ // already updated the probe.
+ if let Some(new_state) =
self.switch_to_skip_aggregation()? {
Review Comment:
It's unclear to me why we set `update_skip_aggregation_probe` here
https://github.com/apache/datafusion/pull/18712/files#diff-69c8ecaca5e2c7005f2ed1facaa41f80b45bfd006f2357e53ff3072f535c287dL687
and not next to `switch_to_skip_aggregation`. I can't fully give an
explanation yet but allowing the probe to be updated and then allowing the look
to break before we get here seems dangerous? It's important that we emit
everything before we move to the `SkipAggregation` state?
--
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]
