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: stub protobuf support for VariadicFunc #11823

Merged
merged 2 commits into from
Apr 19, 2022

Conversation

aalexandrov
Copy link
Contributor

@aalexandrov aalexandrov commented Apr 15, 2022

Adds partial protobuf support for VariadicFunc.

Motivation

  • This PR adds a known-desirable feature.

First step towards resolving MaterializeInc/database-issues#3432.

Tips for reviewer

The first commit partially implements Arbitrary for VariadicFunc.

  • We need a custom implementation because the one synthesized by the derive macro is hitting a known proptest issue1.
  • More arms need to be added as we build up the protobuf support for VariadicFunc towards closing MaterializeInc/database-issues#3432.

The third commit changes things as follows:

  • Adds ProtoVariadicFunc to func.proto. Variants that are not properly modeled as protobuf yet are prefixed with an // unsupported: comment.
  • Implements From<&VariadicFunc> for ProtoVariadicFunc in func.rs. Arms that are not yet supported have a todo!() placeholder.
  • Implements TryFrom<ProtoVariadicFunc> for VariadicFunc in func.rs. Arms that are not yet supported have a todo!() placeholder.

Testing

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

Test coverage has been added for the variants that are already supported. As we add more variants, we will need to expand the custom Arbitrary implementation for VariadicFunc to keep this invariant.

Release notes

N/A

Footnotes

  1. https://github.com/AltSysrq/proptest/issues/152

@aalexandrov aalexandrov added C-feature Category: new feature or request A-compute Area: compute labels Apr 15, 2022
@aalexandrov aalexandrov self-assigned this Apr 15, 2022
@aalexandrov aalexandrov changed the title expr: add protobuf support for VariadicFunc (incomplete) expr: stub protobuf support for VariadicFunc Apr 15, 2022
@aalexandrov aalexandrov force-pushed the protobuf_variadic_func branch 4 times, most recently from 91bb13c to 2f414c8 Compare April 15, 2022 13:35
src/expr/src/proto/scalar/func.proto Outdated Show resolved Hide resolved
- Add `ProtoVariadicFunc` to `func.proto`. Variants that are
  not properly modeled as protobuf yet are prefixed with an
  `// unsupported:` comment.
- Implement `From<&VariadicFunc> for ProtoVariadicFunc` in `func.rs`.
  Arms that are not yet supported have a `todo!()` placeholder.
- Implement `TryFrom<ProtoVariadicFunc> for VariadicFunc` in `func.rs`.
  Arms that are not yet supported have a `todo!()` placeholder.
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