[go: nahoru, domu]

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

Proposed updates to support required and optional enumerations. #49

Merged
merged 3 commits into from
Oct 12, 2021

Conversation

jacques-n
Copy link
Contributor

No description provided.

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.

I'll try and look at functions.yaml in more detail later (but don't wait for me)

site/docs/expressions/scalar_functions.md Outdated Show resolved Hide resolved
Comment on lines +24 to 31
message Enum {
oneof enum_kind {
string specified = 1;
Empty unspecified = 2;
}

message Empty {}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
message Enum {
oneof enum_kind {
string specified = 1;
Empty unspecified = 2;
}
message Empty {}
}
message Enum {
optional string value = 1;
}

Maybe I'm misunderstanding protobof?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You could do this but it makes thing more implicit. The way I've done it makes it clearer (albeit slightly larger on the wire). Optional is implicit in pb3 so doesn't have to be stated. The union also allows us to introduce more than two options later.

The other nice thing is that the "enum_kind" becomes something to switch off of in several languages.

binary/function.proto Show resolved Hide resolved
extensions/aggregate_functions.yaml Outdated Show resolved Hide resolved
jacques-n and others added 2 commits October 6, 2021 19:57
@jacques-n jacques-n merged commit 3bb3d33 into substrait-io:main Oct 12, 2021
@jacques-n jacques-n deleted the enum_options branch October 12, 2021 03:52
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.

None yet

2 participants