jayzhan211 commented on issue #24704:
URL: https://github.com/apache/datafusion/issues/24704#issuecomment-6381189806
@rluvaton to answer your question, here is the proposed solution in one
place.
Neither PR changes how state is stored. Each one stops a hash table from
growing into the huge state this issue is about:
- **Partial stage (#26116, tracked in #26124):** the Partial aggregation
emits its table once it reaches about `batch_size` groups, then starts a new
one. Each emitted batch is a batch of its own, not a slice of a giant one, so
downstream operators charge it correctly, the table never grows past one batch,
and there is no long emit at the end of the input.
- **Final stage (#25724):** once the Final table outgrows the CPU caches,
the stream moves its state into 64 hash buckets and sends the rest of the input
there without aggregating it. At the end it aggregates and emits one bucket at
a time, so only one small table is live, and each bucket can be spilled and
freed on its own. For now it only applies when the group keys and the aggregate
state are fixed-width.
How that maps to the problems listed in the issue:
| Problem | Proposed fix |
|---|---|
| State is held until the table is fully drained (~2x peak) | Partial: flush
early (#26116). Final: buckets (#25724) |
| Long poll when output starts (#19906) | The same two: output is produced
one batch, or one bucket, at a time |
| Each output slice is charged for the whole batch (#22526) | Partial flush
emits batches that own their buffers. Separately, the consumer side can count
shared buffers once, as #25820 did for sort and #22862 for the hash join build
side. `RepartitionExec` still charges every slice in full |
| `Utf8` keys over 2 GiB overflow `i32` offsets (#23694) | Neither PR.
Either #23695 (`i64` offsets), or storing the keys as views: grouping by
`Utf8View` instead of `Utf8` completes the `concat("URL", "Title")` query that
overflows on main |
| Buffers copy when they double (#11931) | Growing from empty to 10M groups,
the doubling copies took about 5 ms against ~104 ms of update work (blocks took
8.5 ms), so I don't think they justify a redesign on their own |
With the options on, the benchmark bot measured ClickBench partitioned at
0.89–0.93x total time for #26116 across its runs and 0.967x for #25724. #26116
is still slower when a moderate number of keys repeat across flushes, which I
raised on the PR.
What neither PR covers: string and nested keys or state (both are excluded
for now), and a Partial table whose keys keep recurring, which would stop
flushing and grow as it does today. Is that close to your use case? If you can
share a query or the shape of the workload, we can check whether #26116 and
#25724 cover it, or whether we still need #25877 for it.
If this sounds right, I suggest we land #26116 first and then #25724,
reduced to the Final stage (it currently flushes the Partial table too), and
turn this issue into a checklist of the rows above.
@Rachelint if you don't have time for #26116 these days, I can continue it.
I left the issues I found on the PR.
--
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]
