From 945b64e53957cda0470e7a3e6cd46928ad6fe896 Mon Sep 17 00:00:00 2001 From: CEL Dev Team Date: Fri, 23 Oct 2026 11:36:25 -0700 Subject: [PATCH] Evaluate `FieldBackedListImpl` lists as `ParsedRepeatedFieldValue`. `FromLegacyList` converts a `FieldBackedListImpl` over a heap-allocated message to an `UnsafeParsedRepeatedFieldValue`, so repeated fields bound with `BindProtoToActivation` avoid a `CelValue` round trip per element. PiperOrigin-RevId: 1005746075 --- common/BUILD | 4 + common/value.cc | 60 ++++-- common/value.h | 5 +- common/values/legacy_list_value.cc | 30 +++ common/values/legacy_list_value_test.cc | 201 ++++++++++++++++++ common/values/parsed_repeated_field_value.cc | 9 +- .../parsed_repeated_field_value_test.cc | 47 ++++ eval/public/containers/BUILD | 2 + .../internal_field_backed_list_impl.h | 13 ++ 9 files changed, 352 insertions(+), 19 deletions(-) create mode 100644 common/values/legacy_list_value_test.cc diff --git a/common/BUILD b/common/BUILD index e9439cf53..46f4e9858 100644 --- a/common/BUILD +++ b/common/BUILD @@ -764,6 +764,7 @@ cc_library( "//eval/internal:cel_value_equal", "//eval/public:cel_value", "//eval/public:message_wrapper", + "//eval/public/containers:internal_field_backed_list_impl", "//eval/public/structs:cel_proto_wrap_value_to_message", "//eval/public/structs:legacy_type_info_apis", "//eval/public/structs:proto_message_type_adapter", @@ -834,6 +835,8 @@ cc_test( ":value_testing", "//base:attributes", "//eval/public:cel_value", + "//eval/public/containers:internal_field_backed_list_impl", + "//eval/public/structs:cel_proto_wrapper", "//eval/public/structs:proto_message_type_adapter", "//internal:parse_text_proto", "//internal:status_macros", @@ -846,6 +849,7 @@ cc_test( "@com_google_absl//absl/base:nullability", "@com_google_absl//absl/functional:function_ref", "@com_google_absl//absl/hash", + "@com_google_absl//absl/log:absl_check", "@com_google_absl//absl/log:die_if_null", "@com_google_absl//absl/status", "@com_google_absl//absl/status:status_matchers", diff --git a/common/value.cc b/common/value.cc index bc8a3d7a0..8421dfeb4 100644 --- a/common/value.cc +++ b/common/value.cc @@ -935,6 +935,9 @@ void BoolRepeatedFieldAccessor( *result = BoolValue(reflection->GetRepeatedBool(*message, field, index)); } +// If `Unsafe` is `std::true_type`, elements are borrowed rather than copied to +// `arena`, as with `Value::WrapRepeatedFieldUnsafe`. +template void StringRepeatedFieldAccessor( int index, const google::protobuf::Message* absl_nonnull message, const google::protobuf::FieldDescriptor* absl_nonnull field, @@ -964,10 +967,15 @@ void StringRepeatedFieldAccessor( string.size() == scratch.size()) { *result = StringValue::From(std::move(scratch), arena); } else { - if (message->GetArena() == nullptr) { - *result = StringValue::From(string, arena); + if constexpr (Unsafe::value) { + *result = StringValue::WrapUnsafe(string); } else { - *result = StringValue::Wrap(string, message->GetArena()); + if (google::protobuf::Arena* message_arena = message->GetArena(); + message_arena != nullptr) { + *result = StringValue::Wrap(string, message_arena); + } else { + *result = StringValue::From(string, arena); + } } } }, @@ -975,9 +983,10 @@ void StringRepeatedFieldAccessor( *result = StringValue::From(std::move(cord), arena); }), well_known_types::AsVariant(well_known_types::GetRepeatedStringField( - *message, field, index, scratch))); + reflection, *message, field, index, scratch))); } +template void MessageRepeatedFieldAccessor( int index, const google::protobuf::Message* absl_nonnull message, const google::protobuf::FieldDescriptor* absl_nonnull field, @@ -999,11 +1008,18 @@ void MessageRepeatedFieldAccessor( ABSL_DCHECK_GE(index, 0); ABSL_DCHECK_LT(index, reflection->FieldSize(*message, field)); - *result = Value::WrapMessage( - &reflection->GetRepeatedMessage(*message, field, index), descriptor_pool, - message_factory, arena); + if constexpr (Unsafe::value) { + *result = Value::WrapMessageUnsafe( + &reflection->GetRepeatedMessage(*message, field, index), + descriptor_pool, message_factory, arena); + } else { + *result = Value::WrapMessage( + &reflection->GetRepeatedMessage(*message, field, index), + descriptor_pool, message_factory, arena); + } } +template void BytesRepeatedFieldAccessor( int index, const google::protobuf::Message* absl_nonnull message, const google::protobuf::FieldDescriptor* absl_nonnull field, @@ -1033,10 +1049,15 @@ void BytesRepeatedFieldAccessor( string.size() == scratch.size()) { *result = BytesValue::From(std::move(scratch), arena); } else { - if (message->GetArena() == nullptr) { - *result = BytesValue::From(string, arena); + if constexpr (Unsafe::value) { + *result = BytesValue::WrapUnsafe(string); } else { - *result = BytesValue::Wrap(string, message->GetArena()); + if (google::protobuf::Arena* message_arena = message->GetArena(); + message_arena != nullptr) { + *result = BytesValue::Wrap(string, message_arena); + } else { + *result = BytesValue::From(string, arena); + } } } }, @@ -1044,7 +1065,7 @@ void BytesRepeatedFieldAccessor( *result = BytesValue::From(std::move(cord), arena); }), well_known_types::AsVariant(well_known_types::GetRepeatedBytesField( - *message, field, index, scratch))); + reflection, *message, field, index, scratch))); } void EnumRepeatedFieldAccessor( @@ -1101,7 +1122,7 @@ void NullRepeatedFieldAccessor( } // namespace absl::StatusOr RepeatedFieldAccessorFor( - const google::protobuf::FieldDescriptor* absl_nonnull field) { + const google::protobuf::FieldDescriptor* absl_nonnull field, bool unsafe) { switch (field->type()) { case google::protobuf::FieldDescriptor::TYPE_DOUBLE: return &DoubleRepeatedFieldAccessor; @@ -1126,13 +1147,22 @@ absl::StatusOr RepeatedFieldAccessorFor( case google::protobuf::FieldDescriptor::TYPE_BOOL: return &BoolRepeatedFieldAccessor; case google::protobuf::FieldDescriptor::TYPE_STRING: - return &StringRepeatedFieldAccessor; + if (unsafe) { + return &StringRepeatedFieldAccessor; + } + return &StringRepeatedFieldAccessor; case google::protobuf::FieldDescriptor::TYPE_GROUP: ABSL_FALLTHROUGH_INTENDED; case google::protobuf::FieldDescriptor::TYPE_MESSAGE: - return &MessageRepeatedFieldAccessor; + if (unsafe) { + return &MessageRepeatedFieldAccessor; + } + return &MessageRepeatedFieldAccessor; case google::protobuf::FieldDescriptor::TYPE_BYTES: - return &BytesRepeatedFieldAccessor; + if (unsafe) { + return &BytesRepeatedFieldAccessor; + } + return &BytesRepeatedFieldAccessor; case google::protobuf::FieldDescriptor::TYPE_FIXED32: ABSL_FALLTHROUGH_INTENDED; case google::protobuf::FieldDescriptor::TYPE_UINT32: diff --git a/common/value.h b/common/value.h index fc7992c49..1757c5b03 100644 --- a/common/value.h +++ b/common/value.h @@ -2888,8 +2888,11 @@ using RepeatedFieldAccessor = google::protobuf::MessageFactory* absl_nonnull, google::protobuf::Arena* absl_nonnull, Value* absl_nonnull); +// If `unsafe` is true, message, string, and bytes elements are borrowed rather +// than copied, as with `Value::WrapRepeatedFieldUnsafe`. Caller is responsible +// for ensuring the resulting values do not outlive the input message. absl::StatusOr RepeatedFieldAccessorFor( - const google::protobuf::FieldDescriptor* absl_nonnull field); + const google::protobuf::FieldDescriptor* absl_nonnull field, bool unsafe = false); } // namespace common_internal diff --git a/common/values/legacy_list_value.cc b/common/values/legacy_list_value.cc index 50bf1e6fe..06ec1141c 100644 --- a/common/values/legacy_list_value.cc +++ b/common/values/legacy_list_value.cc @@ -30,6 +30,7 @@ #include "common/values/list_value_builder.h" #include "common/values/values.h" #include "eval/public/cel_value.h" +#include "eval/public/containers/internal_field_backed_list_impl.h" #include "internal/casts.h" #include "internal/status_macros.h" #include "google/protobuf/arena.h" @@ -300,10 +301,39 @@ CustomListValue WrapLegacyParsedJsonList(ParsedJsonListValue value, arena); } +namespace { + +// Returns true if `list` can be replaced by an `UnsafeParsedRepeatedFieldValue` +// over the same field without changing the elements it produces. +bool CanViewAsParsedRepeatedField( + const google::api::expr::runtime::internal::FieldBackedListImpl& list) { + // Elements of arena messages would be copied rather than borrowed. + if (list.message()->GetArena() != nullptr) { + return false; + } + // `FieldBackedListImpl` produces integers for `google.protobuf.NullValue`. + const google::protobuf::FieldDescriptor* field = list.field_descriptor(); + return field->cpp_type() != google::protobuf::FieldDescriptor::CPPTYPE_ENUM || + field->enum_type()->full_name() != "google.protobuf.NullValue"; +} + +} // namespace + ListValue FromLegacyList( const google::api::expr::runtime::CelList* absl_nullable cel_list) { if (cel_list != nullptr) { NativeTypeId native_type_id = NativeTypeId::Of(*cel_list); + if (native_type_id == + NativeTypeId::For< + google::api::expr::runtime::internal::FieldBackedListImpl>()) { + const auto* impl = cel::internal::down_cast< + const google::api::expr::runtime::internal::FieldBackedListImpl*>( + cel_list); + if (CanViewAsParsedRepeatedField(*impl)) { + return UnsafeParsedRepeatedFieldValue(impl->message(), + impl->field_descriptor()); + } + } if (native_type_id == NativeTypeId::For()) { const auto* impl = diff --git a/common/values/legacy_list_value_test.cc b/common/values/legacy_list_value_test.cc new file mode 100644 index 000000000..efe8fed36 --- /dev/null +++ b/common/values/legacy_list_value_test.cc @@ -0,0 +1,201 @@ +// 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. + +#include "common/values/legacy_list_value.h" + +#include +#include + +#include "google/protobuf/struct.pb.h" +#include "absl/log/absl_check.h" +#include "absl/status/status_matchers.h" +#include "absl/strings/string_view.h" +#include "common/legacy_value.h" +#include "common/memory.h" +#include "common/value.h" +#include "common/value_testing.h" +#include "eval/public/cel_value.h" +#include "eval/public/containers/internal_field_backed_list_impl.h" +#include "eval/public/structs/cel_proto_wrapper.h" +#include "internal/testing.h" +#include "runtime/runtime_options.h" +#include "cel/expr/conformance/proto3/test_all_types.pb.h" +#include "google/protobuf/arena.h" +#include "google/protobuf/descriptor.h" + +namespace cel { +namespace { + +using ::absl_testing::IsOk; +using ::cel::expr::conformance::proto3::TestAllTypes; +using ::cel::test::BoolValueIs; +using ::cel::test::IntValueIs; +using ::google::api::expr::runtime::CelList; +using ::google::api::expr::runtime::CelProtoWrapper; +using ::google::api::expr::runtime::CelValue; +using ::google::api::expr::runtime::internal::FieldBackedListImpl; +using ::testing::SizeIs; + +class LegacyListValueTest : public common_internal::ValueTest<> { + protected: + void SetUp() override { Populate(message_); } + + static void Populate(TestAllTypes& message) { + message.add_repeated_bool(true); + message.add_repeated_bool(false); + message.add_repeated_int32(-1); + message.add_repeated_int32(2); + message.add_repeated_int64(-3); + message.add_repeated_uint32(4); + message.add_repeated_uint64(5); + message.add_repeated_float(6.5); + message.add_repeated_double(7.5); + message.add_repeated_string("foo"); + message.add_repeated_string("bar"); + message.add_repeated_bytes("baz"); + message.add_repeated_nested_enum(TestAllTypes::BAR); + message.add_repeated_nested_message()->set_bb(1); + message.add_repeated_nested_message()->set_bb(2); + message.add_repeated_int64_wrapper()->set_value(8); + message.add_repeated_timestamp()->set_seconds(9); + message.add_repeated_null_value(google::protobuf::NULL_VALUE); + } + + const FieldBackedListImpl* MakeList(const TestAllTypes& message, + absl::string_view field_name) { + const google::protobuf::FieldDescriptor* field = + message.GetDescriptor()->FindFieldByName(field_name); + ABSL_CHECK(field != nullptr) << field_name; + return google::protobuf::Arena::Create( + arena(), &message, field, &CelProtoWrapper::InternalWrapMessage, + arena()); + } + + Value ToModern(const CelList* list) { + Value list_value; + ABSL_CHECK_OK(ModernValue(arena(), CelValue::CreateList(list), list_value)); + return list_value; + } + + // Returns the elements produced by iterating `list_value` with `Next1`. + std::vector Iterate(const Value& list_value) { + auto iterator = list_value.GetList().NewIterator(); + ABSL_CHECK_OK(iterator); + std::vector elements; + Value element; + while (true) { + auto ok = (*iterator)->Next1(descriptor_pool(), message_factory(), + arena(), &element); + ABSL_CHECK_OK(ok); + if (!*ok) { + break; + } + elements.push_back(element); + } + return elements; + } + + // Returns the elements of `list` as converted by `ModernValue`, which is the + // legacy behavior. + std::vector LegacyElements(const CelList* list) { + std::vector elements; + for (int i = 0; i < list->size(); ++i) { + Value element; + ABSL_CHECK_OK(ModernValue(arena(), list->Get(arena(), i), element)); + elements.push_back(element); + } + return elements; + } + + TestAllTypes message_; +}; + +TEST_F(LegacyListValueTest, FieldBackedListBecomesParsedRepeatedField) { + Value list_value = ToModern(MakeList(message_, "repeated_nested_message")); + EXPECT_TRUE(list_value.IsParsedRepeatedField()); +} + +TEST_F(LegacyListValueTest, FieldBackedListMatchesLegacyElements) { + for (absl::string_view field_name : { + "repeated_bool", + "repeated_int32", + "repeated_int64", + "repeated_uint32", + "repeated_uint64", + "repeated_float", + "repeated_double", + "repeated_string", + "repeated_bytes", + "repeated_nested_enum", + "repeated_nested_message", + "repeated_int64_wrapper", + "repeated_timestamp", + "repeated_any", + "repeated_sint32", + }) { + SCOPED_TRACE(field_name); + const FieldBackedListImpl* list = MakeList(message_, field_name); + std::vector actual = Iterate(ToModern(list)); + std::vector expected = LegacyElements(list); + ASSERT_THAT(actual, SizeIs(expected.size())); + for (size_t i = 0; i < actual.size(); ++i) { + EXPECT_EQ(actual[i].kind(), expected[i].kind()); + Value equal; + ASSERT_THAT(actual[i].Equal(expected[i], descriptor_pool(), + message_factory(), arena(), &equal), + IsOk()); + EXPECT_THAT(equal, BoolValueIs(true)) + << actual[i].DebugString() << " vs " << expected[i].DebugString(); + } + } +} + +TEST_F(LegacyListValueTest, FieldBackedListMessageElementsAreBorrowed) { + std::vector actual = + Iterate(ToModern(MakeList(message_, "repeated_nested_message"))); + ASSERT_THAT(actual, SizeIs(2)); + for (int i = 0; i < 2; ++i) { + ASSERT_TRUE(actual[i].IsParsedMessage()); + EXPECT_EQ(cel::to_address(actual[i].GetParsedMessage()), + &message_.repeated_nested_message(i)); + Value bb; + ASSERT_THAT(actual[i].GetParsedMessage().GetFieldByName( + "bb", ProtoWrapperTypeOptions::kUnsetProtoDefault, + descriptor_pool(), message_factory(), arena(), &bb), + IsOk()); + EXPECT_THAT(bb, IntValueIs(i + 1)); + } +} + +TEST_F(LegacyListValueTest, FieldBackedListNullValueStaysLegacy) { + const FieldBackedListImpl* list = MakeList(message_, "repeated_null_value"); + Value list_value = ToModern(list); + EXPECT_FALSE(list_value.IsParsedRepeatedField()); + std::vector actual = Iterate(list_value); + ASSERT_THAT(actual, SizeIs(1)); + EXPECT_THAT(actual[0], IntValueIs(0)); +} + +TEST_F(LegacyListValueTest, FieldBackedListOnArenaStaysLegacy) { + auto* arena_message = google::protobuf::Arena::Create(arena()); + Populate(*arena_message); + const FieldBackedListImpl* list = + MakeList(*arena_message, "repeated_nested_message"); + Value list_value = ToModern(list); + EXPECT_FALSE(list_value.IsParsedRepeatedField()); + EXPECT_THAT(Iterate(list_value), SizeIs(2)); +} + +} // namespace +} // namespace cel diff --git a/common/values/parsed_repeated_field_value.cc b/common/values/parsed_repeated_field_value.cc index b716962cc..632fded09 100644 --- a/common/values/parsed_repeated_field_value.cc +++ b/common/values/parsed_repeated_field_value.cc @@ -274,7 +274,8 @@ absl::Status ParsedRepeatedFieldValue::ForEach( const int size = reflection->FieldSize(*message_, field_); if (size > 0) { CEL_ASSIGN_OR_RETURN(auto accessor, - common_internal::RepeatedFieldAccessorFor(field_)); + common_internal::RepeatedFieldAccessorFor( + field_, /*unsafe=*/arena_ == nullptr)); Value scratch; for (int i = 0; i < size; ++i) { (*accessor)(i, message_, field_, reflection, descriptor_pool, @@ -378,7 +379,8 @@ ParsedRepeatedFieldValue::NewIterator() const { return NewEmptyValueIterator(); } CEL_ASSIGN_OR_RETURN(auto accessor, - common_internal::RepeatedFieldAccessorFor(field_)); + common_internal::RepeatedFieldAccessorFor( + field_, /*unsafe=*/arena_ == nullptr)); return std::make_unique(message_, field_, accessor); } @@ -397,7 +399,8 @@ absl::Status ParsedRepeatedFieldValue::Contains( const int size = reflection->FieldSize(*message_, field_); if (size > 0) { CEL_ASSIGN_OR_RETURN(auto accessor, - common_internal::RepeatedFieldAccessorFor(field_)); + common_internal::RepeatedFieldAccessorFor( + field_, /*unsafe=*/arena_ == nullptr)); Value scratch; for (int i = 0; i < size; ++i) { (*accessor)(i, message_, field_, reflection, descriptor_pool, diff --git a/common/values/parsed_repeated_field_value_test.cc b/common/values/parsed_repeated_field_value_test.cc index bb50bee55..f157b2c67 100644 --- a/common/values/parsed_repeated_field_value_test.cc +++ b/common/values/parsed_repeated_field_value_test.cc @@ -14,6 +14,7 @@ #include #include +#include #include #include @@ -23,6 +24,7 @@ #include "absl/status/statusor.h" #include "absl/strings/string_view.h" #include "absl/time/time.h" +#include "absl/types/optional.h" #include "common/memory.h" #include "common/type.h" #include "common/value.h" @@ -416,6 +418,51 @@ TEST_F(ParsedRepeatedFieldValueTest, NewIterator2) { IsOkAndHolds(Eq(std::nullopt))); } +TEST_F(ParsedRepeatedFieldValueTest, NewIteratorUnsafeBorrowsMessages) { + TestAllTypesProto3 message; + message.add_repeated_nested_message()->set_bb(1); + message.add_repeated_nested_message()->set_bb(2); + ParsedRepeatedFieldValue value = UnsafeParsedRepeatedFieldValue( + &message, TestAllTypesProto3::descriptor()->FindFieldByName( + "repeated_nested_message")); + ASSERT_OK_AND_ASSIGN(auto iterator, value.NewIterator()); + for (int i = 0; i < 2; ++i) { + ASSERT_OK_AND_ASSIGN( + auto element, + iterator->Next1(descriptor_pool(), message_factory(), arena())); + ASSERT_TRUE(element.has_value()); + ASSERT_TRUE(element->IsParsedMessage()); + EXPECT_EQ(cel::to_address(element->GetParsedMessage()), + &message.repeated_nested_message(i)); + } +} + +TEST_F(ParsedRepeatedFieldValueTest, ForEachUnsafeBorrowsStrings) { + TestAllTypesProto3 message; + message.add_repeated_string(std::string(64, 'a')); + message.add_repeated_bytes(std::string(64, 'b')); + std::vector> flat; + for (absl::string_view field_name : {"repeated_string", "repeated_bytes"}) { + ParsedRepeatedFieldValue value = UnsafeParsedRepeatedFieldValue( + &message, + TestAllTypesProto3::descriptor()->FindFieldByName(field_name)); + EXPECT_THAT(value.ForEach( + [&](const Value& element) -> absl::StatusOr { + if (element.IsString()) { + flat.push_back(element.GetString().TryFlat()); + } else { + flat.push_back(element.GetBytes().TryFlat()); + } + return true; + }, + descriptor_pool(), message_factory(), arena()), + IsOk()); + } + ASSERT_THAT(flat, ElementsAre(Optional(_), Optional(_))); + EXPECT_EQ(flat[0]->data(), message.repeated_string(0).data()); + EXPECT_EQ(flat[1]->data(), message.repeated_bytes(0).data()); +} + TEST_F(ParsedRepeatedFieldValueTest, Contains) { ParsedRepeatedFieldValue value( DynamicParseTextProto(R"pb(repeated_bool: true)pb"), diff --git a/eval/public/containers/BUILD b/eval/public/containers/BUILD index 54dbf6189..3523b0ca6 100644 --- a/eval/public/containers/BUILD +++ b/eval/public/containers/BUILD @@ -174,9 +174,11 @@ cc_library( "internal_field_backed_list_impl.h", ], deps = [ + "//common:native_type", "//eval/public:cel_value", "//eval/public/structs:field_access_impl", "//eval/public/structs:protobuf_value_factory", + "@com_google_protobuf//:protobuf", ], ) diff --git a/eval/public/containers/internal_field_backed_list_impl.h b/eval/public/containers/internal_field_backed_list_impl.h index 95f8de425..804437f28 100644 --- a/eval/public/containers/internal_field_backed_list_impl.h +++ b/eval/public/containers/internal_field_backed_list_impl.h @@ -17,8 +17,11 @@ #include +#include "common/native_type.h" #include "eval/public/cel_value.h" #include "eval/public/structs/protobuf_value_factory.h" +#include "google/protobuf/descriptor.h" +#include "google/protobuf/message.h" namespace google::api::expr::runtime::internal { @@ -46,7 +49,17 @@ class FieldBackedListImpl : public CelList { // List element access operator. CelValue operator[](int index) const override; + // Message and repeated field backing the list. + const google::protobuf::Message* message() const { return message_; } + const google::protobuf::FieldDescriptor* field_descriptor() const { + return descriptor_; + } + private: + cel::NativeTypeId GetNativeTypeId() const override { + return cel::NativeTypeId::For(); + } + const google::protobuf::Message* message_; const google::protobuf::FieldDescriptor* descriptor_; const google::protobuf::Reflection* reflection_;