Skip to content

fix(isthmus)!: keep the declared column order of an aggregate over grouping sets - #1161

Open
alexandrefimov wants to merge 4 commits into
substrait-io:mainfrom
alexandrefimov:issue-1159-aggregate-column-order
Open

fix(isthmus)!: keep the declared column order of an aggregate over grouping sets#1161
alexandrefimov wants to merge 4 commits into
substrait-io:mainfrom
alexandrefimov:issue-1159-aggregate-column-order

Conversation

@alexandrefimov

@alexandrefimov alexandrefimov commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Substrait takes the grouping columns of an aggregate to be the distinct grouping expressions in the order they first appear across its grouping sets, while Calcite takes them from a bit set and emits them ordered by field index. Neither direction accounted for that, so a plan whose sets first mention field 1 and then field 0 changed meaning on the way through: a reference to the aggregate's first column reached the column Calcite had put there instead, and its type changed with it.

Both directions now carry the difference in the emit mapping rather than in the shape of the plan. On the way in, the mapping a relation carries is translated into the order the converted aggregate emits, and a relation that emits directly gets the mapping that puts its columns back in the declared order. On the way out, the mapping presents the aggregate's output in Calcite's order, so a parent converted from the same Calcite plan finds its columns where it left them. Neither direction adds a relation the plan did not have, so a plan that already agrees with Calcite round-trips unchanged — which is what Substrait2SqlTest.simpleTestGroupingSets checks, and it is also how I found that fixing only one direction is not enough.

The pre-aggregate projection now reuses one column for a field grouped on by several sets. Two copies of it would each be missing from a grouping set, and Calcite would make both nullable.

Worth a second look, since the change touches a branch that had no coverage: the GROUP_ID branch of SubstraitRelVisitor.visit(Aggregate) used a grouping-field count that included a field shared by several sets more than once, which disagrees with Aggregate.deriveRecordType. It now uses the distinct count. No query reaches that branch — Calcite folds GROUP_ID() into a literal wherever it can work out the answer, including over duplicated grouping sets — so the test that covers it builds the Calcite aggregate directly.

Two more defects in the grouping-set index column came out of the same math and are fixed here. The mapping that keeps that column replaced its index with aggregateCalls.size() - 1, which counts aggregate calls rather than output columns, so what came back was a copy of a grouping column. And the index the relation declares for it counted every mention of a grouping expression, so a field grouped on by several sets shifted it past the end: such an aggregate with a mapping that keeps the index threw ArrayIndexOutOfBoundsException, on main as well. Both counts are over the distinct grouping columns now. What the column holds is still Calcite's folded GROUP_ID() literal, which is #1182.

One difference is left alone: the grouping-set index comes back as an i64. The conversion builds Calcite's GROUP_ID call as a BIGINT while the spec gives the aggregate an i32 column, and Calcite folds the call to a literal of its own type. That is #1162.

Closes #1159

alexandrefimov and others added 3 commits August 28, 2026 11:20
…ouping sets

Substrait takes the grouping columns of an aggregate to be the distinct
grouping expressions in the order they first appear across its grouping
sets, while Calcite takes them from a bit set and emits them ordered by
field index. Neither direction accounted for that, so a plan whose sets
first mention field 1 and then field 0 changed meaning on the way
through: a reference to the aggregate's first column reached the column
Calcite had put there instead, with its type quietly changing along with
it.

Both directions now carry the difference in the emit mapping. On the way
in, the mapping a relation carries is translated into the order the
converted aggregate emits, and a relation that emits directly gets the
mapping that puts its columns back in the declared order. On the way out,
the mapping presents the aggregate's output in Calcite's order, so a
parent converted from the same Calcite plan finds its columns where it
left them. Neither adds a relation the plan did not have, so a plan that
already agrees with Calcite round-trips unchanged.

The pre-aggregate projection now reuses one column for a field grouped on
by several sets. Two copies of it would each be missing from a grouping
set, and Calcite would make both nullable.

Closes substrait-io#1159

BREAKING CHANGE: an aggregate over several grouping sets is now emitted
with an emit mapping that presents its output in the order the plan it
came from had, and a plan carrying such a mapping is read that way.
Consumers that assumed the grouping columns were ordered by field index
will see them in the order the grouping sets declare.
The translation from declared to emitted grouping order gave up when a
grouping expression was not a field reference into the aggregate's input,
on the grounds that anything else is projected below the aggregate in
declared order. That holds for what transformToValidCalciteAggregate
rewrites, but not for an outer reference: it passes the validator, is left
alone, and Calcite projects it itself, after the input's own fields. A plan
that groups on one before a field of its input then read the wrong column.

Order the columns by where they sit in the aggregate's input instead -- a
field reference where its field is, anything else after them all, in
declared order, which a stable sort keeps.
…olumn

The mapping that keeps the index replaced it with aggregateCalls.size() - 1, an
index into the aggregate calls rather than into the aggregate's output, so the
column that came back was a copy of a grouping column.

The index the relation declares for that column counted every mention of a
grouping expression, so a field grouped on by several sets shifted it past the
end: an aggregate with such a field and a mapping that keeps the index threw
ArrayIndexOutOfBoundsException. Both counts are now over the distinct grouping
columns, which is what the record type holds.
@alexandrefimov
alexandrefimov force-pushed the issue-1159-aggregate-column-order branch from a351938 to 6bb070e Compare August 28, 2026 08:21
Three of the four grouping columns in this fixture are BIGINT, so comparing
types cannot show a permutation among them. The mapping is what carries the
declared order, so it is asserted directly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

isthmus: an aggregate over several grouping sets converts to Calcite in a different column order, so references over it read the wrong column

1 participant