[Epic] Protobuf support for ComputeCommand

[uce]

This story is part of the overall versioning epic that spans all teams.

Goals

• Introduce protobuf types for transitive dependencies of ComputeCommand
• Implement mapping between regular and Protobuf-variants of types

Non-Goals

• End-to-end version upgrades

Acceptance criteria

The communication layer accepts data in a stable representation specific to the API versions it supports.

Subtasks

The following subtasks enumerate all structures that need to be serialized as Protobuf.

• [x] #12242
• [x] #12461
• [x] #11737
• [x] #12240
• [x] #11971
• [x] #11738
• [x] #11739
• [x] #11927
• [x] #11928
• [x] #11970
• [x] #11929
• [x] #11930
• [x] #11740
• [x] #11742
• [x] #11743
• [x] #11744
• [x] #11745
• [x] #12125
• [x] #12333
• [x] #11972
• [x] #11973
• [x] #11750
• [x] #11751
• [x] #11752
• [x] #11753
• [x] #11754
• [x] #11755
• [x] #11760
• [x] #11759
• [x] #11756

Related

• Implementation strategy alternatives (Google doc)
• Binary architecture braindump (Notion)

[aalexandrov]

@sploiselle / @mjibson as we are working our way through all of the types that are listed in this meta-issue, I am a bit worried about conflicts with concurrent modifications on these structures due to SQL evolution. Do you know issues that are planned for the next 2-3 weeks on that end? We might have to coordinate a bit in order to avoid massive merge conflicts.

[sploiselle]

All of the items listed in mz_expr are stable and I don't expect any substantial changes to them in the next 2-3 weeks. Are there any particular changes you're worried about?

@petrosagg has been working on moving the mz_expr::scalar::func::* implementations to dynamic dispatch on and off again. Currently, this is only supported for UnaryFunc, so that code is in a bit of an odd state. If you can make the current split state of the code work with protobufs, though, I imagine that it will be possible for us to reach the end goal of a world where all of those items are handled via dynamic dispatch.

[aalexandrov]

No, I just want to raise awareness that changes such as adding new function types or moving types around might create conflicts for types that are currently in progress in this issue. I am working on UnaryFunc now, but sadly this is not a one-day job. If we are not going to touch the outstanding types in the next 2-3 weeks we can avoid such conflicts.

[sploiselle]

Ah @aalexandrov, we're likely to add new functions in the next 2-3 weeks, but those items are most likely to be additions of very small changes. The SQL team is glad to handle/help during the rebase process if that turns out to be challenging whatsoever.

[petrosagg]

@sploiselle I moved a few more functions over https://github.com/MaterializeInc/materialize/pull/11788

We need a few more pushes to finally complete the transition

[aalexandrov]

We need a few more pushes to finally complete the transition

I think we can allocate some cycles of doing that as part of this effort.

[aalexandrov]

@sploiselle We started merging stubs definitions with trait implementations that contain todo!() placeholders for most function variants. See #11762 for example, similar PRs for BinaryFunc and VariadicFunc will land until the end of the week.

With those changes, when you want to add new function variants in the next weeks, you will be forced to extend the corresponding protobuf message type and the From<&{Arity}Func> for Proto{Arity}Func and TryFrom<Proto{Arity}Func> for {Arity}Func trait implementations.

If you give me a heads up with the shapes of the new function variant structs, I can give you more concrete guidance how to encode them in the mirroring Proto{Arity}Func type and how to implement the missing arms.

[sploiselle]

@aalexandrov Sounds good--thank you!

[lluki]

Do we need support timely::progress::Timestamp typed commands or is mz_repr::Timestamp (= a single u64) enough?

[maddyblue]

I'm not sure about that specific question but we need to support changing mz_repr::Timestamp to an i64, which may happen at some later date, so just don't make things too dependent on u64.

[lluki]

So if at any point in time (ignoring a phase where we have to remain backwards compatible) only have to support one type, then I'd simply encode it as u64 for now. If we go to i64 that should not be too complicated, as far as I can see there are only two cases where we actually encode it in a message (Peek and Plan::Constant).

If on the other side we use these two or more different timestamp types at the same time, it might be worth it to come up with something smarter (to automatically infer the type from a message, for example).

[maddyblue]

We will only ever have one. If we switch, it'll be everything at once.

[aalexandrov]

Closing this now... :tada: :tada: :tada: