From 56182b0a40428921d814265beb2e9b52c5ed722d Mon Sep 17 00:00:00 2001 From: Sean Huh Date: Tue, 27 Oct 2026 15:52:34 -0700 Subject: [PATCH] Support optional field selection in SelectOptimizer and the planner runtime PiperOrigin-RevId: 1008434172 --- .../values/OptimizedSelectTraversal.java | 26 +- .../values/OptimizedSelectTraversalTest.java | 27 +- .../optimizers/DefaultOptimizerConstants.java | 6 + .../optimizer/optimizers/SelectOptimizer.java | 198 +++++---- .../optimizers/SubexpressionOptimizer.java | 12 +- .../dev/cel/optimizer/optimizers/BUILD.bazel | 1 + .../optimizers/SelectOptimizerTest.java | 396 +++++++++++++++++- .../SubexpressionOptimizerTest.java | 6 +- .../planner/OptimizedSelectPlanner.java | 199 +++++++-- .../cel/runtime/planner/ProgramPlanner.java | 9 +- .../src/test/java/dev/cel/runtime/BUILD.bazel | 2 + .../CelLiteRuntimeVersionSkewTest.java | 70 +++- .../planner/OptimizedSelectPlannerTest.java | 276 ++++++++++-- 13 files changed, 994 insertions(+), 234 deletions(-) diff --git a/common/src/main/java/dev/cel/common/values/OptimizedSelectTraversal.java b/common/src/main/java/dev/cel/common/values/OptimizedSelectTraversal.java index ce35643dd..415ed702b 100644 --- a/common/src/main/java/dev/cel/common/values/OptimizedSelectTraversal.java +++ b/common/src/main/java/dev/cel/common/values/OptimizedSelectTraversal.java @@ -51,6 +51,23 @@ static Object qualify(Object target, ImmutableList fields, int from return current; } + /** + * Presence tests every field in {@code fields} in turn, returning the terminal value or empty if + * any field is absent. + */ + public static Optional find(Object target, ImmutableList fields) { + Optional current = Optional.empty(); + Object next = target; + for (int i = 0; i < fields.size(); i++) { + current = navigateField(next, fields.get(i)); + if (!current.isPresent()) { + return Optional.empty(); + } + next = current.get(); + } + return current; + } + /** * Presence tests the terminal field of {@code fields}, navigating through all preceding fields. */ @@ -77,9 +94,6 @@ public static boolean hasField(Object target, ImmutableList fields) // StructValue is only ever instantiated with String keys in the select path. @SuppressWarnings("unchecked") private static Object qualifyField(Object target, SelectField field) { - if (target instanceof ErrorValue) { - return target; - } if (target instanceof OptimizedSelectable) { return ((OptimizedSelectable) target).selectByFieldNumber(field); } @@ -99,9 +113,6 @@ private static Object qualifyField(Object target, SelectField field) { // StructValue is only ever instantiated with String keys in the select path. @SuppressWarnings("unchecked") private static Optional navigateField(Object target, SelectField field) { - if (target instanceof ErrorValue) { - return Optional.of(target); - } if (target instanceof OptimizedSelectable) { return ((OptimizedSelectable) target).findByFieldNumber(field); } @@ -114,9 +125,6 @@ private static Optional navigateField(Object target, SelectField field) // StructValue is only ever instantiated with String keys in the select path. @SuppressWarnings("unchecked") private static boolean hasTerminalField(Object target, SelectField field) { - if (target instanceof ErrorValue) { - return false; - } if (target instanceof OptimizedSelectable) { return ((OptimizedSelectable) target).hasFieldByNumber(field); } diff --git a/common/src/test/java/dev/cel/common/values/OptimizedSelectTraversalTest.java b/common/src/test/java/dev/cel/common/values/OptimizedSelectTraversalTest.java index af3cdbe18..070f6142a 100644 --- a/common/src/test/java/dev/cel/common/values/OptimizedSelectTraversalTest.java +++ b/common/src/test/java/dev/cel/common/values/OptimizedSelectTraversalTest.java @@ -271,25 +271,28 @@ public void hasField_unsupportedTarget_throwsException() { } @Test - public void qualify_errorValue_propagatesError() { - ErrorValue error = ErrorValue.create(1L, new RuntimeException("test error")); - ImmutableList fields = - ImmutableList.of(SelectField.create(1L, "field1"), SelectField.create(2L, "field2")); - - Object result = OptimizedSelectTraversal.qualify(error, fields); + public void find_emptyFields_returnsEmpty() { + Optional result = OptimizedSelectTraversal.find(new Object(), ImmutableList.of()); - assertThat(result).isSameInstanceAs(error); + assertThat(result).isEmpty(); } @Test - public void hasField_errorValue_returnsFalse() { - ErrorValue error = ErrorValue.create(1L, new RuntimeException("test error")); + public void find_nestedFields( + @TestParameter TargetType targetType, @TestParameter NestedPresenceTestCase testCase) { + Object target = targetType.createNestedTarget(testCase.innerData); ImmutableList fields = - ImmutableList.of(SelectField.create(1L, "field1"), SelectField.create(2L, "field2")); + ImmutableList.of( + SelectField.create(1L, testCase.outerField), + SelectField.create(2L, testCase.innerField)); - boolean result = OptimizedSelectTraversal.hasField(error, fields); + Optional result = OptimizedSelectTraversal.find(target, fields); - assertThat(result).isFalse(); + if (testCase.expected) { + assertThat(result).hasValue("nested_val"); + } else { + assertThat(result).isEmpty(); + } } @SuppressWarnings("Immutable") diff --git a/optimizer/src/main/java/dev/cel/optimizer/optimizers/DefaultOptimizerConstants.java b/optimizer/src/main/java/dev/cel/optimizer/optimizers/DefaultOptimizerConstants.java index 35fee23d4..1a15daa7e 100644 --- a/optimizer/src/main/java/dev/cel/optimizer/optimizers/DefaultOptimizerConstants.java +++ b/optimizer/src/main/java/dev/cel/optimizer/optimizers/DefaultOptimizerConstants.java @@ -47,7 +47,13 @@ final class DefaultOptimizerConstants { .build(); static final String CEL_ATTRIBUTE_FUNCTION_NAME = "cel.@attribute"; + static final String CEL_OPTIONAL_ATTRIBUTE_FUNCTION_NAME = "cel.@optionalAttribute"; static final String CEL_HAS_FIELD_FUNCTION_NAME = "cel.@hasField"; + static final ImmutableSet SELECT_OPTIMIZED_FUNCTION_NAMES = + ImmutableSet.of( + CEL_ATTRIBUTE_FUNCTION_NAME, + CEL_OPTIONAL_ATTRIBUTE_FUNCTION_NAME, + CEL_HAS_FIELD_FUNCTION_NAME); private DefaultOptimizerConstants() {} } diff --git a/optimizer/src/main/java/dev/cel/optimizer/optimizers/SelectOptimizer.java b/optimizer/src/main/java/dev/cel/optimizer/optimizers/SelectOptimizer.java index 79dfb8fed..32aa749cb 100644 --- a/optimizer/src/main/java/dev/cel/optimizer/optimizers/SelectOptimizer.java +++ b/optimizer/src/main/java/dev/cel/optimizer/optimizers/SelectOptimizer.java @@ -19,6 +19,7 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static dev.cel.optimizer.optimizers.DefaultOptimizerConstants.CEL_ATTRIBUTE_FUNCTION_NAME; import static dev.cel.optimizer.optimizers.DefaultOptimizerConstants.CEL_HAS_FIELD_FUNCTION_NAME; +import static dev.cel.optimizer.optimizers.DefaultOptimizerConstants.CEL_OPTIONAL_ATTRIBUTE_FUNCTION_NAME; import static java.lang.Math.max; import com.google.auto.value.AutoValue; @@ -52,7 +53,6 @@ import dev.cel.common.ast.CelMutableExpr.CelMutableCall; import dev.cel.common.ast.CelMutableExpr.CelMutableList; import dev.cel.common.ast.CelMutableExpr.CelMutableMap; -import dev.cel.common.ast.CelMutableExpr.CelMutableSelect; import dev.cel.common.ast.CelMutableExpr.CelMutableStruct; import dev.cel.common.internal.CelDescriptorPool; import dev.cel.common.internal.CombinedDescriptorPool; @@ -75,6 +75,7 @@ import java.util.Collections; import java.util.List; import java.util.Optional; +import java.util.OptionalInt; /** * Performs field selection optimization on protobuf message select chains. @@ -85,10 +86,10 @@ * provides resilience against protobuf field renames. * *

WARNING: Evaluating optimized ASTs requires explicit runtime support for {@code - * cel.@attribute} and {@code cel.@hasField}. Ensure that the target evaluation environment (in - * Java, C++, Go, or other language runtimes) supports these select optimization functions before - * applying this optimizer. Evaluating an optimized AST in an unsupported runtime will result in an - * evaluation error due to missing function overloads. + * cel.@attribute}, {@code cel.@optionalAttribute} and {@code cel.@hasField}. Ensure that the target + * evaluation environment (in Java, C++, Go, or other language runtimes) supports these select + * optimization functions before applying this optimizer. Evaluating an optimized AST in an + * unsupported runtime will result in an evaluation error due to missing function overloads. * *

Metadata Tuples: In {@code cel.@attribute}, each step in the qualification path is * represented as a metadata tuple: @@ -126,6 +127,12 @@ * // Constant list indices and map keys between fields are inlined as bare constants * request.users[0].age -> cel.@attribute(request, * [[users_num, "users", 11], 0, [age_num, "age", type_code]], 0) + * + * // Optional chains pass the first '?' hop index (or -1 for an optional operand) + * request.?user.age -> cel.@optionalAttribute(request, + * [[user_num, "user", type_code], [age_num, "age", type_code]], 0, 0) + * has(request.user.?address.city) -> cel.@hasField(request, + * [[user_num, "user"], [address_num, "address"], [city_num, "city"]], 1) * * *

