Skip to content

fix(core): use new aggregate grouping behavior in RelProtoConverter#744

Merged
nielspardon merged 5 commits into
substrait-io:mainfrom
nielspardon:par-agg-group
Mar 16, 2026
Merged

fix(core): use new aggregate grouping behavior in RelProtoConverter#744
nielspardon merged 5 commits into
substrait-io:mainfrom
nielspardon:par-agg-group

Conversation

@nielspardon

@nielspardon nielspardon commented Mar 10, 2026

Copy link
Copy Markdown
Member

changes RelProtoConverter to output the new and the old aggregate grouping proto structures

relates to #299 and substrait-io/substrait#1002

This PR changes the RelProtoConverter to output the new and the old grouping behavior keeping the existing io.substrait.relation.Aggregate behavior the same.

@benbellick

Copy link
Copy Markdown
Member

Thanks for doing this! Will review when I have a free moment, but I think this would qualify as a breaking change, so it will need the ! in the PR title to follow conventional commit (plus the BREAKING CHANGE part of the message body).

@nielspardon nielspardon changed the title fix(core): use new aggregate grouping behavior in RelProtoConverter fix(core)!: use new aggregate grouping behavior in RelProtoConverter Mar 10, 2026
@benbellick

benbellick commented Mar 10, 2026

Copy link
Copy Markdown
Member

I think we will want to have a discussion about this one at the community sync. The way I see it there are two ways forwards:

  • faster: We do what this PR suggests and just switch to the new representation (what this PR is doing), or
  • safer: We have a release where we emit both the old and new representation, and only drop the old one in a subsequent PR.

I'm not sure what the right approach is, but probably worth talking about before merging.

EDIT: Thinking about it more, as long as Datafusion can handle the newer version already, then I am fine with just doing the "faster" change, considering all of the other libraries can already handle it.

@nielspardon

Copy link
Copy Markdown
Member Author

I added code that outputs the old data structures in parallel as discussed in the community sync until we fully remove the old field via substrait-io/substrait#1002

@nielspardon nielspardon changed the title fix(core)!: use new aggregate grouping behavior in RelProtoConverter fix(core): use new aggregate grouping behavior in RelProtoConverter Mar 11, 2026
Comment thread core/src/test/java/io/substrait/relation/AggregateRelTest.java Outdated

@benbellick benbellick left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it looks good, just the one comment about making the tests a bit clearer. Thanks!

Comment thread core/src/test/java/io/substrait/relation/AggregateRelTest.java Outdated
BREAKING CHANGE: changes RelProtoConverter to always output the new
aggregate grouping proto structures

Signed-off-by: Niels Pardon <par@zurich.ibm.com>
Signed-off-by: Niels Pardon <par@zurich.ibm.com>
Signed-off-by: Niels Pardon <par@zurich.ibm.com>
Signed-off-by: Niels Pardon <par@zurich.ibm.com>

@benbellick benbellick left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, just a couple small comments. Thanks!

Comment thread core/src/test/java/io/substrait/relation/AggregateRelTest.java
Comment thread core/src/test/java/io/substrait/relation/AggregateRelTest.java Outdated
Comment thread core/src/test/java/io/substrait/relation/AggregateRelTest.java Outdated
Signed-off-by: Niels Pardon <par@zurich.ibm.com>
@nielspardon
nielspardon merged commit dc33518 into substrait-io:main Mar 16, 2026
12 checks passed
@nielspardon
nielspardon deleted the par-agg-group branch June 11, 2026 09:54
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.

2 participants