namanjain24-sudo commented on issue #25208: URL: https://github.com/apache/datafusion/issues/25208#issuecomment-6385205834
> How is this related to datafusion? Are you trying to run a datafusion plan in duckdb or ishmus? No — this is entirely inside DataFusion. Nothing here involves running a DataFusion plan in DuckDB or Isthmus. That opening line is my fault: I was noting that I could not find a reference producer to cross-check against, and leaving it above the bug report made it look like the problem. Sorry for the confusion. The spec ([logical_relations.md v0.102.0](https://github.com/substrait-io/substrait/blob/v0.102.0/site/docs/relations/logical_relations.md#aggregate-operation)): > To further disambiguate which record belongs to which grouping set, an aggregate relation with more than one grouping set receives an extra `i32` column on the right-hand side. The value of this field will be the zero-based index of the grouping set that yielded the record. DataFusion identifies that column with its own `__grouping_id`, which holds a bitmask, not an index. The producer does this explicitly — `grouping_set_output_mapping` is documented as mapping "Substrait's `[groups, measures, grouping_id]`" onto DataFusion's `[groups, grouping_id, measures]` — and the consumer reads it back the same way. Reproducer, DataFusion only. The plan in the issue has sets `(a)` then `(b)`; consumed with `from_substrait_plan` and executed on `main` at `039c0b99d5b`: ``` grouping_set : UInt8 +---+----+--------------+ | a | b | grouping_set | +---+----+--------------+ | 2 | | 1 | <- row from set (a), spec says 0 | 1 | | 1 | | | 20 | 2 | <- row from set (b), spec says 1 | | 10 | 2 | +---+----+--------------+ ``` So the spec asks for an `i32` holding `0` and `1`, and we return a `UInt8` holding `1` and `2`. Why this is easy to miss: the producer writes the same bitmask the consumer reads, so a DataFusion-to-DataFusion round trip agrees with itself and nothing looks wrong from inside DataFusion. It only diverges against an implementation that follows the spec. `(a)` then `(b)` is the smallest case where the two differ — for `(a, b)` then `(a)` the bitmask and the index are both `0, 1`, which is why the existing round-trip tests pass either way. -- 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]
