Thanks for those, they are very helpful and I think that CEL will be
very useful in what I'm attempting to do.
On Sun, 2022-03-20 at 11:15 -0700, 'Tristan Swadell' via CEL Go
Discussion Forum wrote:
> Hi Dan,
>
> Thanks for all of the contributions lately to CEL as well. It means a
> lot when people who use CEL want to improve it. I hope that you're
> finding it useful and if there are areas to improve ease of use, I'd
> be happy to take them as feature requests.
>
I'm impressed by the quality of the codebase, it makes on-boarding so
much easier when you can ask questions of the implementation and get
sensible answers from the source code.
> I have a couple of code suggestions which may simplify / optimize
> your approach:
>
> 1. CEL map literals are generally compatible with conversion to
> protobuf.Struct values. If you replace the `Struct{}` creation with a
> map `{}`, then you can remove the `cel.Container` reference since you
> won't be instantiating new protobuf types.
> > s.map(e,
> > has(e.other) && e.other != '', // inline filter
> > e.num.map(v1, e.let.map(v2, { // cartesian product with map
> > literal
> > "timestamp": now,
> > "other": e.other,
> > "numlet": e.num+e.let,
> > "num": v1,
> > "let": v2})))
>
This is cool. I think what I was doing wrong previously was not using
quoted strings and failing at that and so moving on. The detour through
google.protobuf.Struct helped me understand that.
The inline filter is a nice touch.
> 2. Instead of providing a function which returns a new timestamp
> every time now() is invoked, provide a lazy value
>
> > out, _, err := prg.Eval(map[string]interface{}{
> > "now": func() { time.Now().In(time.UTC) },
> > "s": ...
> > })
> >
I was unable to get this to work (adding a return value to the func
literal and adding a decls.NewVar("now", decls.Timestamp) to the
cel.Declarations — what am I missing?).
> 3. For the flatten method, prefer using the
> `types.NewMutableList(types.DefaultTypeAdapter)` and calling
> `ToImmutableList()` (new in CEL v0.10.*) on the result of
> flattenParts as it will avoid a large number of allocations. It's
> only safe to use the mutable list approach for intermediate
> calculations:
>
> > func flatten(arg ref.Val) ref.Val {
> > obj := arg
> > l, ok := obj.(traits.Lister)
> > if !ok {
> > return types.ValOrErr(obj, "no such overload")
> > }
> > dst := types.NewMutableList(types.DefaultTypeAdapter)
> > flattenParts(dst, l)
> > return dst.ToImmutableList()
> > }
>
I noticed that type, but wasn't sure how to use it; I can see that the
returned value from (*mutableList).Add does not need to be retained
since it mutates in-place, I think it would be helpful to add this in
the doc comment. Also, I'd suggest that NewMutableList returns a type
that indicates that it has the ToImmutableList() Lister method. I've
tested the following change and it all passes, and makes the code you
have above compile without a type assertion. If you are OK with the
change, I'm happy to send it.
diff --git a/common/types/list.go b/common/types/list.go
index f63aef4..0011a97 100644
--- a/common/types/list.go
+++ b/common/types/list.go
@@ -99,7 +99,7 @@ func NewJSONList(adapter ref.TypeAdapter, l *structpb.ListValue) traits.Lister {
//
// The mutable list only handles `Add` calls correctly as it is intended only for use within
// comprehension loops which generate an immutable result upon completion.
-func NewMutableList(adapter ref.TypeAdapter) traits.Lister {
+func NewMutableList(adapter ref.TypeAdapter) traits.MutableLister {
return &mutableList{
TypeAdapter: adapter,
baseList: nil,
diff --git a/common/types/traits/lister.go b/common/types/traits/lister.go
index f40e7ee..5cf2593 100644
--- a/common/types/traits/lister.go
+++ b/common/types/traits/lister.go
@@ -28,5 +28,6 @@ type Lister interface {
// MutableLister interface which emits an immutable result after an intermediate computation.
type MutableLister interface {
+ Lister
ToImmutableList() Lister
}
Since there is no function signature to match here, this seems
reasonable to change.
> 4. When iterating, consider using a predicate of `for it.HasNext() ==
> types.True`
>
Nice. I was wondering what the idiomatic way to express this was.
> As long as the CEL map literals you're constructing only contain
> string keys, I believe all CEL simple types are convertible to
> protobuf.Struct since it's a special type that has a native mapping
> in CEL.
>
> Cheers,
>
> -Tristan
thanks
Dan