Rachelint commented on PR #25567: URL: https://github.com/apache/datafusion/pull/25567#issuecomment-5765694179
I experiment the similar idea in https://github.com/apache/datafusion/pull/23186 before, and see similar improvements, so I think it maybe really promising. However, as the PR description notes, there are still some unresolved issues preventing this idea to be ready. ## For string group values case > Where the remaining loss comes from (per final input row): Q14 costs 79-85 ns with one table and 91-99 ns bucketed... I previously tried moving the entire bucketing step into RepartitionExec, but the results were disappointing: it was noticeably slower than splitting into buckets inside the final aggregation. My current hypothesis is that this may be related to the repartition coalescer being shared across multiple producer tasks, whereas the extra hashing and copying in the final aggregation are local to a single stream. However, I have not confirmed this yet. That said, if we cannot avoid repartitioning the RecordBatch again inside the final aggregation, there seems to be little or no benefit for string group keys, and it can even cause a regression. This has been troubling me for a while. It is not limited to Q12–Q14 either; Q33 and Q34 also show significant regressions. ## Another possible opportunity to improve performance In my experiments, slicing a batch and pushing each slice into a per-bucket coalescer was not particularly fast. In [#23186](https://github.com/apache/datafusion/pull/23186), it even caused a performance regression(so actually I was surprised that this PR still achieves substantial overall improvements while using slice + coalescer). I later switched to MutableArrayData to build the bucket batches column by column, which improved this part of the process. A similar approach might be worth exploring as a follow-up optimization for this PR. The implementation was rather awkward, however, because MutableArrayData does not support continuously appending after an output array has been materialized. What I really wanted was a column-oriented coalescer—essentially direct access to the InProgressArray abstraction used internally by Arrow’s BatchCoalescer. I have wanted to fork Arrow and experiment with exposing that interface publicly, but unfortunately I have not had time to do so yet. -- 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]
