Rich-T-kid commented on PR #23128: URL: https://github.com/apache/datafusion/pull/23128#issuecomment-4927260051
> > Motivation: production workloads that GROUP BY a mix of primitives plus a nested column (Struct 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 columns forces a fall back for all columns) > > 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 -- and not for example FixedSizedList , 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 🤔 @alamb in this context do nested types encompass dictionary arrays? -- 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]
