2010YOUY01 commented on code in PR #24017:
URL: https://github.com/apache/datafusion/pull/24017#discussion_r3912862085
##########
datafusion/physical-plan/src/aggregates/hash_stream.rs:
##########
@@ -249,47 +210,15 @@ pub(crate) struct FinalHashAggregateStream {
/// See comments for the same variable in [`PartialHashAggregateStream`].
group_values_soft_limit: Option<usize>,
- /// Tracks the high-level stream lifecycle. The hash table owns the
lower-level
+ /// The hash table owns the lower-level
/// state for emitting output batches.
- state: Option<FinalHashAggregateState>,
-}
-
-/// States for final hash aggregation processing.
-// The typestate pattern is used in case the inner logic becomes more complex
in
-// the future.
-enum FinalHashAggregateState {
Review Comment:
Is it possible to keep the explicit states, and only use the emitter pattern
to get rid of the verbose control flow implementation?
If we measure complexity as the cognitive load to understand everything,
instead of LoC, I think this explicit state representation approach is better.
As I currently understand it, the heuristic to follow is:
- If the state space is simple (2-3 states, trivial to understand, like the
cross join we did in https://github.com/apache/datafusion/pull/24291), such an
enum is unnecessary.
- If we have to draw a state machine graph to understand the inner logic,
like here, keep the `enum` states.
Additionally, I didn't try to polish the state splits originally, so there
might be some room to simplify them, like merging sibling states to make it
less verbose.
--
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]
