[GLUTEN-12597][CORE] Migrate AggregateRel.Grouping to expression references (Substrait 0.98) - #12724
Open
nielspardon wants to merge 1 commit into
Open
Conversation
|
Run Gluten Clickhouse CI on x86 |
…rences (Substrait 0.98) Substrait 0.98 moved the grouping expressions out of the per-grouping `Grouping.grouping_expressions` (field 1) into a rel-level pool `AggregateRel.grouping_expressions` (field 5) that each grouping set references by index through `Grouping.expression_references` (field 2). This migrates the vendored proto, the JVM producer, and both native consumers in lockstep. Gluten only ever emits a single grouping set with a flat list of grouping expressions (GROUPING SETS / CUBE / ROLLUP are expanded into an ExpandRel upstream, and both native consumers enforce at-most-one grouping), so the mapping is order-preserving: the flat list becomes the pool in declaration order and the single grouping references every entry as [0, 1, ..., n-1]. Empty grouping (global aggregation) stays one empty grouping with zero references. - Proto: reserve Grouping field 1, add Grouping.expression_references=2 and rel-level AggregateRel.grouping_expressions=5 (verbatim 0.98). - Producer: AggregateRelNode populates the pool and the single grouping's references. - Velox: SubstraitToVeloxPlan + SubstraitToVeloxPlanValidator resolve references against the pool; VeloxToSubstraitPlan (the reverse converter) emits pool + references. - ClickHouse: AggregateRelParser and ExpandRelParser read grouping keys through the pool via expression_references. - Migrate the static Substrait plan JSON fixtures to the pooled shape: the Velox q1_first_stage.json consumer fixture, the ClickHouse metrics-suite plans (covar_samp/covar_pop partial + final, tpch-q1/q2/q4), the orphaned native_write_plan_1_spark33.json, and the Velox benchmark plans (generic_q1, q17_joins). protobuf JSON parsing rejects unknown fields, so any fixture still on the inline (now-reserved) shape would fail to load. - Add AggregateRelProtoSuite pinning the producer contract. Part of apache#12597 Generated-by: Claude Code (Claude Opus 4.8)
nielspardon
force-pushed
the
feat/substrait-0.98-aggregate-grouping
branch
from
August 7, 2026 12:47
e9a2bb6 to
a5eab42
Compare
|
Run Gluten Clickhouse CI on x86 |
nielspardon
marked this pull request as ready for review
August 7, 2026 15:50
19 tasks
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
1 participant
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.
What changes are proposed in this pull request?
Substrait 0.98 moved the grouping expressions out of the per-grouping
Grouping.grouping_expressions(field 1) into a rel-level poolAggregateRel.grouping_expressions = 5that each grouping set references by index throughGrouping.expression_references = 2. This migrates Gluten's vendored proto, the JVM producer, and both native consumers in lockstep, as one step of the Substraitv0.23.0→0.98.0proto rebase (#12597).Gluten only ever emits a single grouping set with a flat list of grouping expressions — GROUPING SETS / CUBE / ROLLUP are expanded into an
ExpandRelupstream, and both native consumers enforce at-most-one grouping. So the mapping is order-preserving: the flat list becomes the rel-level pool in declaration order, and the single grouping references every entry as[0, 1, ..., n-1]. Empty grouping (global aggregation) stays one empty grouping with zero references — no behavior change.AggregateRelstays at Rel-oneof field 3, so there is no oneof renumbering.Groupingfield 1, addGrouping.expression_references = 2and rel-levelAggregateRel.grouping_expressions = 5(vendored verbatim from 0.98).AggregateRelNodepopulates the pool in declaration order and has the single grouping reference every entry by index.SubstraitToVeloxPlanandSubstraitToVeloxPlanValidatorresolve references against the pool;VeloxToSubstraitPlan(the Velox→Substrait reverse converter) emits pool + references.AggregateRelParserandExpandRelParserread grouping keys through the pool viaexpression_references.How was this patch tested?
AggregateRelProtoSuitepins the producer contract (flat grouping list → rel-level pool + single grouping referencing[0, 1]; empty grouping → one empty grouping with zero references).q1_first_stage.jsonconsumer fixture, the ClickHouse metrics-suite plans (covar_samp/covar_poppartial + final,tpch-q1/q2/q4), and the Velox benchmark plans (generic_q1,q17_joins).protocdup-field check;gluten-substraitbuilds and the new suite passes (mvn -Pspark-3.5); the Velox native backend compiles (consumer, validator, and reverse converter);clang-format(v15) clean on all changed C++ files.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8)
🤖 Generated with AI