2010YOUY01 commented on code in PR #24697:
URL: https://github.com/apache/datafusion/pull/24697#discussion_r4118328601
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -890,6 +891,12 @@ pub struct AggregateExec {
/// input order". When that is not possible, the constructor overwrites it
/// with the unordered variant [`InputOrderMode::Linear`].
input_order_mode: InputOrderMode,
+ /// Describes when the executor can determine that groups are complete.
+ ///
+ /// Input ordering describes a subset of the cases in which groups can be
+ /// safely emitted before the input ends. Full group completion requires
only
+ /// that rows for each complete grouping tuple are contiguous.
+ group_completion_mode: GroupCompletionMode,
Review Comment:
Is it possible to unify these two modes into a single struct? For example,
could we represent both the clustering property and the ordering property using
only `InputOrderingMode`?
Roughly, I feel clustering is the more general property, while sort order is
a stronger guarantee, so the implementation might model ordering as an optional
additional property.
This is not an issue for now, because the existing ordering optimization in
aggregation only relies on clustering; no optimization currently relies on the
stronger ordering guarantee.
However, if we want to extend this in the future to support both:
- optimizations that rely only on clustering, and
- additional optimizations that exploit sort order,
then representing these properties as a combination of flags could become
error-prone and difficult to extend.
There are already conversions between `GroupCompletionMode` and
`InputOrderMode` in this PR, and I find them quite hard to interpret.
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -1096,6 +1103,8 @@ impl AggregateExec {
input_order_mode = InputOrderMode::Linear;
}
+ let group_completion_mode =
GroupCompletionMode::from(&input_order_mode);
Review Comment:
marker for previous comment: this is a `order_mode` -> `completion_mode`
conversion
##########
datafusion/physical-plan/src/aggregates/order/mod.rs:
##########
@@ -40,15 +84,24 @@ pub enum GroupOrdering {
}
impl GroupOrdering {
- /// Create a `GroupOrdering` for the specified ordering
+ /// Create a `GroupOrdering` for the specified input order mode.
pub fn try_new(mode: &InputOrderMode) -> Result<Self> {
+ Self::try_new_for_group_completion(&GroupCompletionMode::from(mode))
+ }
+
+ /// Create a `GroupOrdering` for the specified group-completion mode.
+ pub(crate) fn try_new_for_group_completion(
Review Comment:
marker for previous comment: this is a `completion-mode` -> `ordering`
conversion.
--
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]
