search

substrait-io / substrait

A cross platform way to express data transformation, relational algebra, standardized record expression and plans.
https://substrait.io
Apache License 2.0
1.14k stars 148 forks source link

feat: add missing rels to rel message #582

Closed westonpace closed 8 months ago

westonpace commented 8 months ago

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[bot] commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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

Fair enough.