Repository navigation
Add a maps extension with putAll - #1409
Open
lopster568 wants to merge 1 commit into
Open
lopster568 wants to merge 1 commit into
lopster568 wants to merge 1 commit into
Conversation
lopster568
force-pushed
the
maps-merge-extension
branch
from
August 8, 2026 03:44
51aea9b to
edbceda
Compare
Collaborator
|
/gcbrun |
1 similar comment
Collaborator
|
/gcbrun |
TristonianJones
previously approved these changes
Aug 29, 2026
Collaborator
|
/gcbrun |
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 a.putAll(b), a member function returning a new map with the entries of both maps and the argument's values winning on conflicting keys. Neither input is modified. The copy 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. The cost is described by an OverloadModel: the call charges for visiting every entry of both maps plus a map allocation, and the result holds between the larger input's entry count and the sum of both, with keys and values drawn from either input. The putAll cases are added to the sizing strategy soundness test. Registers the library in the extension option factory and the repl, and documents it in ext/README.md.
lopster568
force-pushed
the
maps-merge-extension
branch
from
November 3, 2026 07:46
d081260 to
117cd07
Compare
Contributor
Author
|
Rebased onto master and moved the cost estimate to |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rebased on master since the August approval. The cost code is ported to
OverloadModel(see Implementation notes), and arepltest that was failing CI is fixed (see Testing). The API and semantics are unchanged.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 member function:
It returns a new map holding the entries of both maps, and the argument's values win on conflicting keys. Neither input is modified. The copy 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.
Implementation notes
OverloadModelregistered withcel.CostModel, following the ext/lists migration in Migrate ext/lists to OverloadModel and add strategy soundness tests #1566. The call charges for visiting every entry of both maps plus a map allocation. The result size ranges from the larger input's entry count (every key collides) to the sum of both (no key collides), with key and value sizes taken from either input.Findrather thanFold, for the reason given in the review thread:Foldhands back native Go values for reflect and string backed maps, which would then have to be adapted again.ext/extension_option_factory.go(factory and alias) and inrepl/evaluator.go.ext/README.mdgains aMapssection.Testing
ext/maps_test.gocovers empty inputs, disjoint keys, conflicting keys, nested maps being replaced rather than merged, non-string keys, a variable receiver, compile-time rejection of non-map arguments, non-map arguments at runtime, and cost estimates and tracked costs at the latest model version and at version 0. The putAll cases are also added toTestCostSoundnessBySizingStrategy.The earlier
cel-go-builderrun failed in itsbazel-teststep. Runningcd repl && go test ./...reproduced a failure inTestProcessOptionError, whose expected list of extension options did not includemaps. repl is a separate module, so mygo test ./...from the root never covered it. That is fixed.go build ./...andgo test ./...pass in every module exceptconformance, where the policy tests need Bazel runfiles and fail the same way on master.go vet ./...is clean for this change; its only findings areunreachable codewarnings in the generated ANTLR parsers, the same count as on master. I did not run the Bazel build locally.