fix(isthmus): adding support for new proto and deprecated form in AggregateRel#521
Conversation
… of Grouping for the AggregateRel
|
|
…r the AggregateRel
vbarua
left a comment
vbarua
left a comment
There was a problem hiding this comment.
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.
…oto in RelProtoConverter
…of proto in RelProtoConverter
…ithub.com:gord02/substrait-java into gordon.hamilton/aggregateGroupingNewSubstraitForm
… of Grouping for the AggregateRel
…r the AggregateRel
…of proto in RelProtoConverter
dfd50eb to
0a13b15
Compare
…multi grouping set test
…ithub.com:gord02/substrait-java into gordon.hamilton/aggregateGroupingNewSubstraitForm
vbarua
left a comment
vbarua
left a comment
There was a problem hiding this comment.
Overall looks good. Left some final suggestions, along with one improvement.
vbarua
left a comment
vbarua
left a comment
There was a problem hiding this comment.
Looks to me. Thanks for working on this!
|
Just commenting here in case others are confused about this in the future. This PR only introduces support for the old and new format to be consumed, but only the old format is produced. See #744 to see where we begin emitting both the old and the new field. |
4 participants
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.
Key Changes: