fix(isthmus)!: keep the declared column order of an aggregate over grouping sets - #1161
Open
alexandrefimov wants to merge 4 commits into
Open
fix(isthmus)!: keep the declared column order of an aggregate over grouping sets#1161alexandrefimov wants to merge 4 commits into
alexandrefimov wants to merge 4 commits into
Conversation
alexandrefimov
force-pushed
the
issue-1159-aggregate-column-order
branch
from
August 27, 2026 15:33
51d2276 to
a351938
Compare
…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
force-pushed
the
issue-1159-aggregate-column-order
branch
from
August 28, 2026 08:21
a351938 to
6bb070e
Compare
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.simpleTestGroupingSetschecks, 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 withAggregate.deriveRecordType. It now uses the distinct count. No query reaches that branch — Calcite foldsGROUP_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 threwArrayIndexOutOfBoundsException, onmainas well. Both counts are over the distinct grouping columns now. What the column holds is still Calcite's foldedGROUP_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_IDcall 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