2010YOUY01 commented on PR #23407:
URL: https://github.com/apache/datafusion/pull/23407#issuecomment-4964509896
> @2010YOUY01 @Rachelint you might want to consider using the generator to
simplify the aggregate rewrite
Yes this makes sense, I'll give it a try later.
> I also agree with @rluvaton that the SortPreservingMerge code is about as
complicated as it can get and still be undersrtandable without some refactoring
-- so to make progress we will have to refactor it (basically what @Rachelint
@2010YOUY01 and I are doing to the aggregate code in #22710)
>
> One thing I think helps is separting the stream control flow (aka the
poll_next implementations) from the actual logic (aka messing around with data).
>
> A major win of this PR for simplicity I think is that it moves the logic
of updating the Loser tree implementation out of the control logic. Rather than
using a new crate / control flow pattern, we could instead try moving the loser
tree implementation into its own struct (encapsulate the logic and make the
boundaries of the different responsibilities clearer, and make the control flow
easier to read)
I have similar opinions to @alamb on how to reduce complexity in many
existing operators, I’ll try to explain them a little differently.
To make an operator implementation easy to read, its entry point should look
like pseudocode:
```text
# Ideal shape of aggregation
# aggregate_hash_table stores: group_keys -> group_states (with all internal
details encapsulated)
for batch in input_stream:
aggregate_hash_table.push(batch)
output_stream = aggregate_hash_table.finish()
for batch in output_stream:
yield batch
```
There are two major gaps between this ideal and the current implementation:
1. The yield-based pattern is difficult to express using the existing
streaming interface. (this PR is trying to improve)
2. Most implementations directly manipulate low-level primitives—such as
vectors, batches, and flags—in their main control flow.
Point 2 causes an issue: in order to read the code, at the same time, we
have to deal with the complexity for both control flow and logic details around
low-level primitives. This causes huge cognitive load.
A better approach would be to encapsulate these details in a deeper module,
and implement the core logic around that deep module. The goal is to make the
entry-point code look as much like pseudocode as possible—e.g. the
`aggregate_hash_table` abstraction above. This gives readers a clear
abstraction boundary: they can understand the overall control flow first, then
examine the implementation details only when needed.
I think the second issue is the larger source of complexity, but it's often
overlooked.
--
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]
