rluvaton commented on PR #25707:
URL: https://github.com/apache/datafusion/pull/25707#issuecomment-5853846372
> @rluvaton
>
> I'd suggest reversing the order: first get an implementation that
measurably fixes the problem in #24704, then shape the API around it. An API
designed before we know what the fast, memory-efficient implementation needs
tends to get in its way, and it's much harder to change once it's public.
>
> The current draft shows the risk. Every aggregation now goes through the
blocked traits, and most of it runs through adapters over flat state, which is
slower than main and uses more memory:
Yes, but this is to make it easier to review, I can copy the entire
aggregation code and replace there with the blocked but it is harder to review
> * `group_values/blocked.rs:195` and `blocked_groups_accumulator.rs:312`:
emit All through the adapters is O(G²/B). With datafusion-cli, 1 partition,
`GROUP BY concat('k', v)` plus `sum(v)` at 4M groups takes 0.21 s on main and
30.2 s here. A primitive key with `sum` is 0.1 s vs 0.4 s.
I've did not implement in this PR the bytes/bytes view group by, but it is
implemented in later pr
> * `aggregates/spill.rs:220`: one spill file per block.
`aggregate_memory_spill.slt` sees `spill_count` go from 7 to about 800, and the
`count(DISTINCT)` cases at L95/L106 now fail with `ResourcesExhausted`.
I'm aware of that and I added a comment and fixed that problem in later PR
by adding `sort` without `concat` which fixes that problem
> Proposal:
>
> 1. Pick one target from #24704, e.g. peak memory / spill-free limit and
emit time for high-cardinality `GROUP BY <primitive>` with `sum/count/min/max`.
> 2. Implement blocked storage natively for just that case, behind a gate
(blocked only when the group values and all accumulators support it, flat path
unchanged otherwise), with no adapters.
Yes, but I've done this way to make it easier to review, I can copy the
entire aggregation code and replace there with the blocked but it is harder to
review since you have no clear way to see what I actually changed
> 3. Show before/after numbers: peak memory, spill count, and
ClickBench/TPC-H timings with no regressions elsewhere.
This pr is after I've done all of this (but without the flat approach), the
pr that implement it entirely is:
- #24928
> 4. Then extract the API from what that implementation actually needed, and
extend it to more accumulators and group-value types.
>
> That keeps each step reviewable and makes sure the API we commit to is one
that performs well.
--
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]
