alamb commented on PR #23128: URL: https://github.com/apache/datafusion/pull/23128#issuecomment-4918454985
> Motivation: production workloads that GROUP BY a mix of primitives plus a nested column (Struct<List> in the case I benchmarked) fall entirely into GroupValuesRows because of the one nested column. In my measurements the fallback ran at ~25 GiB peak vs ~15 GiB with the nested column removed — about 40% of peak was fallback tax, not the nested data itself. The EPIC brings nested types onto the column-wise path so mixed shapes stop falling back. This makes sense to me (as in as single nested GrOUP BY falls back). My concern is about trying to add per-type support for all remaining types -- it seems like a lot of new code, and as proposed still isn't general purpose (this PR only supports FixedSlizeList<Primitive> -- and not for example FixedSizedList<String> , etc If the goal is to avoid falling back to Rows for multi column grouping, what do you think about implementing a generic `GroupColumn` (based on e`Rows` internally) ? That way the Multi-group column code could always be used, and the Row conversion could limited to only columns that didn't have a `GroupColumn` implementation (rather than falling back to using Rows for all groups)? This would also avoid having to make specialized versons of `GroupColumn` for a bunch of rarely used types We maybe could also remove the `GroupValuesRows` entirely 🤔 -- 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]