Dynamic or trailing indices and non-protobuf selects pass through untouched. @@ -146,8 +153,6 @@ *

  • Oneofs: The lite runtime reads fields from the wire, so it reports every serialized * member of a oneof as present. Protobuf serializes at most one, unless an older-schema * binary merges or mutates the message, or the bytes are concatenated or altered by hand. - *
  • Optionals: Optional field selection on a message ({@code msg.?field}, or a field of - * an optional message) is not yet supported and throws during optimization. * */ public final class SelectOptimizer implements CelAstOptimizer { @@ -162,8 +167,16 @@ public final class SelectOptimizer implements CelAstOptimizer { private static final TypeParamType TYPE_PARAM_T = TypeParamType.create("T"); - private static final ImmutableSet> STANDARD_INDEX_OVERLOADS = - ImmutableSet.of(ImmutableList.of("index_list"), ImmutableList.of("index_map")); + private static final ImmutableSet> INLINED_INDEX_OVERLOADS = + ImmutableSet.of( + ImmutableList.of("index_list"), + ImmutableList.of("index_map"), + ImmutableList.of("optional_list_index_int"), + ImmutableList.of("optional_map_index_value"), + ImmutableList.of("list_optindex_optional_int"), + ImmutableList.of("optional_list_optindex_optional_int"), + ImmutableList.of("map_optindex_optional_value"), + ImmutableList.of("optional_map_optindex_optional_value")); /** * Declaration for {@code cel.@attribute(operand, qualifiers, dummyOrDefVal) -> T}. @@ -183,6 +196,22 @@ public final class SelectOptimizer implements CelAstOptimizer { ListType.create(SimpleType.DYN), TYPE_PARAM_T)); + /** + * Declaration for {@code cel.@optionalAttribute(operand, qualifiers, opt_index, dummy) -> + * optional_type(T)}. + */ + @VisibleForTesting + static final CelFunctionDecl CEL_OPTIONAL_ATTRIBUTE_FUNCTION_DECL = + CelFunctionDecl.newFunctionDeclaration( + CEL_OPTIONAL_ATTRIBUTE_FUNCTION_NAME, + CelOverloadDecl.newGlobalOverload( + "cel_optional_attribute_list_int", + OptionalType.create(TYPE_PARAM_T), + SimpleType.DYN, + ListType.create(SimpleType.DYN), + SimpleType.INT, + TYPE_PARAM_T)); + @VisibleForTesting static final CelFunctionDecl CEL_HAS_FIELD_FUNCTION_DECL = CelFunctionDecl.newFunctionDeclaration( @@ -191,7 +220,13 @@ public final class SelectOptimizer implements CelAstOptimizer { "cel_has_field_list", SimpleType.BOOL, SimpleType.DYN, - ListType.create(SimpleType.DYN))); + ListType.create(SimpleType.DYN)), + CelOverloadDecl.newGlobalOverload( + "cel_has_field_list_int", + SimpleType.BOOL, + SimpleType.DYN, + ListType.create(SimpleType.DYN), + SimpleType.INT)); @VisibleForTesting static final Extension SELECT_OPTIMIZATION_AST_EXTENSION_TAG = @@ -233,7 +268,6 @@ public OptimizationResult optimize(CelAbstractSyntaxTree ast, Cel cel) { CelMutableAst astToModify = CelMutableAst.fromCelAst(ast); CelNavigableMutableAst navAst = CelNavigableMutableAst.fromAst(astToModify); - navAst.getRoot().allNodes().forEach(node -> checkNotOptionalFieldSelection(navAst, node)); ImmutableList topOfChainSelects = navAst .getRoot() @@ -267,7 +301,10 @@ public OptimizationResult optimize(CelAbstractSyntaxTree ast, Cel cel) { return OptimizationResult.create( optimizedAst, ImmutableList.of(), - ImmutableList.of(CEL_ATTRIBUTE_FUNCTION_DECL, CEL_HAS_FIELD_FUNCTION_DECL)); + ImmutableList.of( + CEL_ATTRIBUTE_FUNCTION_DECL, + CEL_OPTIONAL_ATTRIBUTE_FUNCTION_DECL, + CEL_HAS_FIELD_FUNCTION_DECL)); } private void rewriteSelectChain( @@ -275,29 +312,28 @@ private void rewriteSelectChain( CelNavigableMutableAst navAst, CelNavigableMutableExpr topNode, MonotonicIdGenerator idGenerator) { - boolean isHasField = topNode.expr().select().testOnly(); - astToModify.source().getMacroCalls().remove(topNode.expr().id()); + CelMutableExpr topExpr = topNode.expr(); + boolean isHasField = topExpr.getKind() == Kind.SELECT && topExpr.select().testOnly(); + astToModify.source().getMacroCalls().remove(topExpr.id()); FieldDescriptor topField = - getOptimizableField(navAst, topNode) - .orElseThrow( - () -> new IllegalStateException("Expected optimizable field on select node")); + getOptimizableField(navAst, topExpr).orElseThrow(IllegalStateException::new); // Walks from the leaf. Fields before an index are selected by value, so they carry their type // codes even in presence tests. boolean isSelectedByValue = !isHasField; List qualifierLists = new ArrayList<>(); - qualifierLists.add(newFieldQualifier(topField, isSelectedByValue, idGenerator)); - - CelMutableExpr currentExpr = topNode.expr().select().operand(); + // Counted from the leaf; an optional root operand records qualifierLists.size(), yielding -1. + OptionalInt optionalHopFromLeaf = OptionalInt.empty(); + CelMutableExpr currentExpr = topExpr; while (true) { - if (currentExpr.getKind() == Kind.SELECT) { - FieldDescriptor field = - getOptimizableFieldForExpr(navAst, currentExpr.select()).orElse(null); - if (field != null) { - qualifierLists.add(newFieldQualifier(field, isSelectedByValue, idGenerator)); - currentExpr = currentExpr.select().operand(); - continue; - } + if (navAst.getType(currentExpr.id()).filter(OptionalType.class::isInstance).isPresent()) { + optionalHopFromLeaf = OptionalInt.of(qualifierLists.size()); + } + FieldDescriptor field = getOptimizableField(navAst, currentExpr).orElse(null); + if (field != null) { + qualifierLists.add(newFieldQualifier(field, isSelectedByValue, idGenerator)); + currentExpr = fieldOperand(currentExpr); + continue; } CelMutableExpr indexedField = getIndexedField(navAst, currentExpr).orElse(null); if (indexedField == null) { @@ -309,23 +345,25 @@ private void rewriteSelectChain( } Collections.reverse(qualifierLists); - CelMutableExpr qualifiersExpr = - CelMutableExpr.ofList(idGenerator.nextExprId(), CelMutableList.create(qualifierLists)); - if (isHasField) { - topNode - .expr() - .setCall(CelMutableCall.create(CEL_HAS_FIELD_FUNCTION_NAME, currentExpr, qualifiersExpr)); - } else { - CelMutableExpr dummyOrDefExpr = resolveDummyOrDefExpr(topField, idGenerator); - topNode - .expr() - .setCall( - CelMutableCall.create( - CEL_ATTRIBUTE_FUNCTION_NAME, currentExpr, qualifiersExpr, dummyOrDefExpr)); + List args = new ArrayList<>(); + args.add(currentExpr); + args.add( + CelMutableExpr.ofList(idGenerator.nextExprId(), CelMutableList.create(qualifierLists))); + boolean isOptional = optionalHopFromLeaf.isPresent(); + if (isOptional) { + long optIndex = qualifierLists.size() - 1L - optionalHopFromLeaf.getAsInt(); + args.add(CelMutableExpr.ofConstant(idGenerator.nextExprId(), CelConstant.ofValue(optIndex))); } + if (!isHasField) { + args.add(resolveDummyOrDefExpr(topField, idGenerator)); + } + String function = + isHasField + ? CEL_HAS_FIELD_FUNCTION_NAME + : isOptional ? CEL_OPTIONAL_ATTRIBUTE_FUNCTION_NAME : CEL_ATTRIBUTE_FUNCTION_NAME; + topExpr.setCall(CelMutableCall.create(function, args)); } - /** Returns the qualifier hop for {@code field}, with its type codes if {@code withTypeCode}. */ private static CelMutableExpr newFieldQualifier( FieldDescriptor field, boolean withTypeCode, MonotonicIdGenerator idGenerator) { if (field.getType() == FieldDescriptor.Type.GROUP) { @@ -374,7 +412,7 @@ private static long resolveTypeCode(FieldDescriptor field) { } private boolean isTopOfSelectChain(CelNavigableMutableAst navAst, CelNavigableMutableExpr node) { - if (!getOptimizableField(navAst, node).isPresent()) { + if (!getOptimizableField(navAst, node.expr()).isPresent()) { return false; } Optional parent = node.parent(); @@ -382,80 +420,54 @@ private boolean isTopOfSelectChain(CelNavigableMutableAst navAst, CelNavigableMu if (parent.isPresent() && getIndexedField(navAst, parent.get().expr()).isPresent()) { parent = parent.get().parent(); } - return !parent.flatMap(p -> getOptimizableField(navAst, p)).isPresent(); + return !parent.flatMap(p -> getOptimizableField(navAst, p.expr())).isPresent(); } - /** - * Returns the protobuf field select that {@code expr} indexes by a constant through a standard - * {@code _[_]} overload, if the field is optimizable. - */ private Optional getIndexedField( CelNavigableMutableAst navAst, CelMutableExpr expr) { // `field.key` on a map isn't inlined: its missing-key error differs from `field['key']`'s. - boolean isStandardIndex = + boolean isInlinedIndex = navAst .getAst() .getReference(expr.id()) - .map(reference -> STANDARD_INDEX_OVERLOADS.contains(reference.overloadIds())) + .map(reference -> INLINED_INDEX_OVERLOADS.contains(reference.overloadIds())) .orElse(false); - if (!isStandardIndex || expr.call().args().get(1).getKind() != Kind.CONSTANT) { + if (!isInlinedIndex || expr.call().args().get(1).getKind() != Kind.CONSTANT) { return Optional.empty(); } CelMutableExpr fieldExpr = expr.call().args().get(0); - if (fieldExpr.getKind() != Kind.SELECT - || !getOptimizableFieldForExpr(navAst, fieldExpr.select()).isPresent()) { - return Optional.empty(); - } - return Optional.of(fieldExpr); + return getOptimizableField(navAst, fieldExpr).map(unused -> fieldExpr); } private Optional getOptimizableField( - CelNavigableMutableAst navAst, CelNavigableMutableExpr node) { - if (node.getKind() != Kind.SELECT) { + CelNavigableMutableAst navAst, CelMutableExpr expr) { + String field; + if (expr.getKind() == Kind.SELECT) { + field = expr.select().field(); + } else if (expr.getKind() == Kind.CALL + && expr.call().function().equals(Operator.OPTIONAL_SELECT.getFunction())) { + field = expr.call().args().get(1).constant().stringValue(); + } else { return Optional.empty(); } - return getOptimizableFieldForExpr(navAst, node.expr().select()); - } - - private Optional getOptimizableFieldForExpr( - CelNavigableMutableAst navAst, CelMutableSelect select) { - return navAst.getType(select.operand().id()).flatMap(type -> findField(type, select.field())); + return navAst.getType(fieldOperand(expr).id()).flatMap(type -> findField(type, field)); } private Optional findField(CelType type, String field) { - if (type.kind() != CelKind.STRUCT) { + // A field of an optional message is selected from the message, if present. + CelType structType = type instanceof OptionalType ? type.parameters().get(0) : type; + if (structType.kind() != CelKind.STRUCT) { return Optional.empty(); } - return descriptorPool.findDescriptor(type.name()).map(desc -> desc.findFieldByName(field)); + return descriptorPool + .findDescriptor(structType.name()) + .map(desc -> desc.findFieldByName(field)); } - // TODO: Support optional field selection once integrated with lite runtime. - private void checkNotOptionalFieldSelection( - CelNavigableMutableAst navAst, CelNavigableMutableExpr node) { - Optional field = Optional.empty(); - if (node.getKind() == Kind.SELECT) { - // A field of an optional message, e.g. `.bb` in `msg.repeated_nested_message[?0].bb` - CelMutableSelect select = node.expr().select(); - field = - navAst - .getType(select.operand().id()) - .filter(OptionalType.class::isInstance) - .flatMap(type -> findField(type.parameters().get(0), select.field())); - } else if (node.getKind() == Kind.CALL - && node.expr().call().function().equals(Operator.OPTIONAL_SELECT.getFunction())) { - // `msg.?field`, where `msg` may itself be optional - List args = node.expr().call().args(); - field = - navAst - .getType(args.get(0).id()) - .map(type -> type instanceof OptionalType ? type.parameters().get(0) : type) - .flatMap(type -> findField(type, args.get(1).constant().stringValue())); - } - if (field.isPresent()) { - throw new UnsupportedOperationException( - "Optimization of optional field selection is currently unimplemented: " - + field.get().getFullName()); - } + private static CelMutableExpr fieldOperand(CelMutableExpr fieldExpr) { + return fieldExpr.getKind() == Kind.SELECT + ? fieldExpr.select().operand() + : fieldExpr.call().args().get(0); } private static CelMutableExpr resolveDummyOrDefExpr( diff --git a/optimizer/src/main/java/dev/cel/optimizer/optimizers/SubexpressionOptimizer.java b/optimizer/src/main/java/dev/cel/optimizer/optimizers/SubexpressionOptimizer.java index ae08f7146..13c8f35f7 100644 --- a/optimizer/src/main/java/dev/cel/optimizer/optimizers/SubexpressionOptimizer.java +++ b/optimizer/src/main/java/dev/cel/optimizer/optimizers/SubexpressionOptimizer.java @@ -18,8 +18,7 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableSet.toImmutableSet; -import static dev.cel.optimizer.optimizers.DefaultOptimizerConstants.CEL_ATTRIBUTE_FUNCTION_NAME; -import static dev.cel.optimizer.optimizers.DefaultOptimizerConstants.CEL_HAS_FIELD_FUNCTION_NAME; +import static dev.cel.optimizer.optimizers.DefaultOptimizerConstants.SELECT_OPTIMIZED_FUNCTION_NAMES; import static java.lang.Math.max; import com.google.auto.value.AutoValue; @@ -146,8 +145,8 @@ public static SubexpressionOptimizer newInstance(SubexpressionOptimizerOptions c * {@inheritDoc} * * @throws IllegalStateException if {@code ast} contains select-optimized nodes ({@code - * cel.@attribute} or {@code cel.@hasField}), as common subexpression elimination must run - * before {@link SelectOptimizer}. + * cel.@attribute}, {@code cel.@optionalAttribute} or {@code cel.@hasField}), as common + * subexpression elimination must run before {@link SelectOptimizer}. */ @Override public OptimizationResult optimize(CelAbstractSyntaxTree ast, Cel cel) { @@ -164,10 +163,7 @@ private OptimizationResult optimizeUsingCelBlock(CelAbstractSyntaxTree ast, Cel .allNodes() .filter(node -> node.getKind() == Kind.CALL) .map(node -> node.expr().call().function()) - .anyMatch( - func -> - func.equals(CEL_ATTRIBUTE_FUNCTION_NAME) - || func.equals(CEL_HAS_FIELD_FUNCTION_NAME)); + .anyMatch(SELECT_OPTIMIZED_FUNCTION_NAMES::contains); checkState(!hasSelectOptimizedNodes, "SubexpressionOptimizer must run before SelectOptimizer."); CelMutableAst astToModify = CelMutableAst.fromCelAst(ast); diff --git a/optimizer/src/test/java/dev/cel/optimizer/optimizers/BUILD.bazel b/optimizer/src/test/java/dev/cel/optimizer/optimizers/BUILD.bazel index 13d8e1e46..4b8a40072 100644 --- a/optimizer/src/test/java/dev/cel/optimizer/optimizers/BUILD.bazel +++ b/optimizer/src/test/java/dev/cel/optimizer/optimizers/BUILD.bazel @@ -22,6 +22,7 @@ java_library( "//common/ast", "//common/navigation:mutable_navigation", "//common/types", + "//common/types:cel_types", "//common/values", "//common/values:cel_byte_string", "//extensions", diff --git a/optimizer/src/test/java/dev/cel/optimizer/optimizers/SelectOptimizerTest.java b/optimizer/src/test/java/dev/cel/optimizer/optimizers/SelectOptimizerTest.java index 10c2fad43..683b3e9af 100644 --- a/optimizer/src/test/java/dev/cel/optimizer/optimizers/SelectOptimizerTest.java +++ b/optimizer/src/test/java/dev/cel/optimizer/optimizers/SelectOptimizerTest.java @@ -30,6 +30,7 @@ import com.google.protobuf.Descriptors.Descriptor; import com.google.protobuf.Descriptors.FileDescriptor; import com.google.protobuf.Int64Value; +import com.google.protobuf.Message; import com.google.protobuf.Struct; import com.google.protobuf.TextFormat; import com.google.protobuf.Value; @@ -49,8 +50,10 @@ import dev.cel.common.ast.CelExpr.ExprKind.Kind; import dev.cel.common.ast.CelReference; import dev.cel.common.navigation.CelNavigableMutableAst; +import dev.cel.common.types.CelTypes; import dev.cel.common.types.ListType; import dev.cel.common.types.MapType; +import dev.cel.common.types.OptionalType; import dev.cel.common.types.SimpleType; import dev.cel.common.types.StructTypeReference; import dev.cel.common.values.CelByteString; @@ -75,6 +78,7 @@ import dev.cel.testing.CelRuntimeFlavor; import java.time.Duration; import java.time.Instant; +import java.util.Optional; import java.util.stream.LongStream; import org.junit.Before; import org.junit.Test; @@ -94,6 +98,35 @@ public final class SelectOptimizerTest { private static final Descriptor PROTO2_TEST_ALL_TYPES_DESCRIPTOR = checkNotNull(TestAllTypesProto.getDescriptor().findMessageTypeByName("TestAllTypes")); + private static final dev.cel.expr.conformance.proto3.NestedTestAllTypes POPULATED_PROTO3_NESTED = + newProto3Nested() + .setChild( + newProto3Nested() + .setPayload( + TestAllTypes.newBuilder() + .setSingleString("child") + .setSingleInt32(3) + .setSingleInt64(4) + .addRepeatedNestedMessage( + TestAllTypes.NestedMessage.newBuilder().setBb(5)))) + .setPayload( + TestAllTypes.newBuilder() + .setSingleString("payload") + .setSingleInt64(6) + .addRepeatedNestedMessage(TestAllTypes.NestedMessage.newBuilder().setBb(7)) + .putMapStringString("foo", "bar")) + .build(); + + private static final dev.cel.expr.conformance.proto3.NestedTestAllTypes STRICT_PREFIX_ENTRY = + newProto3Nested() + .setPayload( + TestAllTypes.newBuilder() + .addRepeatedNestedMessage(TestAllTypes.NestedMessage.getDefaultInstance())) + .build(); + + private static final StructTypeReference PROTO3_NESTED_TYPE = + StructTypeReference.create(newProto3Nested().getDescriptorForType().getFullName()); + @TestParameter CelRuntimeFlavor runtimeFlavor; private Cel cel; @@ -135,6 +168,10 @@ private static Cel setupEnv(CelBuilder celBuilder) { SimpleType.STRING, StructTypeReference.create(TestAllTypes.getDescriptor().getFullName()))) .addVar("x", SimpleType.INT) + .addVar("nested", PROTO3_NESTED_TYPE) + .addVar("opt_nested", OptionalType.create(PROTO3_NESTED_TYPE)) + .addCompilerLibraries(CelExtensions.optional()) + .addRuntimeLibraries(CelExtensions.optional()) .build(); } @@ -1010,9 +1047,9 @@ private enum NativeSelectEvaluationTestCase { TestAllTypes.newBuilder() .putMapInt64NestedType( 1, - dev.cel.expr.conformance.proto3.NestedTestAllTypes.newBuilder() + newProto3Nested() .setChild( - dev.cel.expr.conformance.proto3.NestedTestAllTypes.newBuilder() + newProto3Nested() .setPayload(newTestAllTypesWithRepeatedNestedMessages(10, 20))) .build()) .build()), @@ -1036,9 +1073,9 @@ private enum NativeSelectEvaluationTestCase { TestAllTypes.newBuilder() .putMapInt64NestedType( 1, - dev.cel.expr.conformance.proto3.NestedTestAllTypes.newBuilder() + newProto3Nested() .setChild( - dev.cel.expr.conformance.proto3.NestedTestAllTypes.newBuilder() + newProto3Nested() .setPayload(newTestAllTypesWithRepeatedNestedMessages(10, 20))) .build()) .build()), @@ -1604,7 +1641,7 @@ public void optimize_wellKnownTypeFieldSelect_preservesResultType( } @Test - public void optimize_resultFunctionDeclarations_containsOnlySingularAttributeAndHasField() + public void optimize_resultFunctionDeclarations_containsSelectOptimizationFunctions() throws Exception { SelectOptimizer optimizer = SelectOptimizer.newInstance(TestAllTypes.getDescriptor().getFile()); CelAbstractSyntaxTree ast = cel.compile("msg.single_int64").getAst(); @@ -1614,11 +1651,20 @@ public void optimize_resultFunctionDeclarations_containsOnlySingularAttributeAnd assertThat(result.newFunctionDecls()) .containsExactly( SelectOptimizer.CEL_ATTRIBUTE_FUNCTION_DECL, + SelectOptimizer.CEL_OPTIONAL_ATTRIBUTE_FUNCTION_DECL, SelectOptimizer.CEL_HAS_FIELD_FUNCTION_DECL); assertThat( SelectOptimizer.CEL_ATTRIBUTE_FUNCTION_DECL.overloads().stream() .map(CelOverloadDecl::overloadId)) .containsExactly("cel_attribute_list"); + assertThat( + SelectOptimizer.CEL_OPTIONAL_ATTRIBUTE_FUNCTION_DECL.overloads().stream() + .map(CelOverloadDecl::overloadId)) + .containsExactly("cel_optional_attribute_list_int"); + assertThat( + SelectOptimizer.CEL_HAS_FIELD_FUNCTION_DECL.overloads().stream() + .map(CelOverloadDecl::overloadId)) + .containsExactly("cel_has_field_list", "cel_has_field_list_int"); } @Test @@ -1693,38 +1739,337 @@ public void optimize_comprehensionOverRepeatedMessageField_preservesStaticElemen assertThat(optimizedAst.getTypeMap().values()).doesNotContain(SimpleType.DYN); } + // Each level of the optional chains under test in turn unset, set to its default, and populated. + // Strictly selected prefixes (map key 1 and its payload's first element) are present in all. + @SuppressWarnings("ImmutableEnumChecker") // Test only + private enum OptionalSelectInput { + UNSET( + TestAllTypes.newBuilder().putMapInt64NestedType(1, STRICT_PREFIX_ENTRY), + newProto3Nested(), + Optional.empty()), + DEFAULT( + TestAllTypes.newBuilder() + .setStandaloneMessage(TestAllTypes.NestedMessage.getDefaultInstance()) + .addRepeatedInt64(0) + .addRepeatedNestedMessage(TestAllTypes.NestedMessage.getDefaultInstance()) + .putMapInt64NestedType(1, STRICT_PREFIX_ENTRY) + .putMapInt64NestedType(7, newProto3Nested().build()) + .putMapStringMessage("k", TestAllTypes.NestedMessage.getDefaultInstance()) + .setSingleStruct(Struct.getDefaultInstance()) + .setSingleInt64Wrapper(Int64Value.getDefaultInstance()) + .setOneofType(newProto3Nested()), + newProto3Nested().setChild(newProto3Nested()).setPayload(TestAllTypes.getDefaultInstance()), + Optional.of(newProto3Nested().build())), + POPULATED( + TestAllTypes.newBuilder() + .setSingleInt32(1) + .setStandaloneMessage(TestAllTypes.NestedMessage.newBuilder().setBb(2)) + .addRepeatedInt64(3) + .addRepeatedNestedMessage(TestAllTypes.NestedMessage.newBuilder().setBb(4)) + .putMapInt64NestedType(1, POPULATED_PROTO3_NESTED) + .putMapInt64NestedType(7, POPULATED_PROTO3_NESTED) + .putMapStringMessage("k", TestAllTypes.NestedMessage.newBuilder().setBb(8).build()) + .setSingleStruct( + Struct.newBuilder() + .putFields("foo", Value.newBuilder().setStringValue("x").build())) + .setSingleInt64Wrapper(Int64Value.of(9)) + .setOneofType(POPULATED_PROTO3_NESTED), + POPULATED_PROTO3_NESTED.toBuilder(), + Optional.of(POPULATED_PROTO3_NESTED)); + + private final ImmutableMap activation; + + OptionalSelectInput(TestAllTypes.Builder msg, Message.Builder nested, Optional optNested) { + this.activation = + ImmutableMap.of("msg", msg.build(), "nested", nested.build(), "opt_nested", optNested); + } + } + + private enum OptionalRewriteTestCase { + SCALAR_LEAF( + "msg.?single_int32", "cel.@optionalAttribute(msg, [[1, \"single_int32\", 5]], 0, 0)"), + FIRST_HOP_OPTIONAL( + "nested.?child.payload.single_string", + "cel.@optionalAttribute(nested, [[1, \"child\", 11], [2, \"payload\", 11]," + + " [14, \"single_string\", 9]], 0, \"\")"), + MIDDLE_HOP_OPTIONAL( + "nested.child.?payload.single_string", + "cel.@optionalAttribute(nested, [[1, \"child\", 11], [2, \"payload\", 11]," + + " [14, \"single_string\", 9]], 1, \"\")"), + LEAF_HOP_OPTIONAL( + "nested.child.payload.?single_string", + "cel.@optionalAttribute(nested, [[1, \"child\", 11], [2, \"payload\", 11]," + + " [14, \"single_string\", 9]], 2, \"\")"), + LATER_OPTIONAL_HOPS_NOT_RECORDED( + "nested.child.?payload.?single_string", + "cel.@optionalAttribute(nested, [[1, \"child\", 11], [2, \"payload\", 11]," + + " [14, \"single_string\", 9]], 1, \"\")"), + MESSAGE_LEAF( + "msg.?standalone_message", + "cel.@optionalAttribute(msg, [[23, \"standalone_message\", 11]], 0," + + " cel.expr.conformance.proto3.TestAllTypes.NestedMessage{})"), + REPEATED_LEAF( + "msg.?repeated_int64", + "cel.@optionalAttribute(msg, [[32, \"repeated_int64\", 3]], 0, [0])"), + MAP_LEAF( + "nested.payload.?map_string_string", + "cel.@optionalAttribute(nested, [[2, \"payload\", 11]," + + " [61, \"map_string_string\", -1, [9, 9]]], 1, {\"\": \"\"})"), + OPTIONAL_LIST_INDEX( + "msg.repeated_nested_message[?0].bb", + "cel.@optionalAttribute(msg, [[51, \"repeated_nested_message\", 11], 0, [1, \"bb\", 5]]," + + " 1, 0)"), + INDEX_AFTER_OPTIONAL_HOP( + "nested.?payload.repeated_nested_message[0].bb", + "cel.@optionalAttribute(nested, [[2, \"payload\", 11]," + + " [51, \"repeated_nested_message\", 11], 0, [1, \"bb\", 5]], 0, 0)"), + OPTIONAL_INT_MAP_KEY( + "msg.map_int64_nested_type[?7].payload.single_int32", + "cel.@optionalAttribute(msg, [[62, \"map_int64_nested_type\", -1, [3, 11]], 7," + + " [2, \"payload\", 11], [1, \"single_int32\", 5]], 1, 0)"), + OPTIONAL_STRING_MAP_KEY( + "msg.map_string_message[?'k'].bb", + "cel.@optionalAttribute(msg, [[227, \"map_string_message\", -1, [9, 11]], \"k\"," + + " [1, \"bb\", 5]], 1, 0)"), + STRICT_MAP_KEY_BEFORE_OPTIONAL_HOP( + "msg.map_int64_nested_type[1].?child.payload.repeated_nested_message[0].bb", + "cel.@optionalAttribute(msg, [[62, \"map_int64_nested_type\", -1, [3, 11]], 1," + + " [1, \"child\", 11], [2, \"payload\", 11], [51, \"repeated_nested_message\", 11]," + + " 0, [1, \"bb\", 5]], 2, 0)"), + STRICT_LIST_INDEX_BEFORE_OPTIONAL_LEAF( + "msg.map_int64_nested_type[1].payload.repeated_nested_message[0].?bb", + "cel.@optionalAttribute(msg, [[62, \"map_int64_nested_type\", -1, [3, 11]], 1," + + " [2, \"payload\", 11], [51, \"repeated_nested_message\", 11], 0," + + " [1, \"bb\", 5]], 5, 0)"), + STRICT_MAP_KEY_BEFORE_OPTIONAL_INDEX( + "msg.map_int64_nested_type[1].payload.repeated_nested_message[?0].bb", + "cel.@optionalAttribute(msg, [[62, \"map_int64_nested_type\", -1, [3, 11]], 1," + + " [2, \"payload\", 11], [51, \"repeated_nested_message\", 11], 0, [1, \"bb\", 5]]," + + " 4, 0)"), + OPTIONAL_OPERAND( + "opt_nested.child.payload.single_int64", + "cel.@optionalAttribute(opt_nested, [[1, \"child\", 11], [2, \"payload\", 11]," + + " [2, \"single_int64\", 3]], -1, 0)"), + OPTIONAL_OPERAND_WITH_OPTIONAL_HOP( + "opt_nested.child.?payload.single_int64", + "cel.@optionalAttribute(opt_nested, [[1, \"child\", 11], [2, \"payload\", 11]," + + " [2, \"single_int64\", 3]], -1, 0)"), + OPTIONAL_OPERAND_WITH_INDEX( + "opt_nested.payload.repeated_nested_message[0].bb", + "cel.@optionalAttribute(opt_nested, [[2, \"payload\", 11]," + + " [51, \"repeated_nested_message\", 11], 0, [1, \"bb\", 5]], -1, 0)"), + OPTIONAL_CALL_OPERAND( + "optional.of(nested).payload.single_int64", + "cel.@optionalAttribute(optional.of(nested), [[2, \"payload\", 11]," + + " [2, \"single_int64\", 3]], -1, 0)"), + TRAILING_INDEX_ON_OPTIONAL_CHAIN( + "msg.?repeated_int64[0]", + "cel.@optionalAttribute(msg, [[32, \"repeated_int64\", 3]], 0, [0])[0]"), + TRAILING_OPTIONAL_INDEX_ON_STRICT_CHAIN( + "msg.repeated_int64[?0]", "cel.@attribute(msg, [[32, \"repeated_int64\", 3]], [0])[?0]"), + OPTIONAL_MAP_KEY_SELECT_NOT_INLINED( + "msg.map_string_string.?key", + "cel.@attribute(msg, [[61, \"map_string_string\", -1, [9, 9]]], {\"\": \"\"}).?key"), + MAP_KEY_SELECT_NOT_INLINED( + "nested.?payload.map_string_string.foo", + "cel.@optionalAttribute(nested, [[2, \"payload\", 11]," + + " [61, \"map_string_string\", -1, [9, 9]]], 0, {\"\": \"\"}).foo"), + STRUCT_ENDS_CHAIN( + "msg.?single_struct.foo", + "cel.@optionalAttribute(msg, [[103, \"single_struct\", 11]], 0," + + " google.protobuf.Struct{}).foo"), + HAS_VALUE_NOT_INLINED( + "msg.?standalone_message.hasValue()", + "cel.@optionalAttribute(msg, [[23, \"standalone_message\", 11]], 0," + + " cel.expr.conformance.proto3.TestAllTypes.NestedMessage{}).hasValue()"), + WRAPPER_LEAF( + "msg.?single_int64_wrapper", + "cel.@optionalAttribute(msg, [[105, \"single_int64_wrapper\", 11]], 0," + + " google.protobuf.Int64Value{})"), + ONEOF_MESSAGE_LEAF( + "msg.oneof_type.?child", + "cel.@optionalAttribute(msg, [[400, \"oneof_type\", 11], [1, \"child\", 11]], 1," + + " cel.expr.conformance.proto3.NestedTestAllTypes{})"), + NEGATIVE_OPTIONAL_INDEX( + "msg.repeated_nested_message[?-1].bb", + "cel.@optionalAttribute(msg, [[51, \"repeated_nested_message\", 11], -1," + + " [1, \"bb\", 5]], 1, 0)"), + OVERSIZED_OPTIONAL_INDEX( + "msg.repeated_nested_message[?4294967296].bb", + "cel.@optionalAttribute(msg, [[51, \"repeated_nested_message\", 11], 4294967296," + + " [1, \"bb\", 5]], 1, 0)"), + INDEX_INTO_OPTIONAL_MAP( + "msg.?map_int64_nested_type[1].payload.single_int32", + "cel.@optionalAttribute(msg, [[62, \"map_int64_nested_type\", -1, [3, 11]], 1," + + " [2, \"payload\", 11], [1, \"single_int32\", 5]], 0, 0)"), + OPTIONAL_INDEX_INTO_OPTIONAL_LIST( + "msg.?repeated_nested_message[?0].bb", + "cel.@optionalAttribute(msg, [[51, \"repeated_nested_message\", 11], 0," + + " [1, \"bb\", 5]], 0, 0)"), + OPTIONAL_INDEX_INTO_OPTIONAL_MAP( + "msg.?map_int64_nested_type[?1].payload.single_int32", + "cel.@optionalAttribute(msg, [[62, \"map_int64_nested_type\", -1, [3, 11]], 1," + + " [2, \"payload\", 11], [1, \"single_int32\", 5]], 0, 0)"), + OR_VALUE_NOT_INLINED( + "nested.?child.payload.single_int64.orValue(-1)", + "cel.@optionalAttribute(nested, [[1, \"child\", 11], [2, \"payload\", 11]," + + " [2, \"single_int64\", 3]], 0, 0).orValue(-1)"), + OR_NOT_INLINED( + "nested.?child.or(optional.of(nested)).payload.single_int64", + "cel.@optionalAttribute(cel.@optionalAttribute(nested, [[1, \"child\", 11]], 0," + + " cel.expr.conformance.proto3.NestedTestAllTypes{}).or(optional.of(nested))," + + " [[2, \"payload\", 11], [2, \"single_int64\", 3]], -1, 0)"), + HAS_FIELD_FIRST_HOP_OPTIONAL( + "has(nested.?child.payload.single_int32)", + "cel.@hasField(nested, [[1, \"child\"], [2, \"payload\"], [1, \"single_int32\"]], 0)"), + HAS_FIELD_MIDDLE_HOP_OPTIONAL( + "has(nested.child.?payload.single_int32)", + "cel.@hasField(nested, [[1, \"child\"], [2, \"payload\"], [1, \"single_int32\"]], 1)"), + HAS_FIELD_OPTIONAL_INDEX( + "has(msg.repeated_nested_message[?0].bb)", + "cel.@hasField(msg, [[51, \"repeated_nested_message\", 11], 0, [1, \"bb\"]], 1)"), + HAS_FIELD_INDEX_AFTER_OPTIONAL_HOP( + "has(nested.?payload.repeated_nested_message[0].bb)", + "cel.@hasField(nested, [[2, \"payload\", 11], [51, \"repeated_nested_message\", 11], 0," + + " [1, \"bb\"]], 0)"), + HAS_FIELD_STRICT_INDEX_BEFORE_OPTIONAL_HOP( + "has(msg.map_int64_nested_type[1].?child.payload)", + "cel.@hasField(msg, [[62, \"map_int64_nested_type\", -1, [3, 11]], 1, [1, \"child\"]," + + " [2, \"payload\"]], 2)"), + STRICT_FIELDS_BEFORE_OPTIONAL_INDEX( + "nested.child.payload.repeated_nested_message[?0].bb", + "cel.@optionalAttribute(nested, [[1, \"child\", 11], [2, \"payload\", 11]," + + " [51, \"repeated_nested_message\", 11], 0, [1, \"bb\", 5]], 3, 0)"), + OPTIONAL_COMPREHENSION_VARIABLE( + "[opt_nested].map(o, o.child.payload.single_int64)", + "[opt_nested].map(o, cel.@optionalAttribute(o, [[1, \"child\", 11]," + + " [2, \"payload\", 11], [2, \"single_int64\", 3]], -1, 0))"), + HAS_FIELD_INDEX_ON_OPTIONAL_OPERAND( + "has(opt_nested.payload.repeated_nested_message[0].bb)", + "cel.@hasField(opt_nested, [[2, \"payload\", 11], [51, \"repeated_nested_message\", 11]," + + " 0, [1, \"bb\"]], -1)"), + HAS_FIELD_STRICT_FIELD_AFTER_INDEX_BEFORE_OPTIONAL_HOP( + "has(msg.map_int64_nested_type[1].child.?payload.single_int32)", + "cel.@hasField(msg, [[62, \"map_int64_nested_type\", -1, [3, 11]], 1, [1, \"child\"]," + + " [2, \"payload\"], [1, \"single_int32\"]], 3)"), + HAS_FIELD_OPTIONAL_OPERAND_WITH_OPTIONAL_HOP( + "has(opt_nested.child.?payload.single_int32)", + "cel.@hasField(opt_nested, [[1, \"child\"], [2, \"payload\"], [1, \"single_int32\"]]," + + " -1)"), + HAS_FIELD_OPTIONAL_OPERAND( + "has(opt_nested.child.payload)", + "cel.@hasField(opt_nested, [[1, \"child\"], [2, \"payload\"]], -1)"); + + private final String expression; + private final String expectedUnparsed; + + OptionalRewriteTestCase(String expression, String expectedUnparsed) { + this.expression = expression; + this.expectedUnparsed = expectedUnparsed; + } + } + @Test - public void optimize_optionalFieldSelection_throwsUnsupportedOperationException( + public void optimize_optionalSelect_rewritesToOptionalForms( + @TestParameter OptionalRewriteTestCase testCase) throws Exception { + CelAbstractSyntaxTree ast = cel.compile(testCase.expression).getAst(); + + CelAbstractSyntaxTree optimizedAst = celOptimizer.optimize(ast); + + assertThat(CEL_UNPARSER.unparse(optimizedAst)).isEqualTo(testCase.expectedUnparsed); + // Formatted, as a message type is a StructTypeReference before optimization but not after. + assertThat(CelTypes.format(optimizedAst.getResultType())) + .isEqualTo(CelTypes.format(ast.getResultType())); + } + + @Test + public void optimizeAndEvaluate_optionalSelect_matchesUnoptimized( + @TestParameter OptionalRewriteTestCase testCase, @TestParameter OptionalSelectInput input) + throws Exception { + assumeTrue(runtimeFlavor == CelRuntimeFlavor.PLANNER); + CelAbstractSyntaxTree ast = cel.compile(testCase.expression).getAst(); + Program unoptimizedProgram = cel.createProgram(ast); + Program optimizedProgram = cel.createProgram(celOptimizer.optimize(ast)); + + Object unoptimizedResult = unoptimizedProgram.eval(input.activation); + Object optimizedResult = optimizedProgram.eval(input.activation); + + assertThat(optimizedResult).isEqualTo(unoptimizedResult); + } + + @Test + public void optimizeAndEvaluate_absentStrictHopBeforeOptionalHop_throwsSameErrorAsUnoptimized( @TestParameter({ - "msg.?single_nested_message", - "msg.repeated_nested_message[?0].?bb", - "msg.repeated_nested_message[?0].bb" + "msg.map_int64_nested_type[2].?child.payload.single_int64", + "msg.map_int64_nested_type[1].payload.repeated_nested_message[1].?bb", + "has(msg.map_int64_nested_type[2].?child.payload)" }) String expression) throws Exception { - Cel celWithOptional = cel.toCelBuilder().addCompilerLibraries(CelExtensions.optional()).build(); - CelAbstractSyntaxTree ast = celWithOptional.compile(expression).getAst(); - SelectOptimizer optimizer = SelectOptimizer.newInstance(TestAllTypes.getDescriptor().getFile()); + assumeTrue(runtimeFlavor == CelRuntimeFlavor.PLANNER); + CelAbstractSyntaxTree ast = cel.compile(expression).getAst(); + Program unoptimizedProgram = cel.createProgram(ast); + Program optimizedProgram = cel.createProgram(celOptimizer.optimize(ast)); - UnsupportedOperationException e = + CelEvaluationException unoptimizedError = + assertThrows( + CelEvaluationException.class, + () -> unoptimizedProgram.eval(OptionalSelectInput.POPULATED.activation)); + CelEvaluationException optimizedError = assertThrows( - UnsupportedOperationException.class, () -> optimizer.optimize(ast, celWithOptional)); + CelEvaluationException.class, + () -> optimizedProgram.eval(OptionalSelectInput.POPULATED.activation)); - assertThat(e) + assertThat(optimizedError) + .hasCauseThat() .hasMessageThat() - .contains("Optimization of optional field selection is currently unimplemented"); + .isEqualTo(unoptimizedError.getCause().getMessage()); } @Test - public void optimize_optionalMapKeySelection_rewritesMapField() throws Exception { - Cel celWithOptional = cel.toCelBuilder().addCompilerLibraries(CelExtensions.optional()).build(); - CelAbstractSyntaxTree ast = celWithOptional.compile("msg.map_string_string.?key").getAst(); + @TestParameters( + "{expression: 'nested.?child.payload.single_int64', unknown: 'nested.child.payload'}") + @TestParameters( + "{expression: 'nested.child.?payload.single_int64', unknown: 'nested.child.payload'}") + @TestParameters( + "{expression: 'nested.payload.repeated_nested_message[?0].bb', unknown: 'nested.payload'}") + @TestParameters( + "{expression: 'has(nested.?child.payload.single_int64)', unknown: 'nested.child.payload'}") + public void optimizeAndEvaluate_optionalSelectUnderUnknownPattern_returnsPatternAsUnknown( + String expression, String unknown) throws Exception { + assumeTrue(runtimeFlavor == CelRuntimeFlavor.PLANNER); + // Unlike unoptimized evaluation, whose attribute trail breaks at each `_?._`, the optimized + // trail spans the whole chain, so the unknown reported is the pattern itself. + Program program = cel.createProgram(celOptimizer.optimize(cel.compile(expression).getAst())); + PartialVars partialVars = + PartialVars.of( + OptionalSelectInput.POPULATED.activation, + CelAttributePattern.fromQualifiedIdentifier(unknown)); + + Object result = program.eval(partialVars); + + assertThat(result).isInstanceOf(CelUnknownSet.class); + assertThat(((CelUnknownSet) result).attributes()) + .containsExactly(CelAttribute.fromQualifiedIdentifier(unknown)); + } - CelAbstractSyntaxTree optimizedAst = newSelectOptimizer(celWithOptional).optimize(ast); + @Test + public void optimizeAndEvaluate_optionalSelectBesideUnknownPattern_matchesUnoptimized() + throws Exception { + assumeTrue(runtimeFlavor == CelRuntimeFlavor.PLANNER); + CelAbstractSyntaxTree ast = cel.compile("msg.repeated_nested_message[?0].bb").getAst(); + Program unoptimizedProgram = cel.createProgram(ast); + Program optimizedProgram = cel.createProgram(celOptimizer.optimize(ast)); + PartialVars partialVars = + PartialVars.of( + OptionalSelectInput.POPULATED.activation, + CelAttributePattern.fromQualifiedIdentifier("msg.single_int64")); - assertThat(CEL_UNPARSER.unparse(optimizedAst)) - .isEqualTo( - "cel.@attribute(msg, [[61, \"map_string_string\", -1, [9, 9]]], {\"\": \"\"}).?key"); + Object unoptimizedResult = unoptimizedProgram.eval(partialVars); + Object optimizedResult = optimizedProgram.eval(partialVars); + + assertThat(optimizedResult).isEqualTo(unoptimizedResult); + assertThat((Optional) optimizedResult).hasValue(4L); } @Test @@ -1759,6 +2104,11 @@ private static CelOptimizer newSelectOptimizer(Cel cel) { .build(); } + // Proto3 NestedTestAllTypes requires FQN due to simple-name collision with proto2's. + private static dev.cel.expr.conformance.proto3.NestedTestAllTypes.Builder newProto3Nested() { + return dev.cel.expr.conformance.proto3.NestedTestAllTypes.newBuilder(); + } + private static TestAllTypes newTestAllTypesWithRepeatedNestedMessages(int... bbs) { TestAllTypes.Builder builder = TestAllTypes.newBuilder(); for (int bb : bbs) { diff --git a/optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerTest.java b/optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerTest.java index 7a86910af..823670502 100644 --- a/optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerTest.java +++ b/optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerTest.java @@ -840,7 +840,11 @@ public void cse_nestedMacro_withOptimization_assertAstIdCorrectness() throws Exc @Test public void optimize_afterSelectOptimizer_throwsException( - @TestParameter({"msg.single_int64 == 1 && msg.single_int64 == 1", "has(msg.single_int64)"}) + @TestParameter({ + "msg.single_int64 == 1 && msg.single_int64 == 1", + "has(msg.single_int64)", + "msg.?single_int64" + }) String expression) throws Exception { CelOptimizer selectThenCseOptimizer = diff --git a/runtime/src/main/java/dev/cel/runtime/planner/OptimizedSelectPlanner.java b/runtime/src/main/java/dev/cel/runtime/planner/OptimizedSelectPlanner.java index 6b6cd7559..56c3e73e3 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/OptimizedSelectPlanner.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/OptimizedSelectPlanner.java @@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.primitives.UnsignedLong; import com.google.errorprone.annotations.Immutable; @@ -28,6 +29,7 @@ import dev.cel.common.types.CelTypes; import dev.cel.common.values.CelByteString; import dev.cel.common.values.CelValueConverter; +import dev.cel.common.values.ErrorValue; import dev.cel.common.values.OptimizedSelectTraversal; import dev.cel.common.values.OptionalValue; import dev.cel.common.values.SelectField; @@ -41,15 +43,21 @@ import org.jspecify.annotations.Nullable; /** - * Plans optimizer-rewritten select chains ({@code cel.@attribute}) and presence tests ({@code - * cel.@hasField}) into {@link EvalAttribute} instances decorated with {@code - * OptimizedSelectQualifier}. + * Plans optimizer-rewritten select chains ({@code cel.@attribute}, {@code cel.@optionalAttribute}) + * and presence tests ({@code cel.@hasField}) into {@link EvalAttribute} instances decorated with + * {@code OptimizedSelectQualifier}. */ @Immutable final class OptimizedSelectPlanner { static final String CEL_ATTRIBUTE_FUNCTION_NAME = "cel.@attribute"; + static final String CEL_OPTIONAL_ATTRIBUTE_FUNCTION_NAME = "cel.@optionalAttribute"; static final String CEL_HAS_FIELD_FUNCTION_NAME = "cel.@hasField"; + static final ImmutableSet FUNCTION_NAMES = + ImmutableSet.of( + CEL_ATTRIBUTE_FUNCTION_NAME, + CEL_OPTIONAL_ATTRIBUTE_FUNCTION_NAME, + CEL_HAS_FIELD_FUNCTION_NAME); // A list index is an int, as typed by an INT64 (3) map key. private static final long LIST_INDEX_TYPE_CODE = 3; @@ -68,7 +76,12 @@ static OptimizedSelectPlanner create( PlannedInterpretable plan(CelExpr expr, String functionName, OperandPlanner operandPlanner) { ImmutableList args = expr.call().args(); boolean isPresenceTest = functionName.equals(CEL_HAS_FIELD_FUNCTION_NAME); - int expectedArgCount = isPresenceTest ? 2 : 3; + // Optional chains carry the index of their first optional qualifier as the 3rd argument, or -1 + // if their operand is optional. + boolean isOptional = + functionName.equals(CEL_OPTIONAL_ATTRIBUTE_FUNCTION_NAME) + || (isPresenceTest && args.size() == 3); + int expectedArgCount = (isPresenceTest ? 2 : 3) + (isOptional ? 1 : 0); checkArgument( args.size() == expectedArgCount, "Expected %s arguments for %s, found %s", @@ -76,9 +89,15 @@ PlannedInterpretable plan(CelExpr expr, String functionName, OperandPlanner oper functionName, args.size()); + ImmutableList hops = unpackQualifierHops(args.get(1)); + // A presence test's last hop is the field it tests, which can't be optionally selected. + int optIndex = + isOptional + ? parseOptIndex(args.get(2), isPresenceTest ? hops.size() - 1 : hops.size()) + : hops.size(); + // Constant index hops split the qualifiers into field segments, each but the last ending at the // repeated or map field that the following index selects from. - ImmutableList hops = unpackQualifierHops(args.get(1)); ImmutableList.Builder qualifiers = ImmutableList.builder(); int segmentStart = 0; for (int i = 0; i < hops.size(); i++) { @@ -92,20 +111,26 @@ PlannedInterpretable plan(CelExpr expr, String functionName, OperandPlanner oper hop); ImmutableList fields = unpackAttributeFields(hops.subList(segmentStart, i), /* dummyOrDefExpr= */ null); - addSegmentQualifiers(qualifiers, fields, /* isPresenceTest= */ false); + addFieldQualifiers(qualifiers, fields, segmentStart, optIndex, /* isPresenceTest= */ false); SelectField.MapEntrySpec mapEntrySpec = Iterables.getLast(fields).mapEntrySpec(); long keyTypeCode = mapEntrySpec != null ? mapEntrySpec.keyTypeCode() : LIST_INDEX_TYPE_CODE; qualifiers.add( new IndexQualifier( - resolveScalarDefaultValue(keyTypeCode, hop), runtimeEquality, celValueConverter)); + resolveScalarDefaultValue(keyTypeCode, hop), + /* isOptional= */ i >= optIndex, + /* expectsOptionalOperand= */ i > optIndex, + runtimeEquality, + celValueConverter)); segmentStart = i + 1; } ImmutableList leafSegment = hops.subList(segmentStart, hops.size()); - addSegmentQualifiers( + addFieldQualifiers( qualifiers, isPresenceTest ? unpackHasFieldFields(leafSegment) - : unpackAttributeFields(leafSegment, args.get(2)), + : unpackAttributeFields(leafSegment, Iterables.getLast(args)), + segmentStart, + optIndex, isPresenceTest); PlannedInterpretable operand = operandPlanner.plan(args.get(0)); @@ -116,14 +141,46 @@ PlannedInterpretable plan(CelExpr expr, String functionName, OperandPlanner oper return EvalAttribute.create(expr, qualified); } - private void addSegmentQualifiers( + // Adds the field hops from qualifier index `start` on: those before optIndex strictly, the rest + // optionally. + private void addFieldQualifiers( ImmutableList.Builder qualifiers, ImmutableList fields, + int start, + int optIndex, boolean isPresenceTest) { + int strictCount = optIndex - start; + // Presence tests needn't split: an unset message has no fields set, as an absent one doesn't. + if (!isPresenceTest && strictCount > 0 && strictCount < fields.size()) { + addSegmentQualifiers( + qualifiers, + fields.subList(0, strictCount), + /* isPresenceTest= */ false, + /* isOptional= */ false, + /* expectsOptionalOperand= */ false); + fields = fields.subList(strictCount, fields.size()); + start = optIndex; + } + addSegmentQualifiers( + qualifiers, + fields, + isPresenceTest, + /* isOptional= */ start >= optIndex, + /* expectsOptionalOperand= */ start > optIndex); + } + + private void addSegmentQualifiers( + ImmutableList.Builder qualifiers, + ImmutableList fields, + boolean isPresenceTest, + boolean isOptional, + boolean expectsOptionalOperand) { for (int i = 0; i < fields.size() - 1; i++) { qualifiers.add(new PassthroughQualifier(fields.get(i).fieldName())); } - qualifiers.add(new OptimizedSelectQualifier(fields, celValueConverter, isPresenceTest)); + qualifiers.add( + new OptimizedSelectQualifier( + fields, celValueConverter, isPresenceTest, isOptional, expectsOptionalOperand)); } /** @@ -289,12 +346,7 @@ private static ImmutableList unpackHopElements(CelExpr hopExpr) { } private static long parseFieldNumber(CelExpr expr) { - checkArgument( - expr.getKind() == Kind.CONSTANT - && expr.constant().getKind() == CelConstant.Kind.INT64_VALUE, - "Expected qualifier hop field number to be an int64 constant, found: %s", - expr); - return expr.constant().int64Value(); + return parseInt64Constant(expr, "qualifier hop field number"); } private static String parseFieldName(CelExpr expr) { @@ -307,10 +359,25 @@ private static String parseFieldName(CelExpr expr) { } private static long parseTypeCode(CelExpr expr) { + return parseInt64Constant(expr, "qualifier hop type code"); + } + + private static int parseOptIndex(CelExpr expr, int exclusiveMax) { + long optIndex = parseInt64Constant(expr, "optional qualifier index"); + checkArgument( + optIndex >= -1 && optIndex < exclusiveMax, + "Optional qualifier index %s is out of range [-1, %s)", + optIndex, + exclusiveMax); + return (int) optIndex; + } + + private static long parseInt64Constant(CelExpr expr, String description) { checkArgument( expr.getKind() == Kind.CONSTANT && expr.constant().getKind() == CelConstant.Kind.INT64_VALUE, - "Expected qualifier hop type code to be an int64 constant, found: %s", + "Expected %s to be an int64 constant, found: %s", + description, expr); return expr.constant().int64Value(); } @@ -335,6 +402,10 @@ private static String parseMessageProtoTypeName(CelExpr expr) { checkArgument( expr.getKind() == Kind.STRUCT, "Expected struct for message type, found: %s", expr); String protoTypeName = expr.struct().messageName(); + // Un-rechecked optimizer dummies carry a leading dot to resist container shadowing. + if (protoTypeName.startsWith(".")) { + protoTypeName = protoTypeName.substring(1); + } checkArgument(!protoTypeName.isEmpty(), "Protobuf message type name must not be empty"); return protoTypeName; } @@ -359,13 +430,6 @@ private static void validateScalarDummy(long typeCode, CelExpr dummyExpr) { resolveScalarDefaultValue(typeCode, dummyExpr); } - private static void validateTarget(Object target) { - if (target instanceof OptionalValue || target instanceof Optional) { - throw new UnsupportedOperationException( - "Optional operands are not yet supported by the select-optimized runtime"); - } - } - /** * Functional interface for planning an operand expression into a {@link PlannedInterpretable}. */ @@ -443,9 +507,54 @@ private PassthroughQualifier(String fieldName) { } @Immutable - private static final class OptimizedSelectQualifier implements Qualifier { + private abstract static class ChainQualifier implements Qualifier { + final CelValueConverter celValueConverter; + final boolean isOptional; + private final boolean expectsOptionalOperand; + + @Override + public final Object qualify(Object operand) { + if (operand instanceof ErrorValue) { + return operand; + } + // Fails closed on an operand whose binding contradicts the optionality it was typed with. + if (operand instanceof OptionalValue != expectsOptionalOperand) { + throw new IllegalArgumentException( + (expectsOptionalOperand + ? "Expected an optional operand for the optional select chain at: " + : "Unexpected optional operand for the select chain at: ") + + value()); + } + if (expectsOptionalOperand) { + OptionalValue optional = (OptionalValue) operand; + return optional.isZeroValue() ? absent() : qualifyValue(optional.value()); + } + return qualifyValue(operand); + } + + abstract Object qualifyValue(Object operand); + + Object absent() { + return OptionalValue.EMPTY; + } + + Object toOptional(Optional value) { + return value.isPresent() + ? OptionalValue.create(celValueConverter.toTraversalTarget(value.get())) + : OptionalValue.EMPTY; + } + + private ChainQualifier( + CelValueConverter celValueConverter, boolean isOptional, boolean expectsOptionalOperand) { + this.celValueConverter = checkNotNull(celValueConverter); + this.isOptional = isOptional; + this.expectsOptionalOperand = expectsOptionalOperand; + } + } + + @Immutable + private static final class OptimizedSelectQualifier extends ChainQualifier { private final ImmutableList fields; - private final CelValueConverter celValueConverter; private final boolean isPresenceTest; @Override @@ -454,33 +563,41 @@ public Object value() { } @Override - public Object qualify(Object operand) { - validateTarget(operand); + Object qualifyValue(Object operand) { if (isPresenceTest) { // A boolean is already a valid traversal target. return OptimizedSelectTraversal.hasField(operand, fields); } + if (isOptional) { + return toOptional(OptimizedSelectTraversal.find(operand, fields)); + } return celValueConverter.toTraversalTarget(OptimizedSelectTraversal.qualify(operand, fields)); } + @Override + Object absent() { + return isPresenceTest ? false : OptionalValue.EMPTY; + } + private OptimizedSelectQualifier( ImmutableList fields, CelValueConverter celValueConverter, - boolean isPresenceTest) { + boolean isPresenceTest, + boolean isOptional, + boolean expectsOptionalOperand) { + super(celValueConverter, isOptional, expectsOptionalOperand); this.fields = checkNotNull(fields); - this.celValueConverter = checkNotNull(celValueConverter); this.isPresenceTest = isPresenceTest; } } /** Indexes a repeated or map field's value by a constant list index or map key. */ @Immutable - private static final class IndexQualifier implements Qualifier { + private static final class IndexQualifier extends ChainQualifier { @SuppressWarnings("Immutable") // Immutable CEL constant. private final Object index; private final RuntimeEquality runtimeEquality; - private final CelValueConverter celValueConverter; @Override public Object value() { @@ -491,7 +608,13 @@ public Object value() { // in place rather than unwrapped first as the generic index operator's arguments are. @Override @SuppressWarnings("unchecked") // Map keys are compared as CEL values. - public Object qualify(Object operand) { + Object qualifyValue(Object operand) { + if (isOptional) { + return toOptional( + operand instanceof Map + ? runtimeEquality.findInMap((Map) operand, index) + : RuntimeHelpers.optIndexList((List) operand, (Long) index)); + } Object element = operand instanceof Map ? runtimeEquality.indexMap((Map) operand, index) @@ -500,10 +623,14 @@ public Object qualify(Object operand) { } private IndexQualifier( - Object index, RuntimeEquality runtimeEquality, CelValueConverter celValueConverter) { + Object index, + boolean isOptional, + boolean expectsOptionalOperand, + RuntimeEquality runtimeEquality, + CelValueConverter celValueConverter) { + super(celValueConverter, isOptional, expectsOptionalOperand); this.index = checkNotNull(index); this.runtimeEquality = checkNotNull(runtimeEquality); - this.celValueConverter = checkNotNull(celValueConverter); } } diff --git a/runtime/src/main/java/dev/cel/runtime/planner/ProgramPlanner.java b/runtime/src/main/java/dev/cel/runtime/planner/ProgramPlanner.java index 4314e66ad..4a4175778 100644 --- a/runtime/src/main/java/dev/cel/runtime/planner/ProgramPlanner.java +++ b/runtime/src/main/java/dev/cel/runtime/planner/ProgramPlanner.java @@ -260,11 +260,10 @@ private PlannedInterpretable planCall(CelExpr expr, PlannerContext ctx) { ResolvedFunction resolvedFunction = resolveFunction(expr, ctx.referenceMap()); String functionName = resolvedFunction.functionName(); - // Intercept optimizer-rewritten select chains (cel.@attribute and cel.@hasField) for direct - // traversal via OptimizedSelectPlanner (proto field number lookup on OptimizedSelectable, - // map key lookup on Map, and SelectableValue fallback). - if (functionName.equals(OptimizedSelectPlanner.CEL_ATTRIBUTE_FUNCTION_NAME) - || functionName.equals(OptimizedSelectPlanner.CEL_HAS_FIELD_FUNCTION_NAME)) { + // Intercept optimizer-rewritten select chains for direct traversal via OptimizedSelectPlanner + // (proto field number lookup on OptimizedSelectable, map key lookup on Map, and + // SelectableValue fallback). + if (OptimizedSelectPlanner.FUNCTION_NAMES.contains(functionName)) { checkArgument( ctx.runtimeExtensions.contains(SELECT_OPTIMIZATION_EXTENSION), "%s requires the %s AST extension", diff --git a/runtime/src/test/java/dev/cel/runtime/BUILD.bazel b/runtime/src/test/java/dev/cel/runtime/BUILD.bazel index 1e6b8db7c..15672ec17 100644 --- a/runtime/src/test/java/dev/cel/runtime/BUILD.bazel +++ b/runtime/src/test/java/dev/cel/runtime/BUILD.bazel @@ -44,6 +44,7 @@ java_library( "//common:proto_v1alpha1_ast", "//common/ast", "//common/ast:cel_block", + "//common/exceptions:attribute_not_found", "//common/exceptions:bad_format", "//common/exceptions:divide_by_zero", "//common/exceptions:numeric_overflow", @@ -68,6 +69,7 @@ java_library( "//compiler:compiler_builder", "//extensions", "//extensions:optional_library", + "//extensions:optional_runtime_library", "//optimizer", "//optimizer:optimizer_builder", "//optimizer/optimizers:common_subexpression_elimination", diff --git a/runtime/src/test/java/dev/cel/runtime/CelLiteRuntimeVersionSkewTest.java b/runtime/src/test/java/dev/cel/runtime/CelLiteRuntimeVersionSkewTest.java index eed9a88b7..6bcc2ab85 100644 --- a/runtime/src/test/java/dev/cel/runtime/CelLiteRuntimeVersionSkewTest.java +++ b/runtime/src/test/java/dev/cel/runtime/CelLiteRuntimeVersionSkewTest.java @@ -57,6 +57,7 @@ import dev.cel.common.CelOverloadDecl; import dev.cel.common.CelProtoAbstractSyntaxTree; import dev.cel.common.ast.CelBlock; +import dev.cel.common.exceptions.CelAttributeNotFoundException; import dev.cel.common.internal.ProtoTimeUtils; import dev.cel.common.types.ListType; import dev.cel.common.types.SimpleType; @@ -71,6 +72,7 @@ import dev.cel.expr.conformance.proto3.TestAllTypes.NestedMessage; import dev.cel.expr.conformance.proto3.TestAllTypesCelDescriptor; import dev.cel.extensions.CelExtensions; +import dev.cel.extensions.CelOptionalRuntimeLibrary; import dev.cel.optimizer.CelOptimizer; import dev.cel.optimizer.CelOptimizerFactory; import dev.cel.optimizer.optimizers.SelectOptimizer; @@ -85,6 +87,7 @@ import java.time.Instant; import java.util.List; import java.util.Map; +import java.util.Optional; import org.jspecify.annotations.Nullable; import org.junit.Before; import org.junit.Test; @@ -293,7 +296,7 @@ public void setUp() { CelFactory.plannerCelBuilder() .setOptions(CEL_OPTIONS) .setStandardMacros(CelStandardMacro.STANDARD_MACROS) - .addCompilerLibraries(CelExtensions.bindings()) + .addCompilerLibraries(CelExtensions.bindings(), CelExtensions.optional()) .addMessageTypes(TestAllTypes.getDescriptor(), NestedTestAllTypes.getDescriptor()) .addVar("msg", StructTypeReference.create(TestAllTypes.getDescriptor().getFullName())) .addVar( @@ -338,6 +341,7 @@ public void setUp() { clientRuntime = CelLiteRuntimeFactory.newLiteRuntimeBuilder() .setStandardFunctions(CelStandardFunctions.ALL_STANDARD_FUNCTIONS) + .addLibraries(CelOptionalRuntimeLibrary.optional()) .setValueProvider(clientValueProvider) .setContainer(CEL_CONTAINER) .build(); @@ -2842,6 +2846,70 @@ public void partialVars_unknownAttributeOnSkewedField_shortCircuits() throws Exc assertThat(((CelUnknownSet) unknownResult).attributes()).containsExactly(attribute); } + @SuppressWarnings("ImmutableEnumChecker") // Test only + private enum OptionalSelectTestCase { + UNKNOWN_SCALAR("msg.?single_int64", Optional.empty(), Optional.of(-42L)), + UNKNOWN_SUBMESSAGE_SCALAR("msg.?single_nested_message.bb", Optional.empty(), Optional.of(123L)), + UNKNOWN_REPEATED_INDEX( + "msg.repeated_nested_message[?1].bb", Optional.empty(), Optional.of(20L)), + UNKNOWN_REPEATED_INDEX_OUT_OF_RANGE( + "msg.repeated_nested_message[?2].bb", Optional.empty(), Optional.empty()), + UNKNOWN_MAP_KEY( + "msg.map_int64_nested_type[?2].payload.single_int64", Optional.empty(), Optional.of(200L)), + UNKNOWN_MAP_KEY_ABSENT( + "msg.map_int64_nested_type[?3].payload.single_int64", Optional.empty(), Optional.empty()), + HAS_FIELD("has(msg.?single_nested_message.bb)", false, true), + OR_VALUE("msg.?single_int64.orValue(7)", 7L, -42L), + HAS_VALUE("msg.?single_nested_message.hasValue()", false, true); + + private final String expression; + private final Object expectedDefault; + private final Object expectedPopulated; + + OptionalSelectTestCase(String expression, Object expectedDefault, Object expectedPopulated) { + this.expression = expression; + this.expectedDefault = expectedDefault; + this.expectedPopulated = expectedPopulated; + } + } + + @Test + public void optionalSelect_unsetSkewedFields_returnsAbsent( + @TestParameter OptionalSelectTestCase testCase) throws Exception { + Object result = eval(testCase.expression, TestAllTypes.getDefaultInstance()); + + assertThat(result).isEqualTo(testCase.expectedDefault); + } + + @Test + public void optionalSelect_populatedSkewedFields_decodesFromWireBytes( + @TestParameter OptionalSelectTestCase testCase) throws Exception { + Object result = eval(testCase.expression, POPULATED_SERVER_MESSAGE); + + assertThat(result).isEqualTo(testCase.expectedPopulated); + } + + @Test + @TestParameters("{expression: 'msg.?single_int32', expected: 42}") + @TestParameters("{expression: 'msg.?standalone_message.bb', expected: 77}") + public void optionalSelect_renamedFields_resolvesByFieldNumber(String expression, long expected) + throws Exception { + Object result = eval(expression, POPULATED_RENAMED_MESSAGE); + + assertThat((Optional) result).hasValue(expected); + } + + @Test + public void optionalSelect_missingMapKeyBeforeOptionalHop_throws() throws Exception { + CelEvaluationException e = + assertThrows( + CelEvaluationException.class, + () -> eval("msg.map_int64_nested_type[3].?child.payload", POPULATED_SERVER_MESSAGE)); + + assertThat(e).hasCauseThat().isInstanceOf(CelAttributeNotFoundException.class); + assertThat(e).hasCauseThat().hasMessageThat().isEqualTo("3"); + } + @Test public void crossSerialization_fullProtoDynamicMessageToV1Lite() throws Exception { // Evolve TestAllTypes.getDescriptor() into a V2 server schema in memory: diff --git a/runtime/src/test/java/dev/cel/runtime/planner/OptimizedSelectPlannerTest.java b/runtime/src/test/java/dev/cel/runtime/planner/OptimizedSelectPlannerTest.java index 06e19d303..4aa8628bc 100644 --- a/runtime/src/test/java/dev/cel/runtime/planner/OptimizedSelectPlannerTest.java +++ b/runtime/src/test/java/dev/cel/runtime/planner/OptimizedSelectPlannerTest.java @@ -47,12 +47,14 @@ import dev.cel.common.types.CelTypeProvider.CombinedCelTypeProvider; import dev.cel.common.types.DefaultTypeProvider; import dev.cel.common.types.MapType; +import dev.cel.common.types.OptionalType; import dev.cel.common.types.ProtoMessageTypeProvider; import dev.cel.common.types.SimpleType; import dev.cel.common.types.StructTypeReference; import dev.cel.common.values.CelByteString; import dev.cel.common.values.CelValueConverter; import dev.cel.common.values.CelValueProvider; +import dev.cel.common.values.ErrorValue; import dev.cel.common.values.ProtoCelValueConverter; import dev.cel.common.values.ProtoMessageValueProvider; import dev.cel.expr.conformance.proto3.NestedTestAllTypes; @@ -80,6 +82,7 @@ import java.time.Instant; import java.util.HashMap; import java.util.Map; +import java.util.Optional; import org.junit.Test; import org.junit.runner.RunWith; @@ -127,6 +130,10 @@ public final class OptimizedSelectPlannerTest { .setOptions(CEL_OPTIONS) .setStandardMacros(CelStandardMacro.STANDARD_MACROS) .addVar("msg", StructTypeReference.create(TestAllTypes.getDescriptor().getFullName())) + .addVar( + "opt_msg", + OptionalType.create( + StructTypeReference.create(TestAllTypes.getDescriptor().getFullName()))) .addVar("b", StructTypeReference.create(TestAllTypes.getDescriptor().getFullName())) .addVar("cond", SimpleType.BOOL) .addVar( @@ -486,20 +493,55 @@ public void plan_missingSelectOptimizationExtension_throwsEvaluationException( assertThat(e).hasMessageThat().contains("requires the select_optimization AST extension"); } + @SuppressWarnings("ImmutableEnumChecker") // Test only + private enum WrongArgCountTestCase { + ATTRIBUTE_TOO_FEW( + OptimizedSelectPlanner.CEL_ATTRIBUTE_FUNCTION_NAME, + ImmutableList.of(CelExpr.ofIdent(2L, "msg")), + "Expected 3 arguments for cel.@attribute, found 1"), + OPTIONAL_ATTRIBUTE_WITHOUT_OPT_INDEX( + OptimizedSelectPlanner.CEL_OPTIONAL_ATTRIBUTE_FUNCTION_NAME, + ImmutableList.of( + CelExpr.ofIdent(0L, "msg"), + listExpr(fieldHop(2L, "single_int64", 3L)), + constantHop(0L)), + "Expected 4 arguments for cel.@optionalAttribute, found 3"), + HAS_FIELD_TOO_FEW( + OptimizedSelectPlanner.CEL_HAS_FIELD_FUNCTION_NAME, + ImmutableList.of(CelExpr.ofIdent(0L, "msg")), + "Expected 2 arguments for cel.@hasField, found 1"), + HAS_FIELD_TOO_MANY( + OptimizedSelectPlanner.CEL_HAS_FIELD_FUNCTION_NAME, + ImmutableList.of( + CelExpr.ofIdent(0L, "msg"), + listExpr(hasFieldHop(2L, "single_int64")), + constantHop(0L), + constantHop(0L)), + "Expected 2 arguments for cel.@hasField, found 4"); + + private final String functionName; + private final ImmutableList args; + private final String expectedMessageSubstring; + + WrongArgCountTestCase( + String functionName, ImmutableList args, String expectedMessageSubstring) { + this.functionName = functionName; + this.args = args; + this.expectedMessageSubstring = expectedMessageSubstring; + } + } + @Test - public void plan_invalidAst_wrongArgCount_throwsEvaluationException() { + public void plan_invalidAst_wrongArgCount_throwsEvaluationException( + @TestParameter WrongArgCountTestCase testCase) { CelAbstractSyntaxTree ast = CelAbstractSyntaxTree.newParsedAst( - CelExpr.ofCall( - 1L, - OptimizedSelectPlanner.CEL_ATTRIBUTE_FUNCTION_NAME, - ImmutableList.of(CelExpr.ofIdent(2L, "msg"))), - SELECT_OPTIMIZED_SOURCE); + CelExpr.ofCall(1L, testCase.functionName, testCase.args), SELECT_OPTIMIZED_SOURCE); CelEvaluationException e = assertThrows(CelEvaluationException.class, () -> PLANNER.plan(ast)); assertThat(e).hasCauseThat().isInstanceOf(IllegalArgumentException.class); - assertThat(e).hasMessageThat().contains("Expected 3 arguments for cel.@attribute, found 1"); + assertThat(e).hasMessageThat().contains(testCase.expectedMessageSubstring); } @Test @@ -821,7 +863,8 @@ public void plan_invalidAst_mapFieldWithEmptyMapDummy_throwsEvaluationException( } @Test - public void plan_invalidAst_messageStructWithEmptyMessageName_throwsEvaluationException() { + public void plan_invalidAst_messageStructWithEmptyMessageName_throwsEvaluationException( + @TestParameter({"", "."}) String messageName) { CelAbstractSyntaxTree ast = CelAbstractSyntaxTree.newParsedAst( CelExpr.ofCall( @@ -829,22 +872,8 @@ public void plan_invalidAst_messageStructWithEmptyMessageName_throwsEvaluationEx OptimizedSelectPlanner.CEL_ATTRIBUTE_FUNCTION_NAME, ImmutableList.of( CelExpr.ofIdent(2L, "msg"), - CelExpr.ofList( - 3L, - ImmutableList.of( - CelExpr.ofList( - 4L, - ImmutableList.of( - CelExpr.ofConstant(5L, CelConstant.ofValue(51L)), - CelExpr.ofConstant( - 6L, CelConstant.ofValue("repeated_nested_message")), - CelExpr.ofConstant(7L, CelConstant.ofValue(11L))), - ImmutableList.of())), - ImmutableList.of()), - CelExpr.ofList( - 8L, - ImmutableList.of(CelExpr.ofStruct(9L, "", ImmutableList.of())), - ImmutableList.of()))), + listExpr(fieldHop(51L, "repeated_nested_message", 11L)), + listExpr(CelExpr.ofStruct(9L, messageName, ImmutableList.of())))), SELECT_OPTIMIZED_SOURCE); CelEvaluationException e = assertThrows(CelEvaluationException.class, () -> PLANNER.plan(ast)); @@ -917,35 +946,150 @@ public void plan_invalidAst_malformedIndexHop_throwsEvaluationException( assertThat(e).hasMessageThat().contains(testCase.expectedMessageSubstring); } + @SuppressWarnings("ImmutableEnumChecker") // Test only + private enum InvalidOptIndexTestCase { + NOT_CONSTANT( + /* isPresenceTest= */ false, + CelExpr.ofIdent(0L, "x"), + "Expected optional qualifier index to be an int64 constant"), + NOT_INT( + /* isPresenceTest= */ false, + constantHop(UnsignedLong.ZERO), + "Expected optional qualifier index to be an int64 constant"), + BELOW_RANGE( + /* isPresenceTest= */ false, + constantHop(-2L), + "Optional qualifier index -2 is out of range [-1, 2)"), + ABOVE_RANGE( + /* isPresenceTest= */ false, + constantHop(2L), + "Optional qualifier index 2 is out of range [-1, 2)"), + HAS_FIELD_ON_TESTED_LEAF( + /* isPresenceTest= */ true, + constantHop(1L), + "Optional qualifier index 1 is out of range [-1, 1)"); + + private final boolean isPresenceTest; + private final CelExpr optIndex; + private final String expectedMessageSubstring; + + InvalidOptIndexTestCase( + boolean isPresenceTest, CelExpr optIndex, String expectedMessageSubstring) { + this.isPresenceTest = isPresenceTest; + this.optIndex = optIndex; + this.expectedMessageSubstring = expectedMessageSubstring; + } + } + + @Test + public void plan_invalidAst_invalidOptIndex_throwsEvaluationException( + @TestParameter InvalidOptIndexTestCase testCase) { + ImmutableList args = + testCase.isPresenceTest + ? ImmutableList.of( + CelExpr.ofIdent(0L, "msg"), + listExpr(hasFieldHop(21L, "single_nested_message"), hasFieldHop(1L, "bb")), + testCase.optIndex) + : ImmutableList.of( + CelExpr.ofIdent(0L, "msg"), + listExpr(fieldHop(21L, "single_nested_message", 11L), fieldHop(1L, "bb", 5L)), + testCase.optIndex, + constantHop(0L)); + CelAbstractSyntaxTree ast = + CelAbstractSyntaxTree.newParsedAst( + CelExpr.ofCall( + 1L, + testCase.isPresenceTest + ? OptimizedSelectPlanner.CEL_HAS_FIELD_FUNCTION_NAME + : OptimizedSelectPlanner.CEL_OPTIONAL_ATTRIBUTE_FUNCTION_NAME, + args), + SELECT_OPTIMIZED_SOURCE); + + CelEvaluationException e = assertThrows(CelEvaluationException.class, () -> PLANNER.plan(ast)); + + assertThat(e).hasCauseThat().isInstanceOf(IllegalArgumentException.class); + assertThat(e).hasMessageThat().contains(testCase.expectedMessageSubstring); + } + + @Test + public void plan_celOptionalAttribute_presentOptionalOperand_returnsOptionalOfValue() + throws Exception { + Program program = PLANNER.plan(optimizeSelectAst("opt_msg.single_int64")); + TestAllTypes msg = TestAllTypes.newBuilder().setSingleInt64(42L).build(); + + Object result = program.eval(ImmutableMap.of("opt_msg", Optional.of(msg))); + + assertThat(result).isEqualTo(Optional.of(42L)); + } + + @Test + public void plan_celOptionalAttribute_absentOptionalOperand_returnsOptionalEmpty() + throws Exception { + Program program = PLANNER.plan(optimizeSelectAst("opt_msg.single_int64")); + + Object result = program.eval(ImmutableMap.of("opt_msg", Optional.empty())); + + assertThat(result).isEqualTo(Optional.empty()); + } + + @Test + public void plan_errorValueOperand_propagatesError( + @TestParameter({ + "msg.?single_int64", + "msg.repeated_nested_message[?0].bb", + "opt_msg.single_int64", + "has(opt_msg.single_nested_message.bb)" + }) + String expression) + throws Exception { + Program program = PLANNER.plan(optimizeSelectAst(expression)); + ErrorValue error = ErrorValue.create(0L, new IllegalStateException("Intentional error")); + ImmutableMap input = ImmutableMap.of("msg", error, "opt_msg", error); + + CelEvaluationException e = + assertThrows(CelEvaluationException.class, () -> program.eval(input)); + + assertThat(e).hasCauseThat().isInstanceOf(IllegalStateException.class); + assertThat(e).hasCauseThat().hasMessageThat().isEqualTo("Intentional error"); + } + + @Test + public void plan_directSelectOptimizerAstWithLeadingDotMessageName_resolvesDefaultValue() + throws Exception { + SelectOptimizer directOptimizer = + SelectOptimizer.newInstance(TestAllTypes.getDescriptor().getFile()); + CelAbstractSyntaxTree optimizedAst = + directOptimizer.optimize(CEL.compile("msg.single_duration").getAst(), CEL).optimizedAst(); + + Object result = + PLANNER.plan(optimizedAst).eval(ImmutableMap.of("msg", TestAllTypes.getDefaultInstance())); + + assertThat(result).isEqualTo(Duration.ZERO); + } + + private static CelExpr hasFieldHop(long fieldNumber, String fieldName) { + return listExpr(constantHop(fieldNumber), constantHop(fieldName)); + } + private static CelExpr fieldHop(long fieldNumber, String fieldName, long typeCode) { - return CelExpr.ofList( - 0L, - ImmutableList.of( - CelExpr.ofConstant(0L, CelConstant.ofValue(fieldNumber)), - CelExpr.ofConstant(0L, CelConstant.ofValue(fieldName)), - CelExpr.ofConstant(0L, CelConstant.ofValue(typeCode))), - ImmutableList.of()); + return listExpr(constantHop(fieldNumber), constantHop(fieldName), constantHop(typeCode)); } private static CelExpr mapHop( long fieldNumber, String fieldName, long keyTypeCode, long valueTypeCode) { - return CelExpr.ofList( - 0L, - ImmutableList.of( - CelExpr.ofConstant(0L, CelConstant.ofValue(fieldNumber)), - CelExpr.ofConstant(0L, CelConstant.ofValue(fieldName)), - CelExpr.ofConstant(0L, CelConstant.ofValue(-1L)), - mapEntrySpec(keyTypeCode, valueTypeCode)), - ImmutableList.of()); + return listExpr( + constantHop(fieldNumber), + constantHop(fieldName), + constantHop(-1L), + mapEntrySpec(keyTypeCode, valueTypeCode)); } private static CelExpr mapEntrySpec(long keyTypeCode, long valueTypeCode) { - return CelExpr.ofList( - 8L, - ImmutableList.of( - CelExpr.ofConstant(9L, CelConstant.ofValue(keyTypeCode)), - CelExpr.ofConstant(10L, CelConstant.ofValue(valueTypeCode))), - ImmutableList.of()); + return listExpr(constantHop(keyTypeCode), constantHop(valueTypeCode)); + } + + private static CelExpr listExpr(CelExpr... elements) { + return CelExpr.ofList(0L, ImmutableList.copyOf(elements), ImmutableList.of()); } private static CelExpr constantHop(Object index) { @@ -1117,7 +1261,47 @@ private enum EvaluationErrorTestCase { CelInvalidArgumentException.class, "Map value cannot be null for key: null_key"), ATTRIBUTE_UNBOUND_ROOT_VARIABLE( - "msg.single_int64", ImmutableMap.of(), CelAttributeNotFoundException.class, "msg"); + "msg.single_int64", ImmutableMap.of(), CelAttributeNotFoundException.class, "msg"), + OPTIONAL_ATTRIBUTE_PLAIN_VALUE_BOUND_TO_OPTIONAL_OPERAND( + "opt_msg.single_int64", + ImmutableMap.of("opt_msg", TestAllTypes.getDefaultInstance()), + IllegalArgumentException.class, + "Expected an optional operand for the optional select chain at: single_int64"), + OPTIONAL_HAS_FIELD_PLAIN_VALUE_BOUND_TO_OPTIONAL_OPERAND( + "has(opt_msg.single_int64)", + ImmutableMap.of("opt_msg", TestAllTypes.getDefaultInstance()), + IllegalArgumentException.class, + "Expected an optional operand for the optional select chain at: single_int64"), + ATTRIBUTE_OPTIONAL_BOUND_TO_PLAIN_OPERAND( + "msg.single_int64", + ImmutableMap.of("msg", Optional.of(TestAllTypes.getDefaultInstance())), + IllegalArgumentException.class, + "Unexpected optional operand for the select chain at: single_int64"), + OPTIONAL_ATTRIBUTE_OPTIONAL_BOUND_TO_PLAIN_OPERAND( + "msg.?single_int64", + ImmutableMap.of("msg", Optional.of(TestAllTypes.getDefaultInstance())), + IllegalArgumentException.class, + "Unexpected optional operand for the select chain at: single_int64"), + HAS_FIELD_OPTIONAL_BOUND_TO_PLAIN_OPERAND( + "has(msg.single_int64)", + ImmutableMap.of("msg", Optional.of(TestAllTypes.getDefaultInstance())), + IllegalArgumentException.class, + "Unexpected optional operand for the select chain at: single_int64"), + OPTIONAL_HAS_FIELD_OPTIONAL_BOUND_TO_PLAIN_OPERAND( + "has(msg.?single_nested_message.bb)", + ImmutableMap.of("msg", Optional.empty()), + IllegalArgumentException.class, + "Unexpected optional operand for the select chain at: bb"), + INDEXED_ATTRIBUTE_OPTIONAL_BOUND_TO_PLAIN_OPERAND( + "msg.repeated_nested_message[0].bb", + ImmutableMap.of("msg", Optional.of(TestAllTypes.getDefaultInstance())), + IllegalArgumentException.class, + "Unexpected optional operand for the select chain at: repeated_nested_message"), + OPTIONAL_INDEX_OPTIONAL_BOUND_TO_PLAIN_OPERAND( + "msg.repeated_nested_message[?0].bb", + ImmutableMap.of("msg", Optional.of(TestAllTypes.getDefaultInstance())), + IllegalArgumentException.class, + "Unexpected optional operand for the select chain at: repeated_nested_message"); private final String expression; private final ImmutableMap input;