Skip to content
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.

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

feat: add expression support for count and offset in the fetch operator #748

Merged
merged 11 commits into from
Dec 10, 2024

Conversation

jcamachor
Copy link
Contributor

@jcamachor jcamachor commented Nov 22, 2024

  • Update FetchRel in Substrait to support expressions for both offset and count.
    This is done in a way that should be effectively compatible with old systems. Old plans should be able to continue to be consumed treating the old values as int64.

BREAKING CHANGE: The encoding of FetchRel has changed in a strictly backwards incompatible way. The change involves transitioning offset and count from a standalone int64 field to a oneof structure, where the original int64 field is marked as deprecated, and a new field of Expression type is introduced. Using a oneof may cause ambiguity between unset and set-to-zero states in older messages. However, the fields are defined such that their logical meaning remains indistinguishable, ensuring consistency across encodings.

@CLAassistant
Copy link

CLAassistant commented Nov 22, 2024

CLA assistant check
All committers have signed the CLA.

site/docs/relations/logical_relations.md Outdated Show resolved Hide resolved
proto/substrait/algebra.proto Outdated Show resolved Hide resolved
EpsilonPrime
EpsilonPrime previously approved these changes Nov 25, 2024
EpsilonPrime
EpsilonPrime previously approved these changes Dec 4, 2024
westonpace
westonpace previously approved these changes Dec 4, 2024
Copy link
Member

@westonpace westonpace left a comment

Choose a reason for hiding this comment

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

Some pedantic nits but looks good otherwise!

proto/substrait/algebra.proto Outdated Show resolved Hide resolved
proto/substrait/algebra.proto Outdated Show resolved Hide resolved
site/docs/relations/logical_relations.md Outdated Show resolved Hide resolved
@jcamachor jcamachor dismissed stale reviews from westonpace and EpsilonPrime via 1b6fa78 December 5, 2024 20:33
@jcamachor
Copy link
Contributor Author

@EpsilonPrime , @westonpace , could you please re-cast your approvals so we can merge? Thanks

@EpsilonPrime EpsilonPrime merged commit bd4b431 into substrait-io:main Dec 10, 2024
13 checks passed
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.

6 participants