Skip to content

Comments

feat: add missing rels to rel message#582

Merged
EpsilonPrime merged 3 commits intosubstrait-io:mainfrom
westonpace:feat/add-rels-to-tree
Dec 11, 2023
Merged

feat: add missing rels to rel message#582
EpsilonPrime merged 3 commits intosubstrait-io:mainfrom
westonpace:feat/add-rels-to-tree

Conversation

@westonpace
Copy link
Member

@westonpace westonpace commented Dec 8, 2023

The ReferenceRel, WriteRel, and DdlRel were defined in algebra.proto but not part
of message Rel which meant they were unusable. This PR adds those back. It is
inspired by #288 but more targeted in scope. One change from that original PR which I
also kept was replacing the word tuple with record in the documentation for consistency.

This is not to imply that ReferenceRel, WriteRel, or DdlRel are "complete" or "stable"
in any way. I feel these relations are still quite ill defined. However, my hope is that by
making them usable we can inspire further change to them.

BREAKING CHANGE: The enum WriteRel::OutputMode had an option change from
OUTPUT_MODE_MODIFIED_TUPLES to OUTPUT_MODE_MODIFIED_RECORDS
BREAKING CHANGE: The message AggregateFunction.ReferenceRel has moved
to ReferenceRel.

@github-actions
Copy link

github-actions bot commented Dec 8, 2023

ACTION NEEDED

Substrait follows the Conventional Commits
specification
for
release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

@westonpace
Copy link
Member Author

This is failing the breaking change check for these two reasons:

 proto/substrait/algebra.proto:560:36:Enum value "2" on enum "OutputMode" changed name from "OUTPUT_MODE_MODIFIED_TUPLES" to "OUTPUT_MODE_MODIFIED_RECORDS".
proto/substrait/algebra.proto:1354:1:Previously present message "AggregateFunction.ReferenceRel" was deleted from file.

Since these were technically unreachable features (from Plan or ExtendedExpression) I am hesitant to label this an actual breaking change. I'm willing to go ahead and do so though if others feel it is needed.

@EpsilonPrime
Copy link
Member

This is failing the breaking change check for these two reasons:

 proto/substrait/algebra.proto:560:36:Enum value "2" on enum "OutputMode" changed name from "OUTPUT_MODE_MODIFIED_TUPLES" to "OUTPUT_MODE_MODIFIED_RECORDS".
proto/substrait/algebra.proto:1354:1:Previously present message "AggregateFunction.ReferenceRel" was deleted from file.

Since these were technically unreachable features (from Plan or ExtendedExpression) I am hesitant to label this an actual breaking change. I'm willing to go ahead and do so though if others feel it is needed.

I'd be inclined to just say that it will break to compilation for anyone using them directly.

@westonpace
Copy link
Member Author

I'd be inclined to just say that it will break to compilation for anyone using them directly.

Fair enough.

Copy link
Member

@EpsilonPrime EpsilonPrime left a comment

Choose a reason for hiding this comment

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

A good step forward, thanks!

Copy link
Contributor

@jacques-n jacques-n left a comment

Choose a reason for hiding this comment

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

LGTM

@EpsilonPrime EpsilonPrime merged commit d952b45 into substrait-io:main Dec 11, 2023
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