From d320da5fb7fe345a81807499de595b73bb7cdbaf Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Wed, 26 Aug 2026 15:08:58 -0700 Subject: [PATCH] Add option to switch internal reflection implementation to new version. Update legacy get field and qualify implementations to use new proto reflection. Uses ParsedMessageValue for field access and qualify on legacy struct values, wrapping repeated and map field containers with the legacy specialization types. PiperOrigin-RevId: 971526514 --- common/legacy_value.cc | 54 ++++++---- common/legacy_value.h | 4 +- eval/eval/select_step.cc | 73 +++++++------ eval/public/cel_options.cc | 1 + eval/public/cel_options.h | 10 ++ .../proto_message_type_adapter_test.cc | 10 +- extensions/select_optimization.cc | 102 ++++++++++-------- runtime/runtime_options.h | 10 ++ 8 files changed, 164 insertions(+), 100 deletions(-) diff --git a/common/legacy_value.cc b/common/legacy_value.cc index b963e5071..a08b8317a 100644 --- a/common/legacy_value.cc +++ b/common/legacy_value.cc @@ -74,11 +74,9 @@ using ::cel::interop_internal::TrivialTypeInfo; using ::google::api::expr::runtime::CelList; using ::google::api::expr::runtime::CelMap; using ::google::api::expr::runtime::CelValue; -using ::google::api::expr::runtime::CreateCelValueFromField; using ::google::api::expr::runtime::GetGenericProtoTypeInfoInstance; using ::google::api::expr::runtime::LegacyTypeInfoApis; using ::google::api::expr::runtime::MessageWrapper; -using ::google::api::expr::runtime::internal::GetGenericProtoAccessApisInstance; using ::google::api::expr::runtime::internal::MaybeWrapValueToMessage; absl::Status InvalidMapKeyTypeError(ValueKind kind) { @@ -923,17 +921,26 @@ absl::Status LegacyStructValue::GetFieldByName( const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, google::protobuf::MessageFactory* absl_nonnull message_factory, google::protobuf::Arena* absl_nonnull arena, Value* absl_nonnull result) const { - auto message_wrapper = AsMessageWrapper(message_ptr_, legacy_type_info_); if (ABSL_PREDICT_FALSE(legacy_type_info_ == TrivialTypeInfo::GetInstance())) { *result = NoSuchFieldError(name); return absl::OkStatus(); } - CEL_ASSIGN_OR_RETURN(auto cel_value, - GetGenericProtoAccessApisInstance().GetField( - name, message_wrapper, unboxing_options, - MemoryManagerRef::Pooling(arena))); - CEL_RETURN_IF_ERROR(ModernValue(arena, cel_value, *result)); - return absl::OkStatus(); + + ParsedMessageValue parsed_message = UnsafeParsedMessageValue(message_ptr_); + const auto* descriptor = parsed_message.GetDescriptor(); + const auto* field = descriptor->FindFieldByName(name); + if (field == nullptr) { + field = descriptor->file()->pool()->FindExtensionByPrintableName(descriptor, + name); + if (field == nullptr) { + *result = NoSuchFieldError(name); + return absl::OkStatus(); + } + } + + return interop_internal::WrapLegacyMessageField( + message_ptr_, field, unboxing_options, descriptor_pool, message_factory, + arena, result); } absl::Status LegacyStructValue::GetFieldByNumber( @@ -985,7 +992,6 @@ absl::Status LegacyStructValue::Qualify( if (ABSL_PREDICT_FALSE(qualifiers.empty())) { return absl::InvalidArgumentError("invalid select qualifier path."); } - auto message_wrapper = AsMessageWrapper(message_ptr_, legacy_type_info_); if (ABSL_PREDICT_FALSE(legacy_type_info_ == TrivialTypeInfo::GetInstance())) { absl::string_view field_name = absl::visit( absl::Overload( @@ -1000,12 +1006,13 @@ absl::Status LegacyStructValue::Qualify( *count = -1; return absl::OkStatus(); } - CEL_ASSIGN_OR_RETURN(auto legacy_result, - GetGenericProtoAccessApisInstance().Qualify( - qualifiers, message_wrapper, presence_test, - MemoryManager::Pooling(arena))); - CEL_RETURN_IF_ERROR(ModernValue(arena, legacy_result.value, *result)); - *count = legacy_result.qualifier_count; + + ParsedMessageValue parsed_message = UnsafeParsedMessageValue(message_ptr_); + CEL_RETURN_IF_ERROR(parsed_message.Qualify(qualifiers, presence_test, + descriptor_pool, message_factory, + arena, result, count)); + + interop_internal::WrapLegacyFieldAccessResult(arena, result); return absl::OkStatus(); } @@ -1311,12 +1318,17 @@ const google::protobuf::Message* absl_nullable GetLegacyMessage(const Value& val absl::Status WrapLegacyMessageField( const google::protobuf::Message* absl_nonnull message, const google::protobuf::FieldDescriptor* absl_nonnull field_descriptor, - ProtoWrapperTypeOptions unboxing_option, google::protobuf::Arena* arena, + ProtoWrapperTypeOptions unboxing_option, + const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, + google::protobuf::MessageFactory* absl_nonnull message_factory, google::protobuf::Arena* arena, Value* absl_nonnull out) { - CEL_ASSIGN_OR_RETURN(CelValue result, - CreateCelValueFromField(message, field_descriptor, - unboxing_option, arena)); - return ModernValue(arena, result, *out); + ParsedMessageValue parsed_message = UnsafeParsedMessageValue(message); + CEL_RETURN_IF_ERROR(parsed_message.GetField(field_descriptor, unboxing_option, + descriptor_pool, message_factory, + arena, out)); + WrapLegacyFieldAccessResult(arena, out); + + return absl::OkStatus(); } } // namespace interop_internal diff --git a/common/legacy_value.h b/common/legacy_value.h index 5b7140387..a89eb0412 100644 --- a/common/legacy_value.h +++ b/common/legacy_value.h @@ -82,7 +82,9 @@ void WrapLegacyFieldAccessResult(google::protobuf::Arena* absl_nonnull arena, absl::Status WrapLegacyMessageField( const google::protobuf::Message* absl_nonnull message, const google::protobuf::FieldDescriptor* absl_nonnull field_descriptor, - ProtoWrapperTypeOptions unboxing_option, google::protobuf::Arena* arena, + ProtoWrapperTypeOptions unboxing_option, + const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, + google::protobuf::MessageFactory* absl_nonnull message_factory, google::protobuf::Arena* arena, Value* absl_nonnull out); absl::StatusOr FromLegacyValue( diff --git a/eval/eval/select_step.cc b/eval/eval/select_step.cc index a57179017..e7974496b 100644 --- a/eval/eval/select_step.cc +++ b/eval/eval/select_step.cc @@ -86,17 +86,22 @@ absl::Status WrappedStructGet( ProtoWrapperTypeOptions unboxing_option, const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, google::protobuf::MessageFactory* absl_nonnull message_factory, - google::protobuf::Arena* absl_nonnull arena, Value* absl_nonnull result) { - if (const google::protobuf::Message* message = - cel::interop_internal::GetLegacyMessage(target); - message != nullptr) { - CelValue::MessageWrapper message_wrapper( - message, &GetGenericProtoTypeInfoInstance()); - CEL_ASSIGN_OR_RETURN(CelValue cel_value, - internal::GetGenericProtoAccessApisInstance().GetField( - field, message_wrapper, unboxing_option, - cel::MemoryManagerRef::Pooling(arena))); - return cel::ModernValue(arena, cel_value, *result); + google::protobuf::Arena* absl_nonnull arena, + bool enable_use_new_field_select_implementation, + Value* absl_nonnull result) { + if (!enable_use_new_field_select_implementation) { + if (const google::protobuf::Message* message = + cel::interop_internal::GetLegacyMessage(target); + message != nullptr) { + CelValue::MessageWrapper message_wrapper( + message, &GetGenericProtoTypeInfoInstance()); + CEL_ASSIGN_OR_RETURN( + CelValue cel_value, + internal::GetGenericProtoAccessApisInstance().GetField( + field, message_wrapper, unboxing_option, + cel::MemoryManagerRef::Pooling(arena))); + return cel::ModernValue(arena, cel_value, *result); + } } return target.GetStruct().GetFieldByName( field, unboxing_option, descriptor_pool, message_factory, arena, result); @@ -132,7 +137,9 @@ absl::Status PerformGet(const Value& target, absl::string_view field, ProtoWrapperTypeOptions unboxing_option, const google::protobuf::DescriptorPool* descriptor_pool, google::protobuf::MessageFactory* message_factory, - google::protobuf::Arena* arena, Value& result) { + google::protobuf::Arena* arena, + bool enable_use_new_field_select_implementation, + Value& result) { switch (target.kind()) { case ValueKind::kMap: { auto status = target.GetMap().Get(field_value, descriptor_pool, @@ -143,9 +150,9 @@ absl::Status PerformGet(const Value& target, absl::string_view field, return absl::OkStatus(); } case ValueKind::kStruct: { - auto status = - WrappedStructGet(target, field, unboxing_option, descriptor_pool, - message_factory, arena, &result); + auto status = WrappedStructGet( + target, field, unboxing_option, descriptor_pool, message_factory, + arena, enable_use_new_field_select_implementation, &result); if (!status.ok()) { result = ErrorValue(std::move(status)); } @@ -161,7 +168,9 @@ absl::Status PerformOptionalGet(const Value& target, absl::string_view field, ProtoWrapperTypeOptions unboxing_option, const google::protobuf::DescriptorPool* descriptor_pool, google::protobuf::MessageFactory* message_factory, - google::protobuf::Arena* arena, Value& result) { + google::protobuf::Arena* arena, + bool enable_use_new_field_select_implementation, + Value& result) { switch (target.kind()) { case ValueKind::kMap: { CEL_ASSIGN_OR_RETURN( @@ -182,9 +191,9 @@ absl::Status PerformOptionalGet(const Value& target, absl::string_view field, result = OptionalValue::None(); return absl::OkStatus(); } - CEL_RETURN_IF_ERROR(WrappedStructGet(target, field, unboxing_option, - descriptor_pool, message_factory, - arena, &result)); + CEL_RETURN_IF_ERROR(WrappedStructGet( + target, field, unboxing_option, descriptor_pool, message_factory, + arena, enable_use_new_field_select_implementation, &result)); ABSL_DCHECK(!result.IsUnknown()); result = OptionalValue::Of(std::move(result), arena); @@ -247,7 +256,7 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const { optional_arg = arg.GetOptional(); } - if (!(optional_arg || arg->Is() || arg->Is())) { + if (!(optional_arg || arg.IsMap() || arg.IsStruct())) { frame->value_stack().PopAndPush(cel::ErrorValue(InvalidSelectTargetError()), std::move(result_trail)); return absl::OkStatus(); @@ -290,7 +299,8 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const { optional_arg->Value(&value); auto status = PerformOptionalGet( value, field_, field_value_, unboxing_option_, frame->descriptor_pool(), - frame->message_factory(), frame->arena(), result); + frame->message_factory(), frame->arena(), + frame->options().enable_use_new_field_select_implementation, result); if (!status.ok()) { result = ErrorValue(std::move(status)); } @@ -300,7 +310,8 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const { CEL_RETURN_IF_ERROR(PerformGet( arg, field_, field_value_, unboxing_option_, frame->descriptor_pool(), - frame->message_factory(), frame->arena(), result)); + frame->message_factory(), frame->arena(), + frame->options().enable_use_new_field_select_implementation, result)); frame->value_stack().PopAndPush(std::move(result), std::move(result_trail)); return absl::OkStatus(); } @@ -380,19 +391,20 @@ class DirectSelectStep : public DirectExpressionStep { } Value value; optional_arg->Value(&value); - auto status = - PerformOptionalGet(value, field_, field_value_, unboxing_option_, - frame.descriptor_pool(), frame.message_factory(), - frame.arena(), result); + auto status = PerformOptionalGet( + value, field_, field_value_, unboxing_option_, + frame.descriptor_pool(), frame.message_factory(), frame.arena(), + frame.options().enable_use_new_field_select_implementation, result); if (!status.ok()) { result = ErrorValue(std::move(status)); } return absl::OkStatus(); } - return PerformGet(result, field_, field_value_, unboxing_option_, - frame.descriptor_pool(), frame.message_factory(), - frame.arena(), result); + return PerformGet( + result, field_, field_value_, unboxing_option_, frame.descriptor_pool(), + frame.message_factory(), frame.arena(), + frame.options().enable_use_new_field_select_implementation, result); } private: @@ -495,7 +507,8 @@ absl::Status ProtoSelectStep::EvaluateLegacyMessageGetField( return absl::OkStatus(); } return cel::interop_internal::WrapLegacyMessageField( - legacy_message, field_descriptor_, unboxing_option_, frame->arena(), + legacy_message, field_descriptor_, unboxing_option_, + frame->descriptor_pool(), frame->message_factory(), frame->arena(), &frame->value_stack().Peek()); } diff --git a/eval/public/cel_options.cc b/eval/public/cel_options.cc index 93b67ad35..100ef5e01 100644 --- a/eval/public/cel_options.cc +++ b/eval/public/cel_options.cc @@ -45,6 +45,7 @@ cel::RuntimeOptions ConvertToRuntimeOptions(const InterpreterOptions& options) { options.enable_fast_builtins, options.enable_precision_preserving_double_format, options.enable_typed_field_access, + options.enable_use_new_field_select_implementation, }; } diff --git a/eval/public/cel_options.h b/eval/public/cel_options.h index 001990431..b0d9e6db0 100644 --- a/eval/public/cel_options.h +++ b/eval/public/cel_options.h @@ -223,6 +223,16 @@ struct InterpreterOptions { // path for field access when the type is known at plan time, instead of using // the generic field access implementation. bool enable_typed_field_access = false; + + // Temporary flag to gate using a new field selection implementation for + // protos. + // + // For the cel::Runtime APIs, this is a no-op. + // + // For google::api::expr::runtime::CelExpression, this will enable updated + // implementations for field access on protobuf messages, aligned with the + // cel::Value implementation. + bool enable_use_new_field_select_implementation = false; }; // LINT.ThenChange(//depot/google3/runtime/runtime_options.h) diff --git a/eval/public/structs/proto_message_type_adapter_test.cc b/eval/public/structs/proto_message_type_adapter_test.cc index 529052025..b44c17062 100644 --- a/eval/public/structs/proto_message_type_adapter_test.cc +++ b/eval/public/structs/proto_message_type_adapter_test.cc @@ -1188,10 +1188,12 @@ TEST(ProtoMesssageTypeAdapter, InteropFieldAccess) { message.GetDescriptor()->FindFieldByName("string_value"); ASSERT_NE(field, nullptr); cel::Value field_value; - ASSERT_THAT(cel::interop_internal::WrapLegacyMessageField( - &message, field, ProtoWrapperTypeOptions::kUnsetNull, &arena, - &field_value), - IsOk()); + ASSERT_THAT( + cel::interop_internal::WrapLegacyMessageField( + &message, field, ProtoWrapperTypeOptions::kUnsetNull, + google::protobuf::DescriptorPool::generated_pool(), + google::protobuf::MessageFactory::generated_factory(), &arena, &field_value), + IsOk()); EXPECT_THAT(field_value, cel::test::StringValueIs("hello")); } diff --git a/extensions/select_optimization.cc b/extensions/select_optimization.cc index 83ea6abc6..4dcd7d594 100644 --- a/extensions/select_optimization.cc +++ b/extensions/select_optimization.cc @@ -276,26 +276,29 @@ absl::StatusOr MapKeyFromQualifier(const AttributeQualifier& qual, } } -// Helper for StructValue::GetFieldByName. Used for opting out of old reflection -// implementation. +// // Helper for StructValue::GetFieldByName. Used for opting out of old +// reflection implementation. absl::StatusOr WrappedStructGet( const Value& target, absl::string_view field, const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, google::protobuf::MessageFactory* absl_nonnull message_factory, - google::protobuf::Arena* absl_nonnull arena) { - if (const google::protobuf::Message* message = - cel::interop_internal::GetLegacyMessage(target); - message != nullptr) { - CelValue::MessageWrapper message_wrapper( - message, &GetGenericProtoTypeInfoInstance()); - CEL_ASSIGN_OR_RETURN( - CelValue cel_value, - GetGenericProtoAccessApisInstance().GetField( - field, message_wrapper, ProtoWrapperTypeOptions::kUnsetProtoDefault, - MemoryManagerRef::Pooling(arena))); - Value result; - CEL_RETURN_IF_ERROR(cel::ModernValue(arena, cel_value, result)); - return result; + google::protobuf::Arena* absl_nonnull arena, + bool enable_use_new_field_select_implementation) { + if (!enable_use_new_field_select_implementation) { + if (const google::protobuf::Message* message = + cel::interop_internal::GetLegacyMessage(target); + message != nullptr) { + CelValue::MessageWrapper message_wrapper( + message, &GetGenericProtoTypeInfoInstance()); + CEL_ASSIGN_OR_RETURN(CelValue cel_value, + GetGenericProtoAccessApisInstance().GetField( + field, message_wrapper, + ProtoWrapperTypeOptions::kUnsetProtoDefault, + MemoryManagerRef::Pooling(arena))); + Value result; + CEL_RETURN_IF_ERROR(cel::ModernValue(arena, cel_value, result)); + return result; + } } return target.GetStruct().GetFieldByName(field, descriptor_pool, message_factory, arena); @@ -308,20 +311,23 @@ absl::StatusOr> WrappedStructQualify( absl::Span qualifiers, bool presence_test, const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, google::protobuf::MessageFactory* absl_nonnull message_factory, - google::protobuf::Arena* absl_nonnull arena) { - if (const google::protobuf::Message* message = - cel::interop_internal::GetLegacyMessage(struct_value); - message != nullptr) { - CelValue::MessageWrapper message_wrapper( - message, &GetGenericProtoTypeInfoInstance()); - CEL_ASSIGN_OR_RETURN(auto legacy_result, - GetGenericProtoAccessApisInstance().Qualify( - qualifiers, message_wrapper, presence_test, - MemoryManagerRef::Pooling(arena))); - Value result; - CEL_RETURN_IF_ERROR(cel::ModernValue(arena, legacy_result.value, result)); - return std::pair{std::move(result), - legacy_result.qualifier_count}; + google::protobuf::Arena* absl_nonnull arena, + bool enable_use_new_field_select_implementation) { + if (!enable_use_new_field_select_implementation) { + if (const google::protobuf::Message* message = + cel::interop_internal::GetLegacyMessage(struct_value); + message != nullptr) { + CelValue::MessageWrapper message_wrapper( + message, &GetGenericProtoTypeInfoInstance()); + CEL_ASSIGN_OR_RETURN(auto legacy_result, + GetGenericProtoAccessApisInstance().Qualify( + qualifiers, message_wrapper, presence_test, + MemoryManagerRef::Pooling(arena))); + Value result; + CEL_RETURN_IF_ERROR(cel::ModernValue(arena, legacy_result.value, result)); + return std::pair{std::move(result), + legacy_result.qualifier_count}; + } } return struct_value.Qualify(qualifiers, presence_test, descriptor_pool, message_factory, arena); @@ -331,7 +337,8 @@ absl::StatusOr ApplyQualifier( const Value& operand, const SelectQualifier& qualifier, const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, google::protobuf::MessageFactory* absl_nonnull message_factory, - google::protobuf::Arena* absl_nonnull arena) { + google::protobuf::Arena* absl_nonnull arena, + bool enable_use_new_field_select_implementation) { return absl::visit( absl::Overload( [&](const FieldSpecifier& field_specifier) -> absl::StatusOr { @@ -341,7 +348,8 @@ absl::StatusOr ApplyQualifier( "