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: initial batch of proto definitions #16

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

roeap
Copy link

@roeap roeap commented Jul 7, 2024

This is just a first draft of what it might look like to generate models via protobuf as discussed in the RFC.

A couple of points that may be worth discussion if this is the way this project wants to go.

  • I used buf to do the heavy lifting, thus avoiding having to write obscure protoc commands.
  • The definitions contain some protovalidate annotations, which due to the lack of an implementation do nothing for the rust world. However clients may be able to leverage. Specifically for instance a prospective web ui. As a corollary, personally I do enjoy using the connect protocol for frontend stuff quite a bit, which can be upcast to protobuf in e.g. envoy.
  • I tried following what buf considers best practices for protobuf, however this als leads to some nested / verbose types, which may not ne desired here.
  • personally I like just as a command runner. this may not be a universal opinion :). introducing some makefile-like thing will eventually be a good thing i believe ...

Happy to hear any feedback and if I should continue with this.

@abhiaagarwal
Copy link
Collaborator

Thank you for this @roeap! @abrassel and I chatted about this a bit on slack, I'm glad you went the route of using pbjson to generate serde compatible structs.

Question: why does it create manually implement Ser/De traits instead of deriving? Is it just protoc-gen-prost-serde's behavior?

@roeap
Copy link
Author

roeap commented Jul 7, 2024

Question: why does it create manually implement Ser/De traits instead of deriving? Is it just protoc-gen-prost-serde's behavior?

@abhiaagarwal - yes, that's the default behavior. Reason being that the protobuf spec has an opinion on what the json representation of a protobuf message looks like. For example accepting snace_case and camelCase field names or how oneof types are represented.

As it turns out, this also makes for nicer rust types. For instance this ..

message Dependency {
  oneof dependency {
    TableDependency table = 1;
    FunctionDependency function = 2;
  }
}

gives us a nice Rust Enum, while accepting e.g.

{
  "table": ...
}

and erroring when both keys are present (which i assumed was the desired bahvior in this case 😄)

@abhiaagarwal
Copy link
Collaborator

abhiaagarwal commented Jul 7, 2024

Awesome, thanks! I'll see if I can use this PR as a basis for the server code I have over at #9. I'm inclined to merge this once it's ready, I'll just let it ruminate for a couple days to see if anyone has any opinions (I'd love @abrassel to take a look at this!)

@abrassel
Copy link
Collaborator

abrassel commented Jul 8, 2024

@abhiaagarwal @roeap Thanks for pinging me! I took a look through and this looks pretty great to me. At the moment, I'm a bit myopically focused on building the rust client, but certainly being able to leverage the proto layer will make my life so much easier.

@roeap can we use these protobuf definitions compatibly with the current unitycatalog REST API? Sorry, hope that question was clear.

@roeap roeap marked this pull request as ready for review July 24, 2024 17:37
Copy link
Collaborator

@abhiaagarwal abhiaagarwal left a comment

Choose a reason for hiding this comment

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

Thanks for doing this, finally getting some time to look. Just a couple comments.


// A map of key-value properties attached to the securable.
// TODO(roeap): maps cannot be optional, how do we distinguish between an empty map and no map?
map<string, string> properties = 3;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Even though this is marked as a TODO, I think it's fine to say "an empty map is no map".

map<string, string> properties = 4;

// Time at which this catalog was created, in epoch milliseconds.
int64 created_at = 5;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of using Integers, should we use Google's Timestamp type? It's basically an struct of (epoch milliseconds, epoch nanoseconds). Ditto to all other created_at/updated_at fields.

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.

3 participants