apache / apache/datafusion

Substrait: the grouping set column holds `__grouping_id`, not the set's index

Open
#25,208 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.