2010YOUY01 commented on code in PR #24017:
URL: https://github.com/apache/datafusion/pull/24017#discussion_r3913136500
##########
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:
Interesting... I feel the opposite way.
If we don't encode the states explicitly, then the states/transitions have
to be encoded with local variables and hidden assumptions, and to understand
that we have to load everything into short-term memory, which is hard.
The benefit of an explicit state representation is that it's a 1:1 mapping
from the mental model to the code:
1) It's possible to understand each state separately
2) It's safer: impossible states are unrepresentable, and it's easy to guard
with sanity checks
It's quite understandable for me now, because I'm already familiar with it;
for someone without prior knowledge trying to understand it, or if it later
evolves into a different shape, I think it will be more challenging to
understand.
--
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]
