2010YOUY01 commented on PR #15591: URL: https://github.com/apache/datafusion/pull/15591#issuecomment-5384052816
> So I took this pr and added support locally for binary and it fixed the issue we had with huge batches coming out of aggregate, however I have concerns because ideally It should be enabled in all cases in order for it to be benefitial - for us it's partial/final with both spill/emit early support but there. > > 1. Grouping impl will be complicated: Adding support for more columns types and multi group by is complicated > 2. In the very hot loop of doing aggregation and jumping in memory you are adding another memory jump > 3. Complicated aggregate expression code - even though the idea is simple, the actual implementation that needed to be done to support that is really complicated To quantify the implementation effort for options 1 and 3: we would need to migrate all implementations of `GroupValues` and `GroupsAccumulator` to the new blocked physical layout. There are 20+ implementations in total, it's definitely significant but seems doable. > Another complexity is supporting `ListGroupValueBuilder`, try to add support for blocking with it(ignore non blocking support and just see how complex even blocked support is) Maybe it's a good idea to experimenting the hardest one first 🤔 -- 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]
