diff --git a/common/src/main/java/dev/cel/common/values/BUILD.bazel b/common/src/main/java/dev/cel/common/values/BUILD.bazel index 53ffdda3d..0d1d5431f 100644 --- a/common/src/main/java/dev/cel/common/values/BUILD.bazel +++ b/common/src/main/java/dev/cel/common/values/BUILD.bazel @@ -118,6 +118,38 @@ java_library( ], ) +java_library( + name = "mutable_map_value", + srcs = ["MutableMapValue.java"], + tags = [ + ], + deps = [ + "//common/annotations", + "//common/exceptions:attribute_not_found", + "//common/types", + "//common/types:type_providers", + "//common/values", + "//common/values:cel_value", + "@maven//:com_google_errorprone_error_prone_annotations", + ], +) + +cel_android_library( + name = "mutable_map_value_android", + srcs = ["MutableMapValue.java"], + tags = [ + ], + deps = [ + ":cel_value_android", + "//common/annotations", + "//common/exceptions:attribute_not_found", + "//common/types:type_providers_android", + "//common/types:types_android", + "//common/values:values_android", + "@maven//:com_google_errorprone_error_prone_annotations", + ], +) + cel_android_library( name = "values_android", srcs = CEL_VALUES_SOURCES, diff --git a/common/src/main/java/dev/cel/common/values/MutableMapValue.java b/common/src/main/java/dev/cel/common/values/MutableMapValue.java new file mode 100644 index 000000000..706436b2e --- /dev/null +++ b/common/src/main/java/dev/cel/common/values/MutableMapValue.java @@ -0,0 +1,146 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package dev.cel.common.values; + +import com.google.errorprone.annotations.Immutable; +import dev.cel.common.annotations.Internal; +import dev.cel.common.exceptions.CelAttributeNotFoundException; +import dev.cel.common.types.CelType; +import dev.cel.common.types.MapType; +import dev.cel.common.types.SimpleType; +import java.util.Collection; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Optional; +import java.util.Set; + +/** + * A custom CelValue implementation that allows O(1) insertions for maps during comprehension. + * + *

CEL Library Internals. Do Not Use. + */ +@Internal +@Immutable +@SuppressWarnings("Immutable") // Intentionally mutable for performance reasons +public final class MutableMapValue extends CelValue + implements SelectableValue, Map { + private final Map internalMap; + private final CelType celType; + + public static MutableMapValue create(Map map) { + return new MutableMapValue(map); + } + + @Override + public int size() { + return internalMap.size(); + } + + @Override + public boolean isEmpty() { + return internalMap.isEmpty(); + } + + @Override + public boolean containsKey(Object key) { + return internalMap.containsKey(key); + } + + @Override + public boolean containsValue(Object value) { + return internalMap.containsValue(value); + } + + @Override + public Object get(Object key) { + return internalMap.get(key); + } + + @Override + public Object put(Object key, Object value) { + return internalMap.put(key, value); + } + + @Override + public Object remove(Object key) { + return internalMap.remove(key); + } + + @Override + public void putAll(Map m) { + internalMap.putAll(m); + } + + @Override + public void clear() { + internalMap.clear(); + } + + @Override + public Set keySet() { + return internalMap.keySet(); + } + + @Override + public Collection values() { + return internalMap.values(); + } + + @Override + public Set> entrySet() { + return internalMap.entrySet(); + } + + @Override + public Object select(Object field) { + Object val = internalMap.get(field); + if (val != null) { + return val; + } + if (!internalMap.containsKey(field)) { + throw CelAttributeNotFoundException.forMissingMapKey(field.toString()); + } + throw CelAttributeNotFoundException.of( + String.format("Map value cannot be null for key: %s", field)); + } + + @Override + public Optional find(Object field) { + if (internalMap.containsKey(field)) { + return Optional.ofNullable(internalMap.get(field)); + } + return Optional.empty(); + } + + @Override + public Object value() { + return this; + } + + @Override + public boolean isZeroValue() { + return internalMap.isEmpty(); + } + + @Override + public CelType celType() { + return celType; + } + + private MutableMapValue(Map map) { + this.internalMap = new LinkedHashMap<>(map); + this.celType = MapType.create(SimpleType.DYN, SimpleType.DYN); + } +} diff --git a/common/values/BUILD.bazel b/common/values/BUILD.bazel index f1fa107b6..74bfa9e0f 100644 --- a/common/values/BUILD.bazel +++ b/common/values/BUILD.bazel @@ -47,6 +47,18 @@ cel_android_library( exports = ["//common/src/main/java/dev/cel/common/values:values_android"], ) +java_library( + name = "mutable_map_value", + visibility = ["//:internal"], + exports = ["//common/src/main/java/dev/cel/common/values:mutable_map_value"], +) + +cel_android_library( + name = "mutable_map_value_android", + visibility = ["//:internal"], + exports = ["//common/src/main/java/dev/cel/common/values:mutable_map_value_android"], +) + java_library( name = "base_proto_cel_value_converter", exports = ["//common/src/main/java/dev/cel/common/values:base_proto_cel_value_converter"], diff --git a/extensions/src/main/java/dev/cel/extensions/BUILD.bazel b/extensions/src/main/java/dev/cel/extensions/BUILD.bazel index 77663f2fa..2eb26846f 100644 --- a/extensions/src/main/java/dev/cel/extensions/BUILD.bazel +++ b/extensions/src/main/java/dev/cel/extensions/BUILD.bazel @@ -307,6 +307,7 @@ java_library( "//common:options", "//common/ast", "//common/types", + "//common/values:mutable_map_value", "//compiler:compiler_builder", "//extensions:extension_library", "//parser:macro", diff --git a/extensions/src/main/java/dev/cel/extensions/CelComprehensionsExtensions.java b/extensions/src/main/java/dev/cel/extensions/CelComprehensionsExtensions.java index 7c298a773..3bf47c4a6 100644 --- a/extensions/src/main/java/dev/cel/extensions/CelComprehensionsExtensions.java +++ b/extensions/src/main/java/dev/cel/extensions/CelComprehensionsExtensions.java @@ -29,6 +29,7 @@ import dev.cel.common.ast.CelExpr; import dev.cel.common.types.MapType; import dev.cel.common.types.TypeParamType; +import dev.cel.common.values.MutableMapValue; import dev.cel.compiler.CelCompilerLibrary; import dev.cel.parser.CelMacro; import dev.cel.parser.CelMacroExprFactory; @@ -171,38 +172,46 @@ public void setParserOptions(CelParserBuilder parserBuilder) { parserBuilder.addMacros(macros()); } - // TODO: Implement a more efficient map insertion based on mutability once mutable - // maps are supported in Java stack. - private static ImmutableMap mapInsertMap( + private static Map mapInsertMap( Map targetMap, Map mapToMerge, RuntimeEquality equality) { - ImmutableMap.Builder resultBuilder = - ImmutableMap.builderWithExpectedSize(targetMap.size() + mapToMerge.size()); - - for (Map.Entry entry : mapToMerge.entrySet()) { - if (equality.findInMap(targetMap, entry.getKey()).isPresent()) { + for (Object key : mapToMerge.keySet()) { + if (equality.findInMap(targetMap, key).isPresent()) { throw new IllegalArgumentException( - String.format("insert failed: key '%s' already exists", entry.getKey())); - } else { - resultBuilder.put(entry.getKey(), entry.getValue()); + String.format("insert failed: key '%s' already exists", key)); } } - return resultBuilder.putAll(targetMap).buildOrThrow(); + + if (targetMap instanceof MutableMapValue) { + MutableMapValue wrapper = (MutableMapValue) targetMap; + wrapper.putAll(mapToMerge); + return wrapper; + } + + return ImmutableMap.builderWithExpectedSize(targetMap.size() + mapToMerge.size()) + .putAll(targetMap) + .putAll(mapToMerge) + .buildOrThrow(); } - private static ImmutableMap mapInsertKeyValue( - Object[] args, RuntimeEquality equality) { - Map map = (Map) args[0]; + private static Map mapInsertKeyValue(Object[] args, RuntimeEquality equality) { + Map mapArg = (Map) args[0]; Object key = args[1]; Object value = args[2]; - if (equality.findInMap(map, key).isPresent()) { + if (equality.findInMap(mapArg, key).isPresent()) { throw new IllegalArgumentException( String.format("insert failed: key '%s' already exists", key)); } + if (mapArg instanceof MutableMapValue) { + MutableMapValue mutableMap = (MutableMapValue) mapArg; + mutableMap.put(key, value); + return mutableMap; + } + ImmutableMap.Builder builder = - ImmutableMap.builderWithExpectedSize(map.size() + 1); - return builder.put(key, value).putAll(map).buildOrThrow(); + ImmutableMap.builderWithExpectedSize(mapArg.size() + 1); + return builder.put(key, value).putAll(mapArg).buildOrThrow(); } private static Optional expandAllMacro( diff --git a/extensions/src/test/java/dev/cel/extensions/BUILD.bazel b/extensions/src/test/java/dev/cel/extensions/BUILD.bazel index 0b6502410..19fd3657e 100644 --- a/extensions/src/test/java/dev/cel/extensions/BUILD.bazel +++ b/extensions/src/test/java/dev/cel/extensions/BUILD.bazel @@ -15,6 +15,7 @@ java_library( "//common:compiler_common", "//common:container", "//common:options", + "//common/exceptions:attribute_not_found", "//common/exceptions:divide_by_zero", "//common/exceptions:index_out_of_bounds", "//common/types", diff --git a/extensions/src/test/java/dev/cel/extensions/CelComprehensionsExtensionsTest.java b/extensions/src/test/java/dev/cel/extensions/CelComprehensionsExtensionsTest.java index 34696b688..374178540 100644 --- a/extensions/src/test/java/dev/cel/extensions/CelComprehensionsExtensionsTest.java +++ b/extensions/src/test/java/dev/cel/extensions/CelComprehensionsExtensionsTest.java @@ -15,28 +15,33 @@ package dev.cel.extensions; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assertWithMessage; import static org.junit.Assert.assertThrows; +import com.google.common.base.Throwables; +import com.google.common.collect.ImmutableMap; import com.google.testing.junit.testparameterinjector.TestParameter; import com.google.testing.junit.testparameterinjector.TestParameterInjector; import com.google.testing.junit.testparameterinjector.TestParameters; +import dev.cel.bundle.Cel; import dev.cel.common.CelAbstractSyntaxTree; import dev.cel.common.CelFunctionDecl; import dev.cel.common.CelOptions; import dev.cel.common.CelValidationException; +import dev.cel.common.exceptions.CelAttributeNotFoundException; import dev.cel.common.exceptions.CelDivideByZeroException; import dev.cel.common.exceptions.CelIndexOutOfBoundsException; import dev.cel.common.types.SimpleType; import dev.cel.common.types.TypeParamType; -import dev.cel.compiler.CelCompiler; -import dev.cel.compiler.CelCompilerFactory; import dev.cel.parser.CelMacro; import dev.cel.parser.CelStandardMacro; import dev.cel.parser.CelUnparser; import dev.cel.parser.CelUnparserFactory; import dev.cel.runtime.CelEvaluationException; -import dev.cel.runtime.CelRuntime; -import dev.cel.runtime.CelRuntimeFactory; +import dev.cel.testing.CelRuntimeFlavor; +import java.util.Map; +import org.junit.Assume; +import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -46,26 +51,35 @@ public class CelComprehensionsExtensionsTest { private static final CelOptions CEL_OPTIONS = CelOptions.current() + .enableHeterogeneousNumericComparisons(true) // Enable macro call population for unparsing .populateMacroCalls(true) .build(); - private static final CelCompiler CEL_COMPILER = - CelCompilerFactory.standardCelCompilerBuilder() - .setOptions(CEL_OPTIONS) - .setStandardMacros(CelStandardMacro.STANDARD_MACROS) - .addLibraries(CelExtensions.comprehensions()) - .addLibraries(CelExtensions.lists()) - .addLibraries(CelExtensions.strings()) - .addLibraries(CelOptionalLibrary.INSTANCE, CelExtensions.bindings()) - .build(); - private static final CelRuntime CEL_RUNTIME = - CelRuntimeFactory.standardCelRuntimeBuilder() - .addLibraries(CelOptionalLibrary.INSTANCE) - .addLibraries(CelExtensions.lists()) - .addLibraries(CelExtensions.strings()) - .addLibraries(CelExtensions.comprehensions()) - .build(); + @TestParameter public CelRuntimeFlavor runtimeFlavor; + @TestParameter public boolean isParseOnly; + + private Cel cel; + + @Before + public void setUp() { + // Legacy runtime does not support parsed-only evaluation mode. + Assume.assumeFalse(runtimeFlavor.equals(CelRuntimeFlavor.LEGACY) && isParseOnly); + this.cel = + runtimeFlavor + .builder() + .setOptions(CEL_OPTIONS) + .setStandardMacros(CelStandardMacro.STANDARD_MACROS) + .addCompilerLibraries(CelExtensions.comprehensions()) + .addCompilerLibraries(CelExtensions.lists()) + .addCompilerLibraries(CelExtensions.strings()) + .addCompilerLibraries(CelOptionalLibrary.INSTANCE, CelExtensions.bindings()) + .addRuntimeLibraries(CelOptionalLibrary.INSTANCE) + .addRuntimeLibraries(CelExtensions.lists()) + .addRuntimeLibraries(CelExtensions.strings()) + .addRuntimeLibraries(CelExtensions.comprehensions()) + .build(); + } private static final CelUnparser UNPARSER = CelUnparserFactory.newUnparser(); @@ -101,11 +115,7 @@ public void allMacro_twoVarComprehension_success( }) String expr) throws Exception { - CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst(); - - Object result = CEL_RUNTIME.createProgram(ast).eval(); - - assertThat(result).isEqualTo(true); + assertThat(eval(expr)).isEqualTo(true); } @Test @@ -127,11 +137,7 @@ public void existsMacro_twoVarComprehension_success( }) String expr) throws Exception { - CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst(); - - Object result = CEL_RUNTIME.createProgram(ast).eval(); - - assertThat(result).isEqualTo(true); + assertThat(eval(expr)).isEqualTo(true); } @Test @@ -156,11 +162,7 @@ public void exists_oneMacro_twoVarComprehension_success( }) String expr) throws Exception { - CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst(); - - Object result = CEL_RUNTIME.createProgram(ast).eval(); - - assertThat(result).isEqualTo(true); + assertThat(eval(expr)).isEqualTo(true); } @Test @@ -182,11 +184,7 @@ public void transformListMacro_twoVarComprehension_success( }) String expr) throws Exception { - CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst(); - - Object result = CEL_RUNTIME.createProgram(ast).eval(); - - assertThat(result).isEqualTo(true); + assertThat(eval(expr)).isEqualTo(true); } @Test @@ -210,11 +208,7 @@ public void transformMapMacro_twoVarComprehension_success( }) String expr) throws Exception { - CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst(); - - Object result = CEL_RUNTIME.createProgram(ast).eval(); - - assertThat(result).isEqualTo(true); + assertThat(eval(expr)).isEqualTo(true); } @Test @@ -229,6 +223,7 @@ public void transformMapEntryMacro_twoVarComprehension_success( + " 'key2': 'value2'}", // map.transformMapEntry() "{'hello': 'world', 'greetings': 'tacocat'}.transformMapEntry(k, v, {}) == {}", + "{'a': 1, 'b': 2}.transformMapEntry(k, v, {k: v}) == {'a': 1, 'b': 2}", "{'a': 1, 'b': 2}.transformMapEntry(k, v, {k + '_new': v * 2}) == {'a_new': 2," + " 'b_new': 4}", "{'a': 1, 'b': 2, 'c': 3}.transformMapEntry(k, v, v % 2 == 1, {k: v * 10}) == {'a': 10," @@ -238,24 +233,22 @@ public void transformMapEntryMacro_twoVarComprehension_success( }) String expr) throws Exception { - CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst(); - - Object result = CEL_RUNTIME.createProgram(ast).eval(); - - assertThat(result).isEqualTo(true); + assertThat(eval(expr)).isEqualTo(true); } @Test public void comprehension_onTypeParam_success() throws Exception { - CelCompiler celCompiler = - CelCompilerFactory.standardCelCompilerBuilder() + Assume.assumeFalse(isParseOnly); + Cel customCel = + runtimeFlavor + .builder() .setOptions(CEL_OPTIONS) .setStandardMacros(CelStandardMacro.STANDARD_MACROS) - .addLibraries(CelExtensions.comprehensions()) + .addCompilerLibraries(CelExtensions.comprehensions()) .addVar("items", TypeParamType.create("T")) .build(); - CelAbstractSyntaxTree ast = celCompiler.compile("items.all(i, v, v > 0)").getAst(); + CelAbstractSyntaxTree ast = customCel.compile("items.all(i, v, v > 0)").getAst(); assertThat(ast.getResultType()).isEqualTo(SimpleType.BOOL); } @@ -275,7 +268,7 @@ public void unparseAST_twoVarComprehension( }) String expr) throws Exception { - CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst(); + CelAbstractSyntaxTree ast = isParseOnly ? cel.parse(expr).getAst() : cel.compile(expr).getAst(); String unparsed = UNPARSER.unparse(ast); assertThat(unparsed).isEqualTo(expr); } @@ -318,8 +311,9 @@ public void unparseAST_twoVarComprehension( "{expr: \"{'hello': 'world', 'greetings': 'tacocat'}.transformMapEntry(k, v, []) == {}\"," + " err: 'no matching overload'}") public void twoVarComprehension_compilerErrors(String expr, String err) throws Exception { + Assume.assumeFalse(isParseOnly); CelValidationException e = - assertThrows(CelValidationException.class, () -> CEL_COMPILER.compile(expr).getAst()); + assertThrows(CelValidationException.class, () -> cel.compile(expr).getAst()); assertThat(e).hasMessageThat().contains(err); } @@ -339,34 +333,59 @@ public void twoVarComprehension_compilerErrors(String expr, String err) throws E + " '2.0' already exists\"}") public void twoVarComprehension_keyCollision_runtimeError(String expr, String err) throws Exception { - CelAbstractSyntaxTree ast = CEL_COMPILER.compile(expr).getAst(); - - CelEvaluationException e = - assertThrows(CelEvaluationException.class, () -> CEL_RUNTIME.createProgram(ast).eval()); - - assertThat(e).hasCauseThat().isInstanceOf(IllegalArgumentException.class); - assertThat(e).hasCauseThat().hasMessageThat().contains(err); + // Planner does not allow decimals for map keys + Assume.assumeFalse(runtimeFlavor.equals(CelRuntimeFlavor.PLANNER) && expr.contains("2.0")); + + CelEvaluationException e = assertThrows(CelEvaluationException.class, () -> eval(expr)); + Throwable cause = + Throwables.getCausalChain(e).stream() + .filter(IllegalArgumentException.class::isInstance) + .filter(t -> t.getMessage() != null && t.getMessage().contains(err)) + .findFirst() + .orElse(null); + + assertWithMessage( + "Expected IllegalArgumentException with message containing '%s' in cause chain", err) + .that(cause) + .isNotNull(); } @Test public void twoVarComprehension_arithmeticException_runtimeError() throws Exception { - CelAbstractSyntaxTree ast = CEL_COMPILER.compile("[0].all(i, k, i/k < k)").getAst(); - CelEvaluationException e = - assertThrows(CelEvaluationException.class, () -> CEL_RUNTIME.createProgram(ast).eval()); - + assertThrows(CelEvaluationException.class, () -> eval("[0].all(i, k, i/k < k)")); assertThat(e).hasCauseThat().isInstanceOf(CelDivideByZeroException.class); assertThat(e).hasCauseThat().hasMessageThat().contains("/ by zero"); } @Test public void twoVarComprehension_outOfBounds_runtimeError() throws Exception { - CelAbstractSyntaxTree ast = CEL_COMPILER.compile("[1, 2].exists(i, v, [0][v] > 0)").getAst(); - CelEvaluationException e = - assertThrows(CelEvaluationException.class, () -> CEL_RUNTIME.createProgram(ast).eval()); - + assertThrows(CelEvaluationException.class, () -> eval("[1, 2].exists(i, v, [0][v] > 0)")); assertThat(e).hasCauseThat().isInstanceOf(CelIndexOutOfBoundsException.class); assertThat(e).hasCauseThat().hasMessageThat().contains("Index out of bounds: 1"); } + + @Test + public void mutableMapValue_select_missingKeyException() throws Exception { + CelEvaluationException e = + assertThrows( + CelEvaluationException.class, () -> eval("cel.bind(my_map, {'a': 1}, my_map.b)")); + assertThat(e).hasCauseThat().isInstanceOf(CelAttributeNotFoundException.class); + assertThat(e).hasCauseThat().hasMessageThat().contains("key 'b' is not present in map."); + } + + private Object eval(String expression) throws Exception { + return eval(this.cel, expression, ImmutableMap.of()); + } + + private Object eval(Cel cel, String expression, Map variables) throws Exception { + CelAbstractSyntaxTree ast; + if (isParseOnly) { + ast = cel.parse(expression).getAst(); + } else { + ast = cel.compile(expression).getAst(); + } + return cel.createProgram(ast).eval(variables); + } } diff --git a/extensions/src/test/java/dev/cel/extensions/CelListsExtensionsTest.java b/extensions/src/test/java/dev/cel/extensions/CelListsExtensionsTest.java index 2083ccc42..b36e0e92e 100644 --- a/extensions/src/test/java/dev/cel/extensions/CelListsExtensionsTest.java +++ b/extensions/src/test/java/dev/cel/extensions/CelListsExtensionsTest.java @@ -334,8 +334,6 @@ private static Cel setupEnv(CelBuilder celBuilder) { .build(); } - - private Object eval(Cel cel, String expr) throws Exception { return eval(cel, expr, ImmutableMap.of()); } diff --git a/runtime/src/main/java/dev/cel/runtime/planner/BUILD.bazel b/runtime/src/main/java/dev/cel/runtime/planner/BUILD.bazel index 824c918d8..96382b9a9 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/BUILD.bazel +++ b/runtime/src/main/java/dev/cel/runtime/planner/BUILD.bazel @@ -365,6 +365,7 @@ java_library( ":activation_wrapper", ":planned_interpretable", "//common/exceptions:runtime_exception", + "//common/values:mutable_map_value", "//runtime:accumulated_unknowns", "//runtime:concatenated_list_view", "//runtime:evaluation_exception", diff --git a/runtime/src/main/java/dev/cel/runtime/planner/EvalAnd.java b/runtime/src/main/java/dev/cel/runtime/planner/EvalAnd.java index eb7406071..91f5b2ff4 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/EvalAnd.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/EvalAnd.java @@ -38,7 +38,10 @@ public Object eval(GlobalResolver resolver, ExecutionFrame frame) { return false; } } else if (argVal instanceof ErrorValue) { - errorValue = (ErrorValue) argVal; + // Preserve the first encountered error instead of overwriting it with subsequent errors. + if (errorValue == null) { + errorValue = (ErrorValue) argVal; + } } else if (argVal instanceof AccumulatedUnknowns) { unknowns = AccumulatedUnknowns.maybeMerge(unknowns, argVal); } else { diff --git a/runtime/src/main/java/dev/cel/runtime/planner/EvalFold.java b/runtime/src/main/java/dev/cel/runtime/planner/EvalFold.java index 2eb30671e..090a8bfae 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/EvalFold.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/EvalFold.java @@ -15,8 +15,10 @@ package dev.cel.runtime.planner; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.errorprone.annotations.Immutable; import dev.cel.common.exceptions.CelRuntimeException; +import dev.cel.common.values.MutableMapValue; import dev.cel.runtime.AccumulatedUnknowns; import dev.cel.runtime.CelEvaluationException; import dev.cel.runtime.ConcatenatedListView; @@ -131,7 +133,7 @@ private Object evalList(Collection iterRange, Folder folder, ExecutionFrame f boolean cond = (boolean) condition.eval(folder, frame); if (!cond) { folder.computeResult = true; - return result.eval(folder, frame); + return maybeUnwrapAccumulator(result.eval(folder, frame)); } folder.accuVal = loopStep.eval(folder, frame); @@ -139,14 +141,16 @@ private Object evalList(Collection iterRange, Folder folder, ExecutionFrame f index++; } folder.computeResult = true; - return result.eval(folder, frame); + return maybeUnwrapAccumulator(result.eval(folder, frame)); } private static Object maybeWrapAccumulator(Object val) { if (val instanceof Collection) { return new ConcatenatedListView<>((Collection) val); } - // TODO: Introduce mutable map support (for comp v2) + if (val instanceof Map) { + return MutableMapValue.create((Map) val); + } return val; } @@ -154,8 +158,9 @@ private static Object maybeUnwrapAccumulator(Object val) { if (val instanceof ConcatenatedListView) { return ImmutableList.copyOf((ConcatenatedListView) val); } - - // TODO: Introduce mutable map support (for comp v2) + if (val instanceof MutableMapValue) { + return ImmutableMap.copyOf((MutableMapValue) val); + } return val; } diff --git a/runtime/src/main/java/dev/cel/runtime/planner/EvalOr.java b/runtime/src/main/java/dev/cel/runtime/planner/EvalOr.java index bc19ed81a..62e617d9d 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/EvalOr.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/EvalOr.java @@ -38,7 +38,10 @@ public Object eval(GlobalResolver resolver, ExecutionFrame frame) { return true; } } else if (argVal instanceof ErrorValue) { - errorValue = (ErrorValue) argVal; + // Preserve the first encountered error instead of overwriting it with subsequent errors. + if (errorValue == null) { + errorValue = (ErrorValue) argVal; + } } else if (argVal instanceof AccumulatedUnknowns) { unknowns = AccumulatedUnknowns.maybeMerge(unknowns, argVal); } else { diff --git a/runtime/src/test/resources/planner_unknownResultSet_errors.baseline b/runtime/src/test/resources/planner_unknownResultSet_errors.baseline index 812067ddf..7885e9da1 100644 --- a/runtime/src/test/resources/planner_unknownResultSet_errors.baseline +++ b/runtime/src/test/resources/planner_unknownResultSet_errors.baseline @@ -32,7 +32,7 @@ single_timestamp { seconds: 15 } , unknown_attributes=[x.single_int32]} -error: evaluation error at test_location:89: Text 'another bad timestamp string' could not be parsed at index 0 +error: evaluation error at test_location:31: Text 'bad timestamp string' could not be parsed at index 0 error_code: BAD_FORMAT Source: x.single_int32 == 1 || x.single_timestamp <= timestamp("bad timestamp string") @@ -69,7 +69,7 @@ single_timestamp { seconds: 15 } , unknown_attributes=[x.single_int32]} -error: evaluation error at test_location:89: Text 'another bad timestamp string' could not be parsed at index 0 +error: evaluation error at test_location:31: Text 'bad timestamp string' could not be parsed at index 0 error_code: BAD_FORMAT Source: x