feat!: remove deprecated Aggregate.Grouping.grouping_expressions

[nielspardon]

BREAKING CHANGE: removed Aggregate.Grouping.grouping_expressions field

In September 2024 #706 introduced the Aggregate.Grouping.expression_references replacing Aggregate.Grouping.grouping_expressions. This PR removes the deprecated field.

---

This change is 

[benbellick]

Same comment as in the other PR:

I haven't investigated myself yet but has this migration been managed within the reasonably maintained implementation libraries? (For us, this means substrait-{go, java, py, rs})

[benbellick]

AFAICT substrait-java still emits the old field and not the new one. It does accept both. The handling was introduced here

[nielspardon]

Same comment as in the other PR:

I haven't investigated myself yet but has this migration been managed within the reasonably maintained implementation libraries? (For us, this means substrait-{go, java, py, rs})

• substrait-go feat: AggregateRel should use grouping references substrait-go#94
• substrait-rs has not a single mention of grouping
• substrait-java has been partially migrated (missed the RelProtoConverter changes and potentially more work handle the new grouping behavior using references substrait-java#299): fix(isthmus): adding support for new proto and deprecated form in AggregateRel substrait-java#521
• substrait-python already uses expression_references: https://github.com/substrait-io/substrait-python/blame/8c80fe273875c1d1c6463fa1758d86da81fd5c3d/src/substrait/builders/plan.py#L443

[benbellick]

substrait-rs is really just a wrapper around the protos so as long as the protos are up to date, there is no problem.

---

Looks like we will need to resolve the substrait-java issue though. I think it makes sense to let that release of substrait-java with that change sit for some time (>1month).

[nielspardon]

Looks like we will need to resolve the substrait-java issue though. I think it makes sense to let that release of substrait-java with that change sit for some time (>1month).

I don't think that is necessary. We don't really have a 1.0 release of substrait / substrait-java so I think it should be ok to introduce such changes without long wait times especially since we had this protobuf change deprecated for almost 1.5 years now. A lot of people don't pay attention to deprecations anyway and only update their code when they are forced to.

[nielspardon]

• substrait-java fix for RelProtoConverter fix(core): use new aggregate grouping behavior in RelProtoConverter substrait-java#744

[benbellick]

We are currently performing the substrait-java upgrade internally v0.86.0 which includes the contribution from @nielspardon to emit both the old and new fields. However, the upgrade is not finished.

As such, I request that we withhold from performing this breaking change until I can confirm that it will have no impact on our systems. Thanks!

[nielspardon]

We are currently performing the substrait-java upgrade internally v0.86.0 which includes the contribution from @nielspardon to emit both the old and new fields. However, the upgrade is not finished.

As such, I request that we withhold from performing this breaking change until I can confirm that it will have no impact on our systems. Thanks!

Does merging a PR into the Substrait spec repo automatically trigger an upgrade of your internal consumers? Isn't there a cascade of steps in between like:

• first a new release of Substrait
• a PR for the Substrait SDKs which need to be reviewed, approved, merged to update Substrait in the SDKs
• a new release of the Substrait SDKs including the Substrait update
• PRs for your internal systems to pick up the new Substrait SDK versions

[vbarua]

Does merging a PR into the Substrait spec repo automatically trigger an upgrade of your internal consumers?

It does not, but it prevents us from picking up other useful changes after this get merged.

A lot of people don't pay attention to deprecations anyway and only update their code when they are forced to.

That's us in this case 😓. I do appreciate you pushing through on removing these deprecations. It's a good forcing function for us to finish off our own internal migrations. For this, I think we should be set soon, but we would appreciate holding off for a couple more days.