Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

duration() allows inf / -inf values #104

Closed
jsannemo opened this issue Dec 19, 2020 · 5 comments · Fixed by #120
Closed

duration() allows inf / -inf values #104

jsannemo opened this issue Dec 19, 2020 · 5 comments · Fixed by #120
Labels
bug Something isn't working

Comments

@jsannemo
Copy link

The string to Duration conversion accepts inf and -inf (since Abseil does) which produces internal durations that are not valid CEL durations.

@kyessenov kyessenov added the bug Something isn't working label Jan 29, 2021
@kyessenov
Copy link
Collaborator

@JimLarson I checked the spec, and it seems that only the format (+|-|)\d+s is a valid input to duration. Is it the case? Also, for reverse, how do we handle nanos?

@kyessenov
Copy link
Collaborator

@JimLarson gentle ping: is it correct to assume that duration() only takes round seconds as input?

@JimLarson
Copy link

Yes, assume that duration(string) --> g.p.duration only takes a signed int followed by "s". There's a request/proposal somewhere for a more flexible format with different units (y/d/h/m), and possibly even mixed units ("3d4h").

We're in the process of defining a set of duration conversion functions duration('120s').toMinutes() ==> 2, and it would be reasonable to add functions for "fromMinutes/Hours/...", but we don't have a proposal yet.

You might be able to create a literal google.protobuf.Duration message, but I we might have intentionally made that an "opaque" type where the proto representation is supposed to be an implementation detail and you're not supposed to create message literals or extract fields. I could be wrong on this - I tried writing a conformance test but it failed RPC'ing the results back to the driver, not an evaluation failure, so it might work even if we don't intend it to.

To get a duration with nanos, you can either bind such a duration message to a cel variable, or (utter hack warning) you can create timestamp literals with nanosecond precision, then take a difference of them to get a duration.

If you have an immediate need for a more flexible format for the duration() function, let us know what features you'd need.

@kyessenov
Copy link
Collaborator

Thanks for the response. In the context of latency measurements, I think microseconds and milliseconds would be nice to support. A second is a long time in that world.

@JimLarson
Copy link

Sounds good. Since an expanded duration notation would affect all CEL implementations, I'm going to refer you to google/cel-spec#138 for any further discussion along those lines and you can keep this issue just for fixing the +/- inf input as described in the original comment.

TristonianJones pushed a commit that referenced this issue Mar 31, 2021
…s in the

valid range.
Fixes #104.

PiperOrigin-RevId: 363701763
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants