2010YOUY01 commented on code in PR #25158:
URL: https://github.com/apache/datafusion/pull/25158#discussion_r3985579128
##########
datafusion/physical-plan/src/aggregates/single_stream.rs:
##########
@@ -449,6 +454,27 @@ impl SingleHashAggregateStream {
return Self::break_with_err(e);
}
+ // Soft limit optimization:
Review Comment:
Can we move this comment to `SingleHashAggregateStream`, and here we can
comment 'see comments at xxx for details'
Additionally we can follow the comment pattern in (first explain how the SQL
get optimized to soft limit, and next the internal early termination mechanism)
-
https://github.com/apache/datafusion/blob/7e5f40a69aae9199c3c682806750e40d6bebb398/datafusion/physical-plan/src/aggregates/hash_stream.rs#L77
##########
datafusion/physical-plan/src/aggregates/single_stream.rs:
##########
@@ -449,6 +454,27 @@ impl SingleHashAggregateStream {
return Self::break_with_err(e);
}
+ // Soft limit optimization:
+ //
+ // Stop reading input once the in-memory table contains enough
distinct
+ // groups to satisfy the soft limit.
+ //
+ // When a limit is present, AggregateExec routes only
unordered,
+ // unfiltered DISTINCT aggregates to this stream.
+ //
+ // With no aggregate expressions, additional input can only
match existing
+ // groups or add new ones; it cannot change any existing
group's output.
+ // Since there is no ordering requirement and we already have
enough
+ // distinct groups, we can finish reading as if the input were
exhausted.
+ //
+ // Reuse the input-exhausted transition to merge any existing
spills
+ // before producing output. The downstream limit operator
enforces
+ // the exact output row count.
+ if self.hit_soft_group_limit(&hash_table) {
Review Comment:
I suggest to skip this optimization if we have spilled before
Here is the pattern to follow, and also the explanaiton
https://github.com/apache/datafusion/blob/7e5f40a69aae9199c3c682806750e40d6bebb398/datafusion/physical-plan/src/aggregates/hash_stream.rs#L857-L865
##########
datafusion/physical-plan/src/aggregates/single_stream.rs:
##########
@@ -449,6 +454,27 @@ impl SingleHashAggregateStream {
return Self::break_with_err(e);
}
+ // Soft limit optimization:
Review Comment:
and we can update control flow comment at `poll_next` to briefly mention
this optimization change
##########
datafusion/physical-plan/src/aggregates/single_stream.rs:
##########
@@ -104,6 +104,10 @@ pub(crate) struct SingleHashAggregateStream {
/// Tracks the high-level stream lifecycle. The hash table owns the
lower-level
/// state for emitting output batches.
state: Option<SingleHashAggregateState>,
+
+ /// When set, there are no aggregate expressions: AggregateExec routes
+ /// limited non-DISTINCT aggregates to a different stream.
Review Comment:
Maybe we can also change it to 'see top comments for details'.
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -4424,6 +4424,199 @@ mod tests {
Ok(())
}
+ #[tokio::test]
Review Comment:
I recommend to write this test differently (follow the pattern in
https://github.com/apache/datafusion/commit/40a64546c8ed25a00c8100d2fb4f44890e22fc25#diff-02af0439a3df656429990b220b80e50d8df259ce45c47e008460c1ca3781aca3)
The main difference is
- Try to exercise this feature end-to-end, from `select distinct` query, and
get it optimized to aggregate with soft limit
- Also assert the internal metric of `AggregateExec`, otherwise we can't
ensure if this soft limit optimization is applied -- limit can also be enforced
by the downstream `LimitExec` operator.
(I think only such e2e test is enough, we don't have to test it individually
on `AggregateExec`, since this optimization is only useful from such SQL
patterns, and should not be directly used on the `AggregateExec`)
--
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]
