Skip to content

Conversation

gord02
Copy link

@gord02 gord02 commented Sep 24, 2025

Substrait is either produced using using the deprecated form where Groupings is set with a grouping_expressions list, or the new proto form where the grouping_expressions is defined on the AggregateRel itself with the Groupings having a list of references into the grouping_expressions list.

This PR would add support for both of these forms resolving bugs that expected the deprecated form of AggregateRel.

@CLAassistant
Copy link

CLAassistant commented Sep 24, 2025

CLA assistant check
All committers have signed the CLA.

@vbarua vbarua self-requested a review September 25, 2025 04:07
Copy link
Member

@vbarua vbarua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future readers, the context of this change arise from:
substrait-io/substrait#700
substrait-io/substrait#706

Thanks for starting on this. It would be good to have some tests for this. I would suggest adding a new test class like AggregateRelTest extends TestBase in this directory. Things to test would include consuming a proto plan with only the deprecated aggregate encoding and consuming a proto plan with the new aggregate encoding.

Beyond this, it might be good to consider updating the RelProtoConverter to output the new format as well For migration purposes, we should set both the new fields alongside the existing ones so that consuming systems can use whichever they support. This might require updating the Aggregate class itself to better align with the new representation.

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.

3 participants