Inconsistency in getMilliseconds behaviour

[jsannemo]

The spec describes getMilliseconds as returning the milliseconds part (i.e. a value 0-999) of a duration.

The conformance test instead specifies this as returning the duration in milliseconds:

    name: "get_milliseconds"
    description: "Need to import a variable to get milliseconds."
    expr: "x.getMilliseconds()"
    type_env {
      name: "x"
      ident: { type: { message_type: "google.protobuf.Duration" } }
    }
    bindings {
      key: "x"
      value {
        value {
          object_value {
            [type.googleapis.com/google.protobuf.Duration]
            {
              seconds: 123
              nanos: 123456789
            }
          }
        }
      }
    }
    value: { int64_value: 123123 }

The cpp implementation computes according to spec: https://github.com/google/cel-cpp/blob/9841e3ee251f3cc4cd5b6dd9deee6818bc9f2854/eval/public/builtin_func_registrar.cc#L748-L752

The go implementation according to conformance: https://github.com/google/cel-go/blob/3ea8bd382b117069f37eac88d1dd3b89eadc4435/common/types/duration.go#L177-L179

Which behaviour is correct?

[TristonianJones]

Thanks for bringing this up. I noticed this mismatch in the conformance test results as well and was planning on looking into it after some other fixes.

In order to triage the issue, I need to look over some of the original commits. However, I expect that most users will use this method to return the duration as milliseconds since this is a common conversion practice. What could have happened is that the function and spec were written when timestamps were being introduced and there are functions available for accessing hours, minutes, seconds, etc. from a timestamp and the behavior for duration may have been done the same way for consistency of naming without a strong consideration of the intended use.

[jsannemo]

Hi @TristonianJones! Do you have any update on what behaviour should be the correct one?

[TristonianJones]

@jsannemo Sorry for the delay in my response. It's been quite a year, and now I'm getting back into the swing of things. I really appreciate all your detailed reports. The C++ implementation and spec are correct; however, these behaviors will be deprecated in future revisions in favor of functions which mirror the current Go and conformance behavior.