search

google / cel-spec

Common Expression Language -- specification and binary representation
https://cel.dev
Apache License 2.0
2.78k stars 222 forks source link

Packing conformance test structures instead #197

Closed jsannemo closed 2 years ago

jsannemo commented 3 years ago

The conformance tests send very deeply nested structures to test the depth 32 required by the spec. For some cases, this unfortunately causes Java gRPC to run in to parsing stack limits. A suggestion would be to pack values into an Any instead for conformance, to make life easier for gRPC implementations with such limits (C# is the same for example, as I understand it).

slott56 commented 3 years ago

The Python implementation also requires relaxing the stack boundaries to permit the tests to pass.

TristonianJones commented 3 years ago

@jsannemo thanks for the report. Thus far we have not encountered any issues with the conformance tests in Java, C++, or Go so I'm curious which stack hits the issue. Could you share a bit more about your use case?

Perhaps I'm mistaken but the proto deserialization issues would just be deferred from API ingress to test execution in the packed Any case. Is the issue that the proto recursion limits cannot be relaxed easily in gRPC, but could be more easily relaxed later in the evaluation?

jsannemo commented 3 years ago

@TristonianJones exactly so. I've left the corresponding comment on grpc-java, but have very little hopes for that to be solved anytime in the near future. Outside of gRPC, it is considerably easier (meaning, actually possible) to configure increased recursion limits.

The exact limit hit is an internal proto deserialization limit, which by default restricts the depth of any proto field path to 100 (and 3*32 + some overhead >= 100). The factor 3 comes from chaining expr -> createstruct -> entry -> expr -> ... in parse/message_literal. Note, in particular, that an environment only needs to serialize/deserialize when talking to the conformance server. During execution, this limit is never hit.

The use-case is simply parsing and evaluating the tests in Java.

TristonianJones commented 2 years ago

It's very possible that I will adjust the supported limit from 32 (to 16-24) since there are other issues with this limit elsewhere. It sounds like it's possible this would also potentially avoid the scenario you've encountered without affecting test readability.

Thoughts?

jsannemo commented 2 years ago

That works too. I guess it mostly depends on how compatible you aim to keep your own implementations. Probably no CEL expression writer would do so outside the context of a particular implementation, which would anyway depend on a bunch of unspecified specifics among the standard functions?