2010YOUY01 commented on code in PR #23182:
URL: https://github.com/apache/datafusion/pull/23182#discussion_r3479035743
##########
datafusion/physical-plan/src/aggregates/aggregate_hash_table/common.rs:
##########
@@ -297,11 +300,50 @@ pub(super) struct AggregateHashTableBuffer {
}
pub(super) enum AggregateHashTableState {
+ /// Accumulating input rows into group keys and aggregate state.
Building(AggregateHashTableBuffer),
+ /// Emitting results directly from group keys and aggregate state.
Review Comment:
```suggestion
/// Materialize all the output results, and then incrementally output in
the `OutputtingMaterializedFinal` state.
///
/// Note this is a temporary solution until the `GroupValues` issue is
solved:
/// Issue: <https://github.com/apache/datafusion/issues/23178>
```
##########
datafusion/physical-plan/src/aggregates/aggregate_hash_table/final_table.rs:
##########
@@ -120,3 +144,131 @@ impl AggregateHashTable<FinalMarker> {
Ok(())
}
}
+
+#[cfg(test)]
Review Comment:
I suggest removing this test. The property being tested is
implementation-specific and likely to change in future refactors.
What we really need to validate is the benchmark numbers. However, we don't
have automated CI for that, and we forgot to run the benchmark during the
previous refactor, which introduced the regression. We manually verified it
this time, so this should be sufficient.
--
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]
