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

binary format support? #80

Open
flokli opened this issue Aug 27, 2024 · 9 comments
Open

binary format support? #80

flokli opened this issue Aug 27, 2024 · 9 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@flokli
Copy link

flokli commented Aug 27, 2024

I'd like to pass a more concise version of CEL expressions.

https://github.com/google/cel-spec mentions:

The required components of a system that supports CEL are:

    The textual representation of an expression as written by a developer. It is of similar syntax to expressions in C/C++/Java/JavaScript
    A binary representation of an expression. It is an abstract syntax tree (AST).
    A compiler library that converts the textual representation to the binary representation. This can be done ahead of time (in the control plane) or just before evaluation (in the data plane).
    A context containing one or more typed variables, often protobuf messages. Most use-cases will use attribute_context.proto
    An evaluator library that takes the binary format in the context and produces a result, usually a Boolean.

It currently looks like the only way to use this library is using Program::compile, passing the textual representation.

The result is not serializable currently (so we cannot persist it anywhere to survive a program restart), and it seems it's also not possible to pass a more binary form to there either.

Do you plan to add support for this?

@clarkmcc clarkmcc added enhancement New feature or request good first issue Good for newcomers labels Aug 27, 2024
@clarkmcc
Copy link
Owner

clarkmcc commented Aug 27, 2024

I don't see any problems with this request other than the binary representation (1) would not be stable between releases pre-1.0 (i.e. #59), and (2) would not be compatible with the binary representation of other CEL runtimes.

I need to find a binary format that can express the Expression enum, but I imagine postcard or bincode.

@flokli
Copy link
Author

flokli commented Aug 27, 2024

I think the spec even defines a format for expressions: https://github.com/google/cel-spec/blob/f027a86d2e5bf18f796be0c4373f637a61041cde/proto/cel/expr/syntax.proto#L40

Not sure if the goal is to have a binary representation that's compatible across different CEL libraries, the spec is a bit squishy on that…

@clarkmcc
Copy link
Owner

I could have sworn that cel-go said they were moving away from the heavy dependency on protobufs for everything. But I could be making that up. In any case, changing the binary format for expressions would be a pretty heavy change in this project, for something that (as you say) isn't strictly required, and for a use-case (compatibility across runtimes) that may not be extremely common.

I could be wrong though, and am happy to hear use-cases where consistent binary formats are important.

@flokli
Copy link
Author

flokli commented Aug 28, 2024

I opened google/cel-spec#385 to clarify that part of the spec.

Maybe there's value on having a standardized compact exchange format that can work across different implementations, but it might not need to be the same as the internal data format of course.

@flokli
Copy link
Author

flokli commented Aug 28, 2024

We heard back, sending a protobuf-encoded version of the AST "to send CEL programs over the wire" seems to be a valid exchange format.

So I think it'd make sense if we could render a parsed expression out as such a proto, and parse them back in. Maybe behind a feature flag, so you can disable it if someone doesn't use this functionality and doesn't want to pull in prost into their dependency tree?

@clarkmcc
Copy link
Owner

Thanks for confirming this! A feature flag and a .proto() method on the Expression makes sense to me. I'll work it in as I have time, or I am also happy to review a PR.

@flokli
Copy link
Author

flokli commented Sep 10, 2024

I took a first stab at this, can be found in the proto-types branch over here.

The data model is a bit different compared to what cel-rust uses internally, and we'd also need to assign IDs for expressions. Maybe it's easier (import-wise) to define the conversion functions inside the proto module, rather than as a method on Expression.

I'm not sure how soon I'll be able to get back to this, but I wanted to at least get the plumbing pushed out, so it's just the implementation now :-)

@clarkmcc
Copy link
Owner

Thank you! I'll put some cycles into adding the type conversions.

@flokli
Copy link
Author

flokli commented Sep 10, 2024

Thanks! With me having opened the draft PR, you should be able to push to the branch. Or just cherry-pick into one of yours :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants