Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -156,10 +156,6 @@ private static Object decodeWireField(
}

int typeCode = field.typeCode();
if (typeCode == SelectField.CEL_MAP_TYPE_CODE) {
throw new UnsupportedOperationException(
"Decoding unknown map field from wire bytes is unsupported: " + field.fieldName());
}
if (typeCode == SelectField.NO_TYPE_CODE) {
throw CelAttributeNotFoundException.forFieldResolution(field.fieldName());
}
Expand Down
2 changes: 2 additions & 0 deletions common/src/main/java/dev/cel/common/values/SelectField.java
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ public static SelectField create(
long typeCode,
@Nullable Object defaultValue,
String protoTypeName) {
checkArgument(
typeCode != CEL_MAP_TYPE_CODE, "Map fields must be created via createMap: %s", fieldName);
return newInstance(
fieldNumber, fieldName, typeCode, defaultValue, protoTypeName, /* mapEntrySpec= */ null);
}
Expand Down
10 changes: 10 additions & 0 deletions common/src/test/java/dev/cel/common/values/SelectFieldTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,16 @@ public void create_typeCodeGroupProto_throwsIllegalArgumentException() {
assertThat(thrown).hasMessageThat().contains("Invalid protobuf type code: 10");
}

@Test
public void create_mapTypeCode_throwsIllegalArgumentException() {
IllegalArgumentException thrown =
assertThrows(
IllegalArgumentException.class,
() -> SelectField.create(1L, "foo", SelectField.CEL_MAP_TYPE_CODE));

assertThat(thrown).hasMessageThat().contains("Map fields must be created via createMap: foo");
}

@Test
public void createMap_threeArguments_success() {
SelectField.MapEntrySpec mapEntrySpec = SelectField.MapEntrySpec.create(9, 9);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,6 @@ java_library(
":default_optimizer_constants",
"//:auto_value",
"//bundle:cel",
"//checker:standard_decl",
"//common:cel_ast",
"//common:cel_descriptor_util",
"//common:cel_descriptors",
Expand All @@ -134,7 +133,6 @@ java_library(
"//common/ast:mutable_expr",
"//common/internal:cel_descriptor_pools",
"//common/navigation:common",
"//common/navigation:expr_util",
"//common/navigation:mutable_navigation",
"//common/types",
"//common/types:cel_types",
Expand Down
240 changes: 116 additions & 124 deletions optimizer/src/main/java/dev/cel/optimizer/optimizers/SelectOptimizer.java

Large diffs are not rendered by default.

Large diffs are not rendered by default.

322 changes: 144 additions & 178 deletions runtime/src/main/java/dev/cel/runtime/planner/OptimizedSelectPlanner.java

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

import static com.google.common.truth.Truth.assertThat;
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.junit.Assert.assertThrows;

import com.google.api.expr.v1alpha1.CheckedExpr;
import com.google.common.collect.ImmutableList;
Expand Down Expand Up @@ -512,14 +511,12 @@ public void select_unsetUnknownMapField_returnsBakedDefault() throws Exception {
}

@Test
public void select_populatedUnknownMapField_throwsEvaluationException() {
public void select_populatedUnknownMapField_decodesMapFromWire() throws Exception {
TestAllTypes msg = TestAllTypes.newBuilder().putMapInt32Int32(1, 2).build();

CelEvaluationException thrown =
assertThrows(CelEvaluationException.class, () -> eval("msg.map_int32_int32", msg));
Object result = eval("msg.map_int32_int32", msg);

assertThat(thrown).hasCauseThat().isInstanceOf(UnsupportedOperationException.class);
assertThat(thrown).hasCauseThat().hasMessageThat().contains("map_int32_int32");
assertThat(result).isEqualTo(ImmutableMap.of(1L, 2L));
}

@SuppressWarnings("ImmutableEnumChecker") // Test only
Expand Down Expand Up @@ -944,17 +941,6 @@ public void celBind_unknownSubmessageWithUnknownInnerField_whenUnset_evaluatesDe
assertThat(result).isEqualTo(-1L);
}

@Test
public void shortCircuiting_hasGuardOnPopulatedUnknownMap_skipsMapEvaluation() throws Exception {
// msg.map_int32_int32 is populated on POPULATED_SERVER_MESSAGE and would throw
// UnsupportedOperationException if selected, but short-circuiting avoids evaluating it.
Object result =
eval(
"!has(msg.map_int32_int32) ? size(msg.map_int32_int32) : 99", POPULATED_SERVER_MESSAGE);

assertThat(result).isEqualTo(99L);
}

@Test
public void shortCircuiting_hasGuardOnUnsetUnknownMap_evaluatesEmptyMapSize() throws Exception {
Object result =
Expand Down Expand Up @@ -1450,21 +1436,17 @@ public void celBind_unknownOuterSubmessageWithKnownInnerMapFields_decodesMapEntr
.setOneofType(NestedTestAllTypes.newBuilder().setPayload(POPULATED_RENAMED_MESSAGE))
.build();

// TODO: Restore assertion once SelectOptimizer emits MapEntrySpec in cl/990623531.
CelEvaluationException thrown =
assertThrows(
CelEvaluationException.class,
() ->
eval(
"cel.bind(sub, msg.oneof_type,"
+ " has(sub.payload.map_string_string) &&"
+ " sub.payload.map_string_string['k'] == 'v' &&"
+ " has(sub.payload.map_string_string.k) &&"
+ " !has(sub.payload.map_string_string.missing) &&"
+ " sub.payload.map_int64_message[1].bb == 100)",
msg));
Object result =
eval(
"cel.bind(sub, msg.oneof_type,"
+ " has(sub.payload.map_string_string) &&"
+ " sub.payload.map_string_string['k'] == 'v' &&"
+ " has(sub.payload.map_string_string.k) &&"
+ " !has(sub.payload.map_string_string.missing) &&"
+ " sub.payload.map_int64_message[1].bb == 100)",
msg);

assertThat(thrown).hasCauseThat().isInstanceOf(UnsupportedOperationException.class);
assertThat(result).isEqualTo(true);
}

@Test
Expand Down Expand Up @@ -1495,20 +1477,16 @@ public void celBind_unknownOuterSubmessageWithKnownInnerMapFields_decodesMapEntr
.setOneofType(NestedTestAllTypes.newBuilder().setPayload(innerPayload))
.build();

// TODO: Restore assertion once SelectOptimizer emits MapEntrySpec in cl/990623531.
CelEvaluationException thrown =
assertThrows(
CelEvaluationException.class,
() ->
eval(
"cel.bind(sub, msg.oneof_type,"
+ " size(sub.payload.map_string_string) == 3 &&"
+ " sub.payload.map_string_string['dup'] == 'second' &&"
+ " sub.payload.map_string_string[''] == 'val_for_default_key' &&"
+ " sub.payload.map_string_string['key_with_default_val'] == '')",
msg));
Object result =
eval(
"cel.bind(sub, msg.oneof_type,"
+ " size(sub.payload.map_string_string) == 3 &&"
+ " sub.payload.map_string_string['dup'] == 'second' &&"
+ " sub.payload.map_string_string[''] == 'val_for_default_key' &&"
+ " sub.payload.map_string_string['key_with_default_val'] == '')",
msg);

assertThat(thrown).hasCauseThat().isInstanceOf(UnsupportedOperationException.class);
assertThat(result).isEqualTo(true);
}

@SuppressWarnings("ImmutableEnumChecker") // Test only
Expand Down Expand Up @@ -1692,21 +1670,6 @@ public void wellKnownTypes_negativeDurationAndPreEpochTimestampOnUnknownFields_d
assertThat(result).isEqualTo(true);
}

@Test
public void comprehension_shortCircuitingPreventsEvaluatingPopulatedUnknownMap()
throws Exception {
// msg.map_int32_int32 is populated on POPULATED_SERVER_MESSAGE and throws
// UnsupportedOperationException if evaluated; short-circuiting on the first element (bb == 10)
// must avoid evaluating the map branch.
String expression =
"msg.repeated_nested_message.exists(x, x.bb == 10 || size(msg.map_int32_int32) > 0) &&"
+ " !msg.repeated_nested_message.all(x, x.bb > 15 && size(msg.map_int32_int32) > 0)";

Object result = eval(expression, POPULATED_SERVER_MESSAGE);

assertThat(result).isEqualTo(true);
}

@Test
public void heterogeneousNumericComparisons_onUnknownWireDecodedFields_evaluatesCorrectly()
throws Exception {
Expand Down
Loading
Loading