Add a maps extension with maps.merge - #1409
Conversation
CEL has no operator for combining two maps: + concatenates strings, bytes, and lists but is not defined for maps, and there is no maps extension. Add maps.merge(a, b), returning a new map with the entries of both and the second argument's values winning on conflicting keys. The merge is shallow, so a value that is itself a map is replaced rather than merged. This is the replace step of the merge semantics discussed in cel-expr#1240; set-if-absent and recursive merge are left for follow-ups. Includes cost estimation and tracking that scale with the combined size of both inputs, registration in the extension option factory and the repl, and documentation in ext/README.md.
51aea9b to
edbceda
Compare
Per review, maps.merge(a, b) becomes a.merge(b). The direction is unchanged, so the argument still wins on conflicting keys. The overload ID drops the namespace segment to match the member naming in ext/strings.go and ext/lists.go, so map_maps_merge_map becomes map_merge_map. The cost estimator now reads the receiver from target rather than args[1]. The checker passes the receiver separately for member calls, so the previous len(args) != 2 guard returned nil and the estimate stopped covering the tracked cost. Updates the doc comment, ext/README.md, and the test expressions to the member form.
| func (mapsLib) CompileOptions() []cel.EnvOption { | ||
| mapType := cel.MapType(cel.TypeParamType("K"), cel.TypeParamType("V")) | ||
| return []cel.EnvOption{ | ||
| cel.Function("merge", |
There was a problem hiding this comment.
Let's maybe call this something other than merge.
A couple of good examples might be putAll (Java convention) or extend (Rust) since merge might imply semantics like combining the values of a colliding key.
I'm inclined toward putAll since it's easy to document what it does, and you could have other methods like <map(K,V)>.put(K, V) -> map(K,V) if you need to update a specific key (and you might want to add <map(K,V)>.remove(K) -> map(K,V), <map(K,V)>.removeAll(<list(K)>) -> map(K,V), map(K,V).keys(), map(K,V).values() while you're at it)
|
|
||
| // trackMapsMergeCost mirrors estimateMapsMergeCost against the actual inputs. | ||
| func trackMapsMergeCost(args []ref.Val, _ ref.Val) *uint64 { | ||
| entries := safeAdd(actualSize(args[0]), actualSize(args[1])) |
There was a problem hiding this comment.
I'm in the process of overhauling the cost framework, so this is fine for now, but please add a TODO to update this method once the new model is available.
| // error or unknown value. | ||
| func copyEntries(m traits.Mapper, dst map[ref.Val]ref.Val) ref.Val { | ||
| it := m.Iterator() | ||
| for it.HasNext() == types.True { |
There was a problem hiding this comment.
Consider using the folder interfaces to manage setting entries from traits.Foldable
Description
CEL has no way to combine two maps. The
+operator concatenates strings, bytes, and lists, but has no map overload, and there is nomapsextension.This adds one, with a single function:
It returns a new map holding the entries of both arguments, with the second argument's values winning on conflicting keys. Neither input is modified. The merge is shallow, so a value that is itself a map is replaced rather than merged recursively.
Scope
This is deliberately only the
replacestep from #1240. @TristonianJones suggested starting there:So set-if-absent and recursive merge are left for follow-ups rather than guessed at here.
On the API shape, I went with a bare
maps.merge(a, b)in a newmapsnamespace rather than a single entry point taking a strategy argument, because that is what the existing libraries do:sets.contains/sets.equivalent/sets.intersects,math.ceil/math.floor/math.abs. No function inexttakes a mode or strategy parameter today, so a latermaps.mergeIfAbsentwould fit that pattern without changing this signature. Happy to reshape it if you would rather have something else.Implementation notes
ext/sets.go. Without them the checker would price this as a fixed cost of 1 despite the O(n+m) copy.mapAllocCostis added toext/costs.goalongside the existinglistAllocCost.ext/extension_option_factory.go(factory and alias) and inrepl/evaluator.go.ext/README.mdgains aMapssection in the style of the surrounding entries.Testing
ext/maps_test.gocovers empty inputs, disjoint keys, conflicting keys, nested maps being replaced rather than merged, non-string keys, merging a variable, associativity over disjoint keys, compile-time rejection of non-map arguments, non-map arguments at runtime, and that the cost estimate scales with input size and brackets the tracked cost.go build ./...,go test ./...,go vet ./ext/...andgofmtare clean.