gene-bordegaray commented on code in PR #24697:
URL: https://github.com/apache/datafusion/pull/24697#discussion_r3907744222
##########
datafusion/physical-plan/src/aggregates/order/mod.rs:
##########
@@ -28,6 +28,33 @@ use crate::InputOrderMode;
pub use full::GroupOrderingFull;
pub use partial::GroupOrderingPartial;
+/// Describes how an aggregate can determine that groups are complete.
+///
Review Comment:
I think this would greatly benefit from some type of example like the PR has
to show why this property is important and distinct from InputOrderMode and why
the property hold up
##########
datafusion/physical-plan/src/aggregates/order/mod.rs:
##########
@@ -28,6 +28,33 @@ use crate::InputOrderMode;
pub use full::GroupOrderingFull;
pub use partial::GroupOrderingPartial;
+/// Describes how an aggregate can determine that groups are complete.
+///
+/// This is distinct from [`InputOrderMode`], which describes the ordering of
+/// the input relative to the grouping expressions. Input ordering is one way
+/// to establish a group-completion mode, but the execution machinery only
+/// needs to know when it can safely emit completed groups.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub(crate) enum GroupCompletionMode {
+ /// Groups cannot be completed before the input ends.
+ None,
+ /// Groups sharing the values at these grouping-expression indices form a
+ /// contiguous range.
Review Comment:
This is also kinda of confusing. I think saying something like:
> Each key group in the aggregation is contiguous thus it is safe to emit
once the key changes.
or something like that would be good. And doing something similar for each
variant. This is a very new and in the weeds property so I want to make sure it
is upheld and well understood by new contributors to this code
##########
datafusion/physical-plan/src/aggregates/ordered_partial_stream.rs:
##########
@@ -29,10 +29,10 @@ use futures::stream::{Stream, StreamExt};
use super::AggregateExec;
use super::aggregate_hash_table::{OrderedAggregateTable, PartialMarker};
use crate::aggregates::AggregateMode;
-use crate::aggregates::order::GroupOrdering;
+use crate::aggregates::order::{GroupCompletionMode, GroupOrdering};
use crate::metrics::{BaselineMetrics, MetricBuilder, SpillMetrics};
use crate::stream::{EmptyRecordBatchStream, ObservedStream,
RecordBatchStreamAdapter};
-use crate::{InputOrderMode, SendableRecordBatchStream, metrics};
+use crate::{SendableRecordBatchStream, metrics};
/// Partial aggregate stream for `InputOrderMode::Sorted` and
Review Comment:
updae docs
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -877,6 +878,8 @@ pub struct AggregateExec {
required_input_ordering: Option<OrderingRequirements>,
/// Describes how the input is ordered relative to the group by columns
input_order_mode: InputOrderMode,
+ /// Describes how the executor can determine that groups are complete.
+ group_completion_mode: GroupCompletionMode,
cache: Arc<PlanProperties>,
Review Comment:
might be good to just briefly describe these two guys relationship in the
docs
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
Review Comment:
this is a suggestion as you see fit @xavlee. it also seems lek there ar
checks for ordering properties in other tests, maybe worth to add a group
completion assertion in some of these other ones
##########
datafusion/physical-plan/src/aggregates/ordered_final_stream.rs:
##########
@@ -35,16 +35,16 @@ use super::AggregateExec;
use super::aggregate_hash_table::{
FinalMarker, OrderedAggregateTable, OrderedAggregateTableMetrics,
};
+use super::order::GroupCompletionMode;
use crate::aggregates::AggregateMode;
use crate::metrics::{BaselineMetrics, RecordOutput, SpillMetrics};
use crate::sorts::IncrementalSortIterator;
use crate::sorts::streaming_merge::{SortedSpillFile, StreamingMergeBuilder};
use crate::spill::spill_manager::SpillManager;
use crate::stream::EmptyRecordBatchStream;
-use crate::{InputOrderMode, RecordBatchStream, SendableRecordBatchStream};
+use crate::{RecordBatchStream, SendableRecordBatchStream};
-/// Final aggregate stream for `InputOrderMode::Sorted` and
-/// `InputOrderMode::PartiallySorted`.
+/// Final aggregate stream for input whose completed group ranges can be
identified.
Review Comment:
I am not examplty clear what completed group ranges being identified means
in this context.
Just modes full and partial?
--
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]
