Background
Isthmus already supports GROUPING SETS / ROLLUP / CUBE and GROUP_ID() (see #527 / #565). The one remaining piece of #71 is the SQL GROUPING(expr) / GROUPING_ID(expr, …) aggregate functions, which have no direct Substrait equivalent (there is no grouping function in substrait/extensions/*.yaml).
Because of this, TpcdsQueryTest runs TPC-DS queries 27, 36, 70, 86 via rewritten alternate forms (27a/36a/70a/86a.sql, added in #513) that strip out GROUPING. The original queries do not convert.
Why not add a Substrait grouping function?
That was proposed upstream in substrait#824 but stalled without consensus and was auto-closed as stale. The maintainer-preferred alternative there does not require any spec change: GROUPING can be derived from the extra i32 grouping-set-index ("disambiguation") column that an Aggregate with more than one grouping set already emits, using a CASE expression. See tokoko's worked examples.
Proposed approach (forward: Calcite → Substrait)
Extend the existing project-wrapper in SubstraitRelVisitor.visit(Aggregate) (which already substitutes LITERAL_AGG literals and remaps GROUP_ID onto the disambiguation column). Add a third case: replace each Calcite GROUPING/GROUPING_ID call position with a CASE over the grouping-set-index column, where each arm is the bit/bitmask value computed from which grouping sets contain the referenced field(s).
Open sub-problems
- Reverse direction (Substrait → Calcite
GROUPING) — reconstructing a GROUPING call from the CASE/index pattern. This is what blocked the earlier attempt.
- Duplicate grouping sets edge case (flagged in the PR discussion).
Acceptance
GROUPING/GROUPING_ID convert Calcite → Substrait (forward), with round-trip tests.- Ideally reverse direction too; if not feasible initially, scope it as a follow-up.
- Remove 27/36/70/86 from
alternateForms in TpcdsQueryTest and delete the *a.sql files once the originals pass.
Remaining work split out from #71.
Background
Isthmus already supports
GROUPING SETS/ROLLUP/CUBEandGROUP_ID()(see #527 / #565). The one remaining piece of #71 is the SQLGROUPING(expr)/GROUPING_ID(expr, …)aggregate functions, which have no direct Substrait equivalent (there is nogroupingfunction insubstrait/extensions/*.yaml).Because of this,
TpcdsQueryTestruns TPC-DS queries 27, 36, 70, 86 via rewritten alternate forms (27a/36a/70a/86a.sql, added in #513) that strip outGROUPING. The original queries do not convert.Why not add a Substrait
groupingfunction?That was proposed upstream in substrait#824 but stalled without consensus and was auto-closed as stale. The maintainer-preferred alternative there does not require any spec change:
GROUPINGcan be derived from the extra i32 grouping-set-index ("disambiguation") column that an Aggregate with more than one grouping set already emits, using aCASEexpression. See tokoko's worked examples.Proposed approach (forward: Calcite → Substrait)
Extend the existing project-wrapper in
SubstraitRelVisitor.visit(Aggregate)(which already substitutesLITERAL_AGGliterals and remapsGROUP_IDonto the disambiguation column). Add a third case: replace each CalciteGROUPING/GROUPING_IDcall position with aCASEover the grouping-set-index column, where each arm is the bit/bitmask value computed from which grouping sets contain the referenced field(s).Open sub-problems
GROUPING) — reconstructing aGROUPINGcall from theCASE/index pattern. This is what blocked the earlier attempt.Acceptance
GROUPING/GROUPING_IDconvert Calcite → Substrait (forward), with round-trip tests.alternateFormsinTpcdsQueryTestand delete the*a.sqlfiles once the originals pass.Remaining work split out from #71.