Skip to content

feat(core,isthmus): support grouping set index in Aggregate #565

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Oct 23, 2025
Merged

feat(core,isthmus): support grouping set index in Aggregate #565

merged 4 commits into from
Oct 23, 2025

Conversation

@nielspardon
Copy link
Member

fixes #527

  • adds the int32 grouping set index to Aggregate.deriveRecordType()
  • adds Calcite mappings to isthmus which maps a GROUP_ID() aggregate function call mapping for the grouping set index since it has the same semantic as the Substrait grouping set index
  • ensures remap removes the grouping set index field if it there is no GROUP_ID() function call in the Calcite plan

@nielspardon
feat: support grouping set index in Aggregate
9944d55 
Signed-off-by: Niels Pardon <par@zurich.ibm.com>
@nielspardon nielspardon self-assigned this Oct 22, 2025
bestbeforetoday
bestbeforetoday reviewed Oct 23, 2025
View reviewed changes
Copy link
Contributor

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

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

Generally this looks great with clearly laid out code. Just a few very minor suggestions to improve the implementation. See in-line comments.

nielspardon and others added 3 commits October 23, 2025 16:31
@nielspardon @bestbeforetoday
fix: apply suggestions from code review
4c4d7b4 
Co-authored-by: Mark S. Lewis <Mark.S.Lewis@outlook.com>
@nielspardon
fix: change ArrayList to LinkedList
958ca81 
Signed-off-by: Niels Pardon <par@zurich.ibm.com>
@nielspardon
fix: include aggcall in exception message
efd82d9 
Signed-off-by: Niels Pardon <par@zurich.ibm.com>
bestbeforetoday
bestbeforetoday approved these changes Oct 23, 2025
View reviewed changes
Copy link
Contributor

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

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

👍

@bestbeforetoday bestbeforetoday merged commit a00a811 into substrait-io:main Oct 23, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bestbeforetoday bestbeforetoday bestbeforetoday approved these changes

@andrew-coleman andrew-coleman Awaiting requested review from andrew-coleman

Assignees

@nielspardon nielspardon

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add support for grouping set index in Aggregate

2 participants

@nielspardon @bestbeforetoday