Substrait: the grouping set column holds `__grouping_id`, not the set's index
- Dominant language
- Rust
- Stars
- 9.3k
- Forks
- 2.4k
- Avg merge
- 3d 7h
- Merged PRs (30d)
- 344
Description
### Describe the bug
The output of an `AggregateRel` with more than one grouping set ends with "an extra `i32` column" whose value "will be the zero-based index of the grouping set that yielded the record" ([logical_relations.md at v0.102.0](https://github.com/substrait-io/substrait/blob/v0.102.0/site/docs/relations/logical_relations.md#aggregate-operation)). DataFusion's consumer maps that column to its own `__grouping_id`, so executing the plan puts the grouping-id bitmask in it rather than the set's index. For the two sets below that bitmask is a `UInt8`. #23468 moved the column to the position Substrait gives it and left its contents as they were.
The producer makes the same identification when it writes a plan: it maps DataFusion's `[groups, grouping_id, measures]` onto Substrait's `[groups, measures, grouping_id]` ([`producer/rel/aggregate_rel.rs:57-58`](https://github.com/apache/datafusion/blob/f8cc678272c50c781a56b74b49912b999aebd89e/datafusion/substrait/src/logical_plan/producer/rel/aggregate_rel.rs#L57-L58)). A plan passed between DataFusion and an implementation that follows the spec, such as substrait-java, which types the column `i32`, is therefore read with two meanings for it.
### To Reproduce
On `main` at `f8cc67827`, this plan has two grouping sets, `(a)` and `(b)`, over a two-row virtual table:
```json
{"relations": [{"root": {
"names": ["a", "b", "grouping_set"],
"input": {"aggregate": {
"input": {"read": {
"baseSchema": {"names": ["a", "b"], "struct": {"types": [
{"i64": {"nullability": "NULLABILITY_REQUIRED"}},
{"i64": {"nullability": "NULLABILITY_REQUIRED"}}], "nullability": "NULLABILITY_REQUIRED"}},
"virtualTable": {"expressions": [
{"fields": [{"literal": {"i64": "1"}}, {"literal": {"i64": "10"}}]},
{"fields": [{"literal": {"i64": "2"}}, {"literal": {"i64": "20"}}]}]}}},
"groupingExpressions": [
{"selection": {"directReference": {"structField": {"field": 0}}, "rootReference": {}}},
{"selection": {"directReference": {"structField": {"field": 1}}, "rootReference": {}}}],
"groupings": [{"expressionReferences": [0]}, {"expressionReferences": [1]}]}}}}]}
```
Consumed with `from_substrait_plan` and executed, it returns:
```text
grouping_set: UInt8 (nullable: false)
+---+----+--------------+
| a | b | grouping_set |
+---+----+--------------+
| 2 | | 1 |
| | 20 | 2 |
| 1 | | 1 |
| | 10 | 2 |
+---+----+--------------+
```
### Expected behavior
`grouping_set` as a required `Int32`, holding 0 on the two rows from `(a)` and 1 on the two rows from `(b)`. The bitmask and the index coincide for some lists of sets: for `(a, b)` followed by `(a)` both are 0 and then 1. A test needs a list on which they differ, such as `(a)` followed by `(b)`.
### Additional context
First seen in a [relation conformance case](https://github.com/alexandrefimov/substrait-conformance-cases/blob/a046105/tests/relations/cases/aggregate/grouping_set_index.yaml).
Contributor guide
Research direction
Start with datafusion/substrait/src/logical_plan/producer/rel/aggregate_rel.rs:57-58 and the from_substrait_plan path, then reproduce the supplied plan with grouping sets (a) followed by (b). Add a regression test covering the exchanged plan and verify the output column is required Int32 with values 0 for (a) and 1 for (b).
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- data-engineering
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100