Repository navigation
Generate GroupByHash output in multiple RecordBatches rather than one large one #9562
Description
Activity
take
This issue is interesting, let me try implement it
FYI this issue may be tricky -- as it will be performance critical -- I will be happy to assist
Reacted by guojidanthis issue is a bit tricky for me 😢 , I can only think of the following approaches:
change theGroupedHashAggregateStream::emit()function return a vectorResult<Vec<RecordBatch>>, eachRecordBatchnum_rows equalGroupedHashAggregateStream::batch_size, and changeExecutionState::ProducingOutput(RecordBatch)toExecutionState::ProducingOutput(Vec<RecordBatch>), thenGroupedHashAggregateStream::poll_next()function returns one element of a vector at one loop, likelet output = batchs.pop()sourcethis issue is a bit tricky for me 😢 , I can only think of the following approaches: change the
GroupedHashAggregateStream::emit()function return a vectorResult<Vec<RecordBatch>>, eachRecordBatchnum_rows equalGroupedHashAggregateStream::batch_size, and changeExecutionState::ProducingOutput(RecordBatch)toExecutionState::ProducingOutput(Vec<RecordBatch>), thenGroupedHashAggregateStream::poll_next()function returns one element of a vector at one loop, likelet output = batchs.pop()sourceI think this approach sounds good -- nice proposal
One thing that could help keep the PRs small and manageable would be to switch the APIs as described above but you could avoid having to change all the
GroupAccumulatorsin one PR by returning return aVec<>of size 1 for most of them.Then we can make subsequent PRs to switch over other groups accumulators as needed
Actually, I found the
slicefunction is not trivial because the eager computation for the null count.
One way I could think to solve it is making the computation lazy, see arrow-rs pr(q32 1.10~1.15x faster without the computation for null count):
apache/arrow-rs#6155The alternative may be the way mentioned in this issue.
I reached the same conclusion. Had a hash join after aggregation and it wanted to allocated > 16GB memory for couple hundred MB of aggregation state 😢
I think this approach sounds good -- nice proposal
One thing that could help keep the PRs small and manageable would be to switch the APIs as described above but you could avoid having to change all the GroupAccumulators in one PR by returning return a Vec<> of size 1 for most of them.
Then we can make subsequent PRs to switch over other groups accumulators as needed
@alamb why are you talking about accumulator APIs?
ExecutionState::ProducingOutputis purely local toGroupedHashAggregateStream. I already tried implementing this change and it worked great for my use case - the join no longer ran out of memory 😄@alamb why are you talking about accumulator APIs? ExecutionState::ProducingOutput is purely local to GroupedHashAggregateStream. I already tried implementing this change and it worked great for my use case - the join no longer ran out of memory 😄
I was thinking that the output of the aggregate exec is both the group columns and the aggregate columns. If we want to produce the output without one giant array, I think we would need to update both the group columns and aggregates
Note that @Rachelint is actively working on a PR
Reacted by Georgi KrastevWell the hacky solution is just
EmitTo::First(batch_size)until done 😄
I guess the concern is that it's not efficient because it's usingVec::split_offunder the hood in most cases?
Although that's not mandated by the API. But currently some plans simply don't work because of the memory overcounting.Well the hacky solution is just
EmitTo::First(batch_size)until done 😄 I guess the concern is that it's not efficient because it's usingVec::split_offunder the hood in most cases? Although that's not mandated by the API. But currently some plans simply don't work because of the memory overcounting.I think that will effectively copy all the values potentially total_size/batch_size times.
But it will result in smaller allocations of record batches for sure
Reacted by Georgi KrastevI filed a ticket / epic to track the idea of blocked state management here
Is your feature request related to a problem or challenge?
AggregateExecgenerates one single (giant)RecordBatchon output (source)RecordBatch::slice(), which does not actually allocate any additional memory) (source)This has at least two potential downsides:
RecordBatchs, those slices are treated as though they were an additional allocation that needs to be tracked (source)Something like this in pictures:
Describe the solution you'd like
If we had infinite time / engineering hours I think a better approach would actually be to change GroupByHash so it didn't create a single giant contiguous
RecordBatchInstead it would be better if GroupByHash produced a
Vec<RecordBatch>and then incrementally fed those batches outDoing this would allow the GroupByHash to release memory incrementally as it output. This is analogous to how @korowa made join output incremental in #8658
Perhaps something like
Describe alternatives you've considered
No response
Additional context
@yjshen notes: