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

expr: add protobuf support for VariadicFunc (complete) #11902

Merged
merged 4 commits into from
Apr 20, 2022

Conversation

aalexandrov
Copy link
Contributor

@aalexandrov aalexandrov commented Apr 19, 2022

Leftovers from #11823.

Motivation

  • This PR adds a known-desirable feature.

    Closes #11749.

Tips for reviewer

I only need one from @wangandi or @lluki to take a look. The commits (in append order) do the following:

  1. Add a note that states why we need explicit Arbitrary implementations for most ~Func enums.
  2. Complete the implementation of the missing VariadicFunc variants from the various traits and the *.proto file.
  3. Swap some field tags from the ProtoVariadicFunc definition so the most common cases receive a 1-byte tag.
  4. Fix a bad range in any_fixed_offset.

Testing

  • This PR has adequate test coverage / QA involvement has been duly considered.

Tested with

cargo test -p mz-expr variadic_func_protobuf_roundtrip -- --nocapture

Release notes

N/A

@aalexandrov aalexandrov self-assigned this Apr 19, 2022
@aalexandrov aalexandrov added A-compute Area: compute C-feature Category: new feature or request labels Apr 19, 2022
@aalexandrov aalexandrov requested a review from lluki April 19, 2022 17:58
src/expr/src/scalar/func.rs Outdated Show resolved Hide resolved
Add a note explaining why we need explicit `Arbitrary`
implementations for most `~Func` enums and when we can
revert back to the derive-based variants.
@aalexandrov aalexandrov merged commit 10c8467 into MaterializeInc:main Apr 20, 2022
@aalexandrov aalexandrov deleted the protobuf_variadic_func branch April 20, 2022 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compute Area: compute C-feature Category: new feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants