andygrove commented on code in PR #7222:
URL: https://github.com/apache/datafusion-comet/pull/7222#discussion_r4392181593
##########
native/core/src/execution/planner/aggregate.rs:
##########
@@ -247,6 +247,37 @@ impl OperatorBuilder for HashAggBuilder {
})
.collect();
+ // DataFusion 55 runs Single over input sorted on some of its keys
with the legacy
+ // GroupedHashAggregateStream, whose spill can ask for more than its
share of the pool
+ // (#6880). Its Final runs an OrderedFinalAggregateStream there
instead, which spills, and
+ // MergeAsPartial evaluates to its state under Final too. Two things
make the task succeed
+ // there: the new stream's spill path, and the fair pool exempting
every consumer named
+ // OrderedFinalAggregateStream from its fair limit
(`limited_to_its_share`), the limit the
+ // legacy stream's spill failed against. So this PartialMerge is
checked only against the
+ // pool and Spark's grant.
+ // Final reads each state from the columns after the keys and spills
its groups in the
+ // input's types, so plan the PartialMerge as Final only when its
input is its keys
+ // followed by native states. A PartialMerge over Spark's binary
collect buffers keeps
+ // Single, but that input arrives through a ScanExec with no ordering,
so it is not
+ // expected to be sorted on a key.
+ // DataFusion 56 runs Single over this input with a
ClusteredSingleAggregateStream, and
+ // renames this stream ClusteredFinalAggregateStream, which is also
its consumer name
+ // (apache/datafusion#24697). So on that upgrade, drop this switch and
update the
+ // consumer name prefix in `spill_replay.rs` that the fair pool
exemption uses (#6410).
+ let mode = if proto_mode == ProtoAggregateMode::PartialMerge
+ && input_is_keys_then_states(agg, &aggr_expr, &schema)?
+ && !child
+ .native_plan
+ .equivalence_properties()
+ .find_longest_permutation(&group_by.input_exprs())?
+ .1
+ .is_empty()
+ {
+ DFAggregateMode::Final
+ } else {
+ mode
+ };
Review Comment:
No, that was not intended. Thanks for the detail on `num_spill`. The switch
now applies only when `find_longest_permutation` matches some but not all of
the grouping keys, so fully sorted input keeps `Single`. The `Sorted` planner
case now expects `Single`, and the comment, the `limited_to_its_share` doc and
`memory_management.md` say a fully sorted `PartialMerge` stays on the hash
aggregate.
--
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]
