namanjain24-sudo commented on PR #25527: URL: https://github.com/apache/datafusion/pull/25527#issuecomment-6394126525
Fair point, and I can make it smaller. The bug in one line: after an aggregate with several grouping sets, Substrait's trailing `i32` column means "which grouping set produced this row" (0, 1, 2, ...), while DataFusion's `__grouping_id` means "which grouping columns this set leaves out" (a bitmask, plus an ordinal that separates repeated sets). The consumer and the producer currently treat the two as the same column. Nothing failed before because DataFusion writes and reads the same convention, so a DataFusion-to-DataFusion round trip agrees with itself. It only diverges against a consumer that follows the spec. For `GROUPING SETS ((a), (b))` a consumed plan yields 1 and 2 where the spec says 0 and 1. The fix in each direction is a `CASE` that maps one value to the other. Most of the diff is tests and fixtures: 745 of 1398 lines. Of the remaining 653, 395 is the shared `grouping_set` module that both directions use. If it helps review, I can split it: consumer first (map `__grouping_id` to the index, plus the shared id computation it needs), then the producer's inverse projection on top. @vbarua, could you take a look at the Substrait side when you have a chance? -- 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]
