From a124a217d89bf71c3f61adf91666a39aeae30ab5 Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Wed, 15 Jul 2026 13:39:30 -0700 Subject: [PATCH] Add support for resolving types at plan time. Add an option to prefetch field descriptors when type is known. PiperOrigin-RevId: 948520646 --- conformance/BUILD | 1 + conformance/service.cc | 6 + eval/compiler/BUILD | 1 + eval/compiler/flat_expr_builder.cc | 77 ++++++++++++- eval/eval/BUILD | 10 +- eval/eval/select_step.cc | 118 +++++++++++++++++++- eval/eval/select_step.h | 10 +- eval/eval/select_step_test.cc | 129 ++++++++++++++++++++++ eval/public/cel_options.cc | 50 +++++---- eval/public/cel_options.h | 5 + eval/tests/BUILD | 17 ++- eval/tests/benchmark_test.cc | 85 ++++++++++---- eval/tests/modern_benchmark_test.cc | 104 ++++++++++++----- runtime/internal/BUILD | 1 + runtime/internal/runtime_type_provider.cc | 45 ++------ runtime/internal/runtime_type_provider.h | 5 +- runtime/runtime_options.h | 13 +++ 17 files changed, 554 insertions(+), 123 deletions(-) diff --git a/conformance/BUILD b/conformance/BUILD index 35d554c7b..d72237841 100644 --- a/conformance/BUILD +++ b/conformance/BUILD @@ -64,6 +64,7 @@ cc_library( "//runtime:constant_folding", "//runtime:optional_types", "//runtime:reference_resolver", + "//runtime:regex_precompilation", "//runtime:runtime_options", "//runtime:standard_runtime_builder_factory", "//testutil:test_macros", diff --git a/conformance/service.cc b/conformance/service.cc index d81200cad..4d3815d37 100644 --- a/conformance/service.cc +++ b/conformance/service.cc @@ -76,6 +76,7 @@ #include "runtime/constant_folding.h" #include "runtime/optional_types.h" #include "runtime/reference_resolver.h" +#include "runtime/regex_precompilation.h" #include "runtime/runtime.h" #include "runtime/runtime_options.h" #include "runtime/standard_runtime_builder_factory.h" @@ -283,6 +284,7 @@ class LegacyConformanceServiceImpl : public ConformanceServiceInterface { std::cerr << "Enabling optimizations" << std::endl; options.constant_folding = true; options.constant_arena = constant_arena; + options.enable_typed_field_access = true; } if (select_optimization) { @@ -486,6 +488,9 @@ class ModernConformanceServiceImpl : public ConformanceServiceInterface { absl::string_view container) { RuntimeOptions options(options_); options.container = std::string(container); + if (enable_optimizations_) { + options.enable_typed_field_access = true; + } CEL_ASSIGN_OR_RETURN( auto builder, CreateStandardRuntimeBuilder( google::protobuf::DescriptorPool::generated_pool(), options)); @@ -493,6 +498,7 @@ class ModernConformanceServiceImpl : public ConformanceServiceInterface { if (enable_optimizations_) { CEL_RETURN_IF_ERROR(cel::extensions::EnableConstantFolding( builder, google::protobuf::MessageFactory::generated_factory())); + CEL_RETURN_IF_ERROR(cel::extensions::EnableRegexPrecompilation(builder)); } CEL_RETURN_IF_ERROR(cel::EnableReferenceResolver( builder, cel::ReferenceResolverEnabled::kAlways)); diff --git a/eval/compiler/BUILD b/eval/compiler/BUILD index ea4dd46b3..2012abeda 100644 --- a/eval/compiler/BUILD +++ b/eval/compiler/BUILD @@ -109,6 +109,7 @@ cc_library( "//common:expr", "//common:kind", "//common:type", + "//common:type_spec_resolver", "//common:value", "//eval/eval:comprehension_step", "//eval/eval:const_value_step", diff --git a/eval/compiler/flat_expr_builder.cc b/eval/compiler/flat_expr_builder.cc index d1435b4a3..a650657de 100644 --- a/eval/compiler/flat_expr_builder.cc +++ b/eval/compiler/flat_expr_builder.cc @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -30,8 +31,6 @@ #include #include "absl/algorithm/container.h" -#include "absl/base/attributes.h" -#include "absl/base/optimization.h" #include "absl/container/flat_hash_map.h" #include "absl/container/flat_hash_set.h" #include "absl/container/node_hash_map.h" @@ -45,7 +44,6 @@ #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "absl/strings/strip.h" -#include "absl/types/optional.h" #include "absl/types/span.h" #include "absl/types/variant.h" #include "base/ast.h" @@ -59,6 +57,7 @@ #include "common/expr.h" #include "common/kind.h" #include "common/type.h" +#include "common/type_spec_resolver.h" #include "common/value.h" #include "eval/compiler/check_ast_extensions.h" #include "eval/compiler/flat_expr_builder_extensions.h" @@ -528,7 +527,7 @@ class FlatExprVisitor : public cel::AstVisitor { FlatExprVisitor( const Resolver& resolver, const cel::RuntimeOptions& options, std::vector> program_optimizers, - const absl::flat_hash_map& reference_map, + const absl::flat_hash_map& type_map, const cel::TypeProvider& type_provider, IssueCollector& issue_collector, ProgramBuilder& program_builder, PlannerContext& extension_context, bool enable_optional_types) @@ -538,6 +537,7 @@ class FlatExprVisitor : public cel::AstVisitor { resolved_select_expr_(nullptr), options_(options), program_optimizers_(std::move(program_optimizers)), + type_map_(type_map), issue_collector_(issue_collector), program_builder_(program_builder), extension_context_(extension_context), @@ -606,6 +606,21 @@ class FlatExprVisitor : public cel::AstVisitor { bool PlanRecursiveProgram() const { return max_recursion_depth_ > 0; } + void SetResolvedType(const cel::Expr& expr, cel::Type type) { + resolved_types_[&expr] = std::move(type); + } + + std::optional GetResolvedType(const cel::Expr* expr) const { + if (expr == nullptr) { + return std::nullopt; + } + auto it = resolved_types_.find(expr); + if (it != resolved_types_.end()) { + return it->second; + } + return std::nullopt; + } + void PreVisitExpr(const cel::Expr& expr) override { ValidateOrError(!absl::holds_alternative(expr.kind()), "Invalid empty expression"); @@ -617,6 +632,10 @@ class FlatExprVisitor : public cel::AstVisitor { resume_from_suppressed_branch_ = &expr; } + if (options_.enable_typed_field_access) { + MaybeResolveType(expr); + } + if (block_.has_value()) { BlockInfo& block = *block_; if (block.in && block.bindings_set.contains(&expr)) { @@ -977,6 +996,24 @@ class FlatExprVisitor : public cel::AstVisitor { } StringValue field = cel::StringValue(select_expr.field()); + std::optional struct_type; + std::optional field_type; + if (options_.enable_typed_field_access) { + std::optional operand_type = + GetResolvedType(&select_expr.operand()); + if (operand_type.has_value() && operand_type->IsStruct()) { + struct_type = operand_type->GetStruct(); + if (struct_type.has_value()) { + auto field_lookup = + extension_context_.type_reflector().FindStructTypeFieldByName( + *struct_type, select_expr.field()); + // Swallow error to fallback to duck typing behavior. + if (field_lookup.ok() && field_lookup->has_value()) { + field_type = *std::move(field_lookup); + } + } + } + } if (auto depth = RecursionEligible(); depth.has_value()) { auto deps = ExtractRecursiveDependencies(); if (deps.size() != 1) { @@ -994,6 +1031,13 @@ class FlatExprVisitor : public cel::AstVisitor { return; } + if (field_type.has_value()) { + AddStep(CreateTypedSelectStep( + std::move(field), *struct_type, *std::move(field_type), + select_expr.test_only(), expr.id(), + options_.enable_empty_wrapper_null_unboxing, enable_optional_types_)); + return; + } AddStep(CreateSelectStep( std::move(field), select_expr.test_only(), expr.id(), options_.enable_empty_wrapper_null_unboxing, enable_optional_types_)); @@ -1921,6 +1965,8 @@ class FlatExprVisitor : public cel::AstVisitor { CallHandlerResult HandleHeterogeneousEqualityIn(const cel::Expr& expr, const cel::CallExpr& call); + void MaybeResolveType(const cel::Expr& expr); + const Resolver& resolver_; const cel::TypeProvider& type_provider_; absl::Status progress_status_; @@ -1942,6 +1988,8 @@ class FlatExprVisitor : public cel::AstVisitor { absl::flat_hash_set suppressed_branches_; const cel::Expr* resume_from_suppressed_branch_ = nullptr; std::vector> program_optimizers_; + const absl::flat_hash_map& type_map_; + absl::flat_hash_map resolved_types_; IssueCollector& issue_collector_; ProgramBuilder& program_builder_; @@ -2161,6 +2209,23 @@ FlatExprVisitor::HandleHeterogeneousEqualityIn(const cel::Expr& expr, return CallHandlerResult::kIntercepted; } +void FlatExprVisitor::MaybeResolveType(const cel::Expr& expr) { + // Try to resolve the type from the type map, but don't fail if it's not + // there. This permits cases where the runtime type is compatible but not + // the same as the type checked type. + auto it = type_map_.find(expr.id()); + if (it == type_map_.end()) { + return; + } + absl::StatusOr type = cel::ConvertTypeSpecToType( + it->second, extension_context_.type_reflector(), + extension_context_.MutableArena()); + if (!type.ok()) { + return; + } + SetResolvedType(expr, *type); +} + void LogicalCondVisitor::PreVisit(const cel::Expr* expr) { visitor_->ValidateOrError( !expr->call_expr().has_target() && expr->call_expr().args().size() >= 2, @@ -2561,8 +2626,8 @@ absl::StatusOr FlatExprBuilder::CreateExpressionImpl( // These objects are expected to remain scoped to one build call -- references // to them shouldn't be persisted in any part of the result expression. FlatExprVisitor visitor(resolver, options_, std::move(optimizers), - ast->reference_map(), GetTypeProvider(), - issue_collector, program_builder, extension_context, + ast->type_map(), GetTypeProvider(), issue_collector, + program_builder, extension_context, enable_optional_types_); if (options_.max_recursion_depth == -1 || options_.max_recursion_depth > 0) { diff --git a/eval/eval/BUILD b/eval/eval/BUILD index c0f544405..2597563a4 100644 --- a/eval/eval/BUILD +++ b/eval/eval/BUILD @@ -314,13 +314,11 @@ cc_library( ":direct_expression_step", ":evaluator_core", ":expression_step_base", - "//common:expr", + "//common:type", "//common:value", "//common:value_kind", - "//eval/internal:errors", "//internal:status_macros", "//runtime:runtime_options", - "@com_google_absl//absl/base:nullability", "@com_google_absl//absl/log:absl_check", "@com_google_absl//absl/log:absl_log", "@com_google_absl//absl/status", @@ -791,7 +789,9 @@ cc_test( deps = [ ":attribute_trail", ":cel_expression_flat_impl", + ":compiler_constant_step", ":const_value_step", + ":create_map_step", ":evaluator_core", ":ident_step", ":select_step", @@ -800,11 +800,14 @@ cc_test( "//common:casting", "//common:expr", "//common:legacy_value", + "//common:type", "//common:value", "//common:value_testing", "//eval/public:activation", "//eval/public:cel_attribute", "//eval/public:cel_value", + "//eval/public:unknown_attribute_set", + "//eval/public:unknown_set", "//eval/public/containers:container_backed_map_impl", "//eval/public/structs:cel_proto_wrapper", "//eval/public/structs:legacy_type_adapter", @@ -831,6 +834,7 @@ cc_test( "@com_google_absl//absl/strings", "@com_google_cel_spec//proto/cel/expr:syntax_cc_proto", "@com_google_cel_spec//proto/cel/expr/conformance/proto3:test_all_types_cc_proto", + "@com_google_protobuf//:protobuf", "@com_google_protobuf//:wrappers_cc_proto", ], ) diff --git a/eval/eval/select_step.cc b/eval/eval/select_step.cc index 806e20b31..c02c8521f 100644 --- a/eval/eval/select_step.cc +++ b/eval/eval/select_step.cc @@ -11,6 +11,8 @@ #include "absl/status/statusor.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" +#include "common/legacy_value.h" +#include "common/type.h" #include "common/value.h" #include "common/value_kind.h" #include "eval/eval/attribute_trail.h" @@ -182,7 +184,7 @@ class SelectStep : public ExpressionStepBase { absl::Status Evaluate(ExecutionFrame* frame) const override; - private: + protected: cel::StringValue field_value_; std::string field_; bool test_field_presence_; @@ -207,7 +209,7 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const { AttributeTrail result_trail; // Handle unknown resolution. - if (frame->enable_unknowns() || frame->enable_missing_attribute_errors()) { + if (frame->attribute_tracking_enabled()) { result_trail = trail.Step(&field_); } @@ -382,6 +384,93 @@ class DirectSelectStep : public DirectExpressionStep { bool enable_optional_types_; }; +class ProtoSelectStep : public SelectStep { + public: + ProtoSelectStep(StringValue value, int64_t expr_id, + bool enable_wrapper_type_null_unboxing, + bool enable_optional_types, + const google::protobuf::Descriptor* descriptor, + const google::protobuf::FieldDescriptor* field_descriptor) + : SelectStep(std::move(value), /*test_field_presence=*/false, expr_id, + enable_wrapper_type_null_unboxing, enable_optional_types), + descriptor_(descriptor), + field_descriptor_(field_descriptor) { + ABSL_DCHECK(descriptor_ != nullptr); + ABSL_DCHECK(field_descriptor_ != nullptr); + } + + absl::Status Evaluate(ExecutionFrame* frame) const override { + if (!frame->value_stack().HasEnough(1)) { + return absl::InternalError( + "No arguments supplied for Select-type expression"); + } + + const Value& arg = frame->value_stack().Peek(); + if (auto unwrapped = arg.AsParsedMessage(); + unwrapped.has_value() && unwrapped->GetDescriptor() == descriptor_) { + return ModernGet(frame, *unwrapped); + } else if (const google::protobuf::Message* legacy_message = + cel::interop_internal::GetLegacyMessage(arg); + legacy_message != nullptr && + legacy_message->GetDescriptor() == descriptor_) { + // A little unfortunate, but need to special case for legacy values so we + // can minimize back and forth interop conversions. + return LegacyGet(frame, legacy_message); + } + // If we get an unexpected value type, fall back to the generic + // implementation. + return SelectStep::Evaluate(frame); + } + + private: + absl::Status ModernGet(ExecutionFrame* frame, + const cel::ParsedMessageValue& parsed_message) const; + absl::Status LegacyGet(ExecutionFrame* frame, + const google::protobuf::Message* legacy_message) const; + + const google::protobuf::Descriptor* descriptor_; + const google::protobuf::FieldDescriptor* field_descriptor_; +}; + +static bool CheckAttributeTrail(const std::string& field, + ExecutionFrame* frame) { + if (!frame->attribute_tracking_enabled()) { + return false; + } + AttributeTrail& attr = frame->value_stack().PeekAttribute(); + attr = attr.Step(&field); + + absl::optional marked_attribute_check = + CheckForMarkedAttributes(attr, *frame); + if (marked_attribute_check.has_value()) { + frame->value_stack().Peek() = std::move(marked_attribute_check).value(); + return true; + } + + return false; +} + +absl::Status ProtoSelectStep::ModernGet( + ExecutionFrame* frame, + const cel::ParsedMessageValue& parsed_message) const { + if (CheckAttributeTrail(field_, frame)) { + return absl::OkStatus(); + } + return parsed_message.GetField( + field_descriptor_, unboxing_option_, frame->descriptor_pool(), + frame->message_factory(), frame->arena(), &frame->value_stack().Peek()); +} + +absl::Status ProtoSelectStep::LegacyGet( + ExecutionFrame* frame, const google::protobuf::Message* legacy_message) const { + if (CheckAttributeTrail(field_, frame)) { + return absl::OkStatus(); + } + return cel::interop_internal::WrapLegacyMessageField( + legacy_message, field_descriptor_, unboxing_option_, frame->arena(), + &frame->value_stack().Peek()); +} + } // namespace std::unique_ptr CreateDirectSelectStep( @@ -402,4 +491,29 @@ absl::StatusOr> CreateSelectStep( enable_optional_types); } +// Factory method for Select - based Execution step +absl::StatusOr> CreateTypedSelectStep( + cel::StringValue field, cel::StructType resolved_operand_type, + cel::StructTypeField resolved_field, bool test_only, int64_t expr_id, + bool enable_wrapper_type_null_unboxing, bool enable_optional_types) { + if (!resolved_operand_type.IsMessage() || test_only) { + // The specialization only supports messages. Fallback to the generic + // implementation for other types. + // TODO(uncreated-issue/89): support has() for messages. + return CreateSelectStep(std::move(field), test_only, expr_id, + enable_wrapper_type_null_unboxing, + enable_optional_types); + } + const google::protobuf::Descriptor* descriptor = + resolved_operand_type.GetMessage().descriptor(); + + ABSL_DCHECK(resolved_field.IsMessage()); + const google::protobuf::FieldDescriptor* field_descriptor = + resolved_field.GetMessage().descriptor(); + + return std::make_unique( + std::move(field), expr_id, enable_wrapper_type_null_unboxing, + enable_optional_types, descriptor, field_descriptor); +} + } // namespace google::api::expr::runtime diff --git a/eval/eval/select_step.h b/eval/eval/select_step.h index 2fe35ecb0..c3f965a94 100644 --- a/eval/eval/select_step.h +++ b/eval/eval/select_step.h @@ -5,6 +5,7 @@ #include #include "absl/status/statusor.h" +#include "common/type.h" #include "common/value.h" #include "eval/eval/direct_expression_step.h" #include "eval/eval/evaluator_core.h" @@ -17,10 +18,15 @@ std::unique_ptr CreateDirectSelectStep( bool test_only, int64_t expr_id, bool enable_wrapper_type_null_unboxing, bool enable_optional_types = false); -// Factory method for Select - based Execution step +// Factory method for Select stack machine based Execution step absl::StatusOr> CreateSelectStep( cel::StringValue field, bool test_only, int64_t expr_id, - bool enable_wrapper_type_null_unboxing, bool enable_optional_types = false); + bool enable_wrapper_type_null_unboxing, bool enable_optional_ytpes = false); + +absl::StatusOr> CreateTypedSelectStep( + cel::StringValue field, cel::StructType resolved_operand_type, + cel::StructTypeField resolved_field, bool test_only, int64_t expr_id, + bool enable_wrapper_type_null_unboxing, bool enable_optional_types); } // namespace google::api::expr::runtime diff --git a/eval/eval/select_step_test.cc b/eval/eval/select_step_test.cc index d5ca2aecd..1580472ba 100644 --- a/eval/eval/select_step_test.cc +++ b/eval/eval/select_step_test.cc @@ -19,11 +19,14 @@ #include "common/casting.h" #include "common/expr.h" #include "common/legacy_value.h" +#include "common/type.h" #include "common/value.h" #include "common/value_testing.h" #include "eval/eval/attribute_trail.h" #include "eval/eval/cel_expression_flat_impl.h" +#include "eval/eval/compiler_constant_step.h" #include "eval/eval/const_value_step.h" +#include "eval/eval/create_map_step.h" #include "eval/eval/evaluator_core.h" #include "eval/eval/ident_step.h" #include "eval/public/activation.h" @@ -34,6 +37,8 @@ #include "eval/public/structs/legacy_type_adapter.h" #include "eval/public/structs/trivial_legacy_type_info.h" #include "eval/public/testing/matchers.h" +#include "eval/public/unknown_attribute_set.h" +#include "eval/public/unknown_set.h" #include "eval/testutil/test_extensions.pb.h" #include "eval/testutil/test_message.pb.h" #include "extensions/protobuf/value.h" @@ -48,6 +53,7 @@ #include "runtime/internal/runtime_type_provider.h" #include "runtime/runtime_options.h" #include "cel/expr/conformance/proto3/test_all_types.pb.h" +#include "google/protobuf/descriptor.h" namespace google::api::expr::runtime { @@ -1062,6 +1068,129 @@ TEST_F(SelectStepTest, UnknownPatternResolvesToUnknown) { } } +TEST_P(SelectStepConformanceTest, TypedSelectStepTest) { + ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep("message", -1)); + + cel::StructType resolved_operand_type( + (cel::MessageType(TestAllTypes::descriptor()))); + const google::protobuf::FieldDescriptor* field_desc = + TestAllTypes::descriptor()->FindFieldByName("single_int64"); + ASSERT_NE(field_desc, nullptr); + cel::StructTypeField resolved_field((cel::MessageTypeField(field_desc))); + + ASSERT_OK_AND_ASSIGN( + auto step1, + CreateTypedSelectStep(cel::StringValue("single_int64"), + resolved_operand_type, resolved_field, + /*test_only=*/false, -1, + /*enable_wrapper_type_null_unboxing=*/false, + /*enable_optional_types=*/false)); + + ExecutionPath path; + path.push_back(std::move(step0)); + path.push_back(std::move(step1)); + cel::RuntimeOptions options; + if (GetParam()) { + options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; + } + CelExpressionFlatImpl cel_expr( + env_, + FlatExpression(std::move(path), /*comprehension_slot_count=*/0, + env_->type_registry.GetComposedTypeProvider(), options)); + + TestAllTypes message; + message.set_single_int64(42); + Activation activation; + activation.InsertValue("message", + CelProtoWrapper::CreateMessage(&message, &arena_)); + + ASSERT_OK_AND_ASSIGN(CelValue result, cel_expr.Evaluate(activation, &arena_)); + ASSERT_TRUE(result.IsInt64()); + EXPECT_EQ(result.Int64OrDie(), 42); +} + +TEST_P(SelectStepConformanceTest, TypedSelectStepPropagatesUnknown) { + ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep("message", -1)); + + cel::StructType resolved_operand_type( + (cel::MessageType(TestAllTypes::descriptor()))); + const google::protobuf::FieldDescriptor* field_desc = + TestAllTypes::descriptor()->FindFieldByName("single_int64"); + ASSERT_NE(field_desc, nullptr); + cel::StructTypeField resolved_field((cel::MessageTypeField(field_desc))); + + ASSERT_OK_AND_ASSIGN( + auto step1, + CreateTypedSelectStep(cel::StringValue("single_int64"), + resolved_operand_type, resolved_field, + /*test_only=*/false, -1, + /*enable_wrapper_type_null_unboxing=*/false, + /*enable_optional_types=*/false)); + + ExecutionPath path; + path.push_back(std::move(step0)); + path.push_back(std::move(step1)); + cel::RuntimeOptions options; + if (GetParam()) { + options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; + } + CelExpressionFlatImpl cel_expr( + env_, + FlatExpression(std::move(path), /*comprehension_slot_count=*/0, + env_->type_registry.GetComposedTypeProvider(), options)); + + UnknownSet unknown_set(UnknownAttributeSet({CelAttribute("message", {})})); + Activation activation; + activation.InsertValue("message", CelValue::CreateUnknownSet(&unknown_set)); + + ASSERT_OK_AND_ASSIGN(CelValue result, cel_expr.Evaluate(activation, &arena_)); + ASSERT_TRUE(result.IsUnknownSet()); +} + +TEST_F(SelectStepTest, TypedSelectStepUnknownPatternResolvesToUnknown) { + ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep("message", -1)); + + cel::StructType resolved_operand_type( + (cel::MessageType(TestAllTypes::descriptor()))); + const google::protobuf::FieldDescriptor* field_desc = + TestAllTypes::descriptor()->FindFieldByName("single_int64"); + ASSERT_NE(field_desc, nullptr); + cel::StructTypeField resolved_field((cel::MessageTypeField(field_desc))); + + ASSERT_OK_AND_ASSIGN( + auto step1, + CreateTypedSelectStep(cel::StringValue("single_int64"), + resolved_operand_type, resolved_field, + /*test_only=*/false, -1, + /*enable_wrapper_type_null_unboxing=*/false, + /*enable_optional_types=*/false)); + + ExecutionPath path; + path.push_back(std::move(step0)); + path.push_back(std::move(step1)); + cel::RuntimeOptions options; + options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; + CelExpressionFlatImpl cel_expr( + env_, + FlatExpression(std::move(path), /*comprehension_slot_count=*/0, + env_->type_registry.GetComposedTypeProvider(), options)); + + TestAllTypes message; + message.set_single_int64(42); + + std::vector unknown_patterns; + unknown_patterns.push_back(CelAttributePattern( + "message", {CelAttributeQualifierPattern::OfString("single_int64")})); + + Activation activation; + activation.InsertValue("message", + CelProtoWrapper::CreateMessage(&message, &arena_)); + activation.set_unknown_attribute_patterns(unknown_patterns); + + ASSERT_OK_AND_ASSIGN(CelValue result, cel_expr.Evaluate(activation, &arena_)); + ASSERT_TRUE(result.IsUnknownSet()); +} + INSTANTIATE_TEST_SUITE_P(UnknownsEnabled, SelectStepConformanceTest, testing::Bool()); diff --git a/eval/public/cel_options.cc b/eval/public/cel_options.cc index 938b5e96f..93b67ad35 100644 --- a/eval/public/cel_options.cc +++ b/eval/public/cel_options.cc @@ -19,29 +19,33 @@ namespace google::api::expr::runtime { cel::RuntimeOptions ConvertToRuntimeOptions(const InterpreterOptions& options) { - return cel::RuntimeOptions{/*.container=*/"", - options.unknown_processing, - options.enable_missing_attribute_errors, - options.enable_timestamp_duration_overflow_errors, - options.short_circuiting, - options.enable_comprehension, - options.comprehension_max_iterations, - options.enable_comprehension_list_append, - options.enable_comprehension_mutable_map, - options.enable_regex, - options.regex_max_program_size, - options.enable_string_conversion, - options.enable_string_concat, - options.enable_list_concat, - options.enable_list_contains, - options.fail_on_warnings, - options.enable_qualified_type_identifiers, - options.enable_heterogeneous_equality, - options.enable_empty_wrapper_null_unboxing, - options.enable_lazy_bind_initialization, - options.max_recursion_depth, - options.enable_recursive_tracing, - options.enable_fast_builtins}; + return cel::RuntimeOptions{ + /*.container=*/"", + options.unknown_processing, + options.enable_missing_attribute_errors, + options.enable_timestamp_duration_overflow_errors, + options.short_circuiting, + options.enable_comprehension, + options.comprehension_max_iterations, + options.enable_comprehension_list_append, + options.enable_comprehension_mutable_map, + options.enable_regex, + options.regex_max_program_size, + options.enable_string_conversion, + options.enable_string_concat, + options.enable_list_concat, + options.enable_list_contains, + options.fail_on_warnings, + options.enable_qualified_type_identifiers, + options.enable_heterogeneous_equality, + options.enable_empty_wrapper_null_unboxing, + options.enable_lazy_bind_initialization, + options.max_recursion_depth, + options.enable_recursive_tracing, + options.enable_fast_builtins, + options.enable_precision_preserving_double_format, + options.enable_typed_field_access, + }; } } // namespace google::api::expr::runtime diff --git a/eval/public/cel_options.h b/eval/public/cel_options.h index 4d81eb8a7..001990431 100644 --- a/eval/public/cel_options.h +++ b/eval/public/cel_options.h @@ -218,6 +218,11 @@ struct InterpreterOptions { // Otherwise, will fall back to formatting with the worst-case required // precision. bool enable_precision_preserving_double_format = true; + + // When enabled, the planner will attempt to use a more performant execution + // 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; }; // LINT.ThenChange(//depot/google3/runtime/runtime_options.h) diff --git a/eval/tests/BUILD b/eval/tests/BUILD index 9163548d1..c5a7a7062 100644 --- a/eval/tests/BUILD +++ b/eval/tests/BUILD @@ -24,6 +24,12 @@ cc_test( ], deps = [ ":request_context_cc_proto", + "//common:ast_proto", + "//common:decl", + "//common:type", + "//compiler", + "//compiler:compiler_factory", + "//compiler:standard_library", "//eval/public:activation", "//eval/public:builtin_func_registrar", "//eval/public:cel_expr_builder_factory", @@ -42,6 +48,7 @@ cc_test( "@com_google_absl//absl/container:flat_hash_set", "@com_google_absl//absl/container:node_hash_set", "@com_google_absl//absl/flags:flag", + "@com_google_absl//absl/status:status_matchers", "@com_google_absl//absl/strings", "@com_google_cel_spec//proto/cel/expr:syntax_cc_proto", "@com_google_googleapis//google/rpc/context:attribute_context_cc_proto", @@ -61,22 +68,25 @@ cc_test( ], deps = [ ":request_context_cc_proto", + "//checker:validation_result", "//common:allocator", "//common:casting", + "//common:decl", "//common:legacy_value", - "//common:memory", "//common:native_type", + "//common:type", "//common:value", + "//compiler", + "//compiler:compiler_factory", + "//compiler:standard_library", "//extensions:comprehensions_v2_functions", "//extensions:comprehensions_v2_macros", "//extensions/protobuf:runtime_adapter", - "//extensions/protobuf:value", "//internal:benchmark", "//internal:testing", "//internal:testing_descriptor_pool", "//internal:testing_message_factory", "//parser", - "//parser:macro", "//parser:macro_registry", "//runtime", "//runtime:activation", @@ -94,7 +104,6 @@ cc_test( "@com_google_absl//absl/status:status_matchers", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", - "@com_google_absl//absl/types:optional", "@com_google_cel_spec//proto/cel/expr:syntax_cc_proto", "@com_google_googleapis//google/rpc/context:attribute_context_cc_proto", "@com_google_protobuf//:protobuf", diff --git a/eval/tests/benchmark_test.cc b/eval/tests/benchmark_test.cc index f188dc0b7..abeac608a 100644 --- a/eval/tests/benchmark_test.cc +++ b/eval/tests/benchmark_test.cc @@ -12,7 +12,14 @@ #include "absl/container/flat_hash_set.h" #include "absl/container/node_hash_set.h" #include "absl/flags/flag.h" +#include "absl/status/status_matchers.h" #include "absl/strings/match.h" +#include "common/ast_proto.h" +#include "common/decl.h" +#include "common/type.h" +#include "compiler/compiler.h" +#include "compiler/compiler_factory.h" +#include "compiler/standard_library.h" #include "eval/public/activation.h" #include "eval/public/builtin_func_registrar.h" #include "eval/public/cel_expr_builder_factory.h" @@ -31,6 +38,7 @@ ABSL_FLAG(bool, enable_optimizations, false, "enable const folding opt"); ABSL_FLAG(bool, enable_recursive_planning, false, "enable recursive planning"); +ABSL_FLAG(bool, enable_typed_field_access, false, "enable typed field access"); namespace google { namespace api { @@ -39,6 +47,7 @@ namespace runtime { namespace { +using ::absl_testing::IsOk; using ::cel::expr::Expr; using ::cel::expr::ParsedExpr; using ::cel::expr::SourceInfo; @@ -56,6 +65,10 @@ InterpreterOptions GetOptions(google::protobuf::Arena& arena) { options.max_recursion_depth = -1; } + if (absl::GetFlag(FLAGS_enable_typed_field_access)) { + options.enable_typed_field_access = true; + } + return options; } @@ -67,7 +80,8 @@ static void BM_Eval(benchmark::State& state) { InterpreterOptions options = GetOptions(arena); auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); int len = state.range(0); @@ -113,7 +127,8 @@ static void BM_Eval_Trace(benchmark::State& state) { options.enable_recursive_tracing = true; auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); int len = state.range(0); @@ -155,7 +170,8 @@ static void BM_EvalString(benchmark::State& state) { InterpreterOptions options = GetOptions(arena); auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); int len = state.range(0); @@ -198,7 +214,8 @@ static void BM_EvalString_Trace(benchmark::State& state) { options.enable_recursive_tracing = true; auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); int len = state.range(0); @@ -295,7 +312,8 @@ void BM_PolicySymbolic(benchmark::State& state) { options.constant_arena = &arena; auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); SourceInfo source_info; ASSERT_OK_AND_ASSIGN(auto cel_expr, builder->CreateExpression( @@ -351,7 +369,8 @@ void BM_PolicySymbolicMap(benchmark::State& state) { InterpreterOptions options = GetOptions(arena); auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); SourceInfo source_info; ASSERT_OK_AND_ASSIGN(auto cel_expr, builder->CreateExpression( @@ -373,22 +392,37 @@ BENCHMARK(BM_PolicySymbolicMap); // Uses a protobuf container for "ip", "path", and "token". void BM_PolicySymbolicProto(benchmark::State& state) { google::protobuf::Arena arena; - ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, parser::Parse(R"cel( + ASSERT_OK_AND_ASSIGN( + auto compiler_builder, + cel::NewCompilerBuilder(google::protobuf::DescriptorPool::generated_pool())); + ASSERT_THAT(compiler_builder->AddLibrary(cel::StandardCompilerLibrary()), + IsOk()); + ASSERT_THAT( + compiler_builder->GetCheckerBuilder().AddVariable(cel::MakeVariableDecl( + "request", cel::MessageType(RequestContext::descriptor()))), + IsOk()); + ASSERT_OK_AND_ASSIGN(auto compiler, compiler_builder->Build()); + + ASSERT_OK_AND_ASSIGN(auto validation_result, compiler->Compile(R"cel( !(request.ip in ["10.0.1.4", "10.0.1.5", "10.0.1.6"]) && ((request.path.startsWith("v1") && request.token in ["v1", "v2", "admin"]) || (request.path.startsWith("v2") && request.token in ["v2", "admin"]) || (request.path.startsWith("/admin") && request.token == "admin" && request.ip in ["10.0.1.1", "10.0.1.2", "10.0.1.3"]) ))cel")); + ASSERT_TRUE(validation_result.IsValid()); + ASSERT_OK_AND_ASSIGN(auto ast, validation_result.ReleaseAst()); + + cel::expr::CheckedExpr checked_expr; + ASSERT_THAT(cel::AstToCheckedExpr(*ast, &checked_expr), IsOk()); InterpreterOptions options = GetOptions(arena); auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); - SourceInfo source_info; - ASSERT_OK_AND_ASSIGN(auto cel_expr, builder->CreateExpression( - &parsed_expr.expr(), &source_info)); + ASSERT_OK_AND_ASSIGN(auto cel_expr, builder->CreateExpression(&checked_expr)); Activation activation; RequestContext request; @@ -475,7 +509,8 @@ void BM_Comprehension(benchmark::State& state) { InterpreterOptions options = GetOptions(arena); options.comprehension_max_iterations = 10000000; auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); ASSERT_OK_AND_ASSIGN(auto cel_expr, builder->CreateExpression(&expr, nullptr)); @@ -509,7 +544,8 @@ void BM_Comprehension_Trace(benchmark::State& state) { options.comprehension_max_iterations = 10000000; auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); ASSERT_OK_AND_ASSIGN(auto cel_expr, builder->CreateExpression(&expr, nullptr)); @@ -531,7 +567,8 @@ void BM_HasMap(benchmark::State& state) { InterpreterOptions options = GetOptions(arena); auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); ASSERT_OK_AND_ASSIGN(auto cel_expr, builder->CreateExpression(&parsed_expr.expr(), nullptr)); @@ -700,7 +737,8 @@ void BM_ProtoStructAccess(benchmark::State& state) { )cel")); InterpreterOptions options = GetOptions(arena); auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); ASSERT_OK_AND_ASSIGN(auto cel_expr, builder->CreateExpression(&parsed_expr.expr(), nullptr)); @@ -730,7 +768,8 @@ void BM_ProtoListAccess(benchmark::State& state) { )cel")); InterpreterOptions options = GetOptions(arena); auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); ASSERT_OK_AND_ASSIGN(auto cel_expr, builder->CreateExpression(&parsed_expr.expr(), nullptr)); @@ -867,7 +906,8 @@ void BM_NestedComprehension(benchmark::State& state) { InterpreterOptions options = GetOptions(arena); options.comprehension_max_iterations = 10000000; auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); ASSERT_OK_AND_ASSIGN(auto cel_expr, builder->CreateExpression(&expr, nullptr)); @@ -903,7 +943,8 @@ void BM_NestedComprehension_Trace(benchmark::State& state) { options.enable_recursive_tracing = true; auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); ASSERT_OK_AND_ASSIGN(auto cel_expr, builder->CreateExpression(&expr, nullptr)); @@ -936,7 +977,8 @@ void BM_ListComprehension(benchmark::State& state) { options.comprehension_max_iterations = 10000000; options.enable_comprehension_list_append = true; auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); ASSERT_OK_AND_ASSIGN( auto cel_expr, builder->CreateExpression(&(parsed_expr.expr()), nullptr)); @@ -971,7 +1013,8 @@ void BM_ListComprehension_Trace(benchmark::State& state) { options.enable_recursive_tracing = true; auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry(), options)); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry(), options), + IsOk()); ASSERT_OK_AND_ASSIGN( auto cel_expr, builder->CreateExpression(&(parsed_expr.expr()), nullptr)); @@ -1006,7 +1049,7 @@ void BM_ListComprehension_Opt(benchmark::State& state) { options.comprehension_max_iterations = 10000000; options.enable_comprehension_list_append = true; auto builder = CreateCelExpressionBuilder(options); - ASSERT_OK(RegisterBuiltinFunctions(builder->GetRegistry())); + ASSERT_THAT(RegisterBuiltinFunctions(builder->GetRegistry()), IsOk()); ASSERT_OK_AND_ASSIGN( auto cel_expr, builder->CreateExpression(&(parsed_expr.expr()), nullptr)); diff --git a/eval/tests/modern_benchmark_test.cc b/eval/tests/modern_benchmark_test.cc index 005f93aa5..28daed981 100644 --- a/eval/tests/modern_benchmark_test.cc +++ b/eval/tests/modern_benchmark_test.cc @@ -14,7 +14,6 @@ // // General benchmarks for CEL evaluator. -#include #include #include #include @@ -36,19 +35,24 @@ #include "absl/status/status_matchers.h" #include "absl/status/statusor.h" #include "absl/strings/match.h" +#include "absl/strings/string_view.h" +#include "checker/validation_result.h" #include "common/allocator.h" #include "common/casting.h" +#include "common/decl.h" #include "common/native_type.h" +#include "common/type.h" #include "common/value.h" +#include "compiler/compiler.h" +#include "compiler/compiler_factory.h" +#include "compiler/standard_library.h" #include "eval/tests/request_context.pb.h" #include "extensions/comprehensions_v2_functions.h" #include "extensions/comprehensions_v2_macros.h" #include "extensions/protobuf/runtime_adapter.h" -#include "extensions/protobuf/value.h" #include "internal/benchmark.h" #include "internal/testing.h" #include "internal/testing_descriptor_pool.h" -#include "internal/testing_message_factory.h" #include "parser/macro_registry.h" #include "parser/parser.h" #include "runtime/activation.h" @@ -62,6 +66,7 @@ #include "google/protobuf/text_format.h" ABSL_FLAG(bool, enable_recursive_planning, false, "enable recursive planning"); +ABSL_FLAG(bool, enable_typed_field_access, false, "enable typed field access"); namespace cel { @@ -85,16 +90,21 @@ RuntimeOptions GetOptions() { options.max_recursion_depth = -1; } + if (absl::GetFlag(FLAGS_enable_typed_field_access)) { + options.enable_typed_field_access = true; + } + return options; } enum class ConstFoldingEnabled { kNo, kYes }; std::unique_ptr StandardRuntimeOrDie( - const cel::RuntimeOptions& options, google::protobuf::Arena* arena = nullptr, + const cel::RuntimeOptions& options, + const google::protobuf::DescriptorPool* descriptor_pool, + google::protobuf::Arena* arena = nullptr, ConstFoldingEnabled const_folding = ConstFoldingEnabled::kNo) { - auto builder = CreateStandardRuntimeBuilder( - internal::GetTestingDescriptorPool(), options); + auto builder = CreateStandardRuntimeBuilder(descriptor_pool, options); ABSL_CHECK_OK(builder.status()); switch (const_folding) { @@ -111,13 +121,17 @@ std::unique_ptr StandardRuntimeOrDie( return std::move(runtime).value(); } +std::unique_ptr StandardRuntimeOrDie( + const cel::RuntimeOptions& options, google::protobuf::Arena* arena = nullptr, + ConstFoldingEnabled const_folding = ConstFoldingEnabled::kNo) { + return StandardRuntimeOrDie(options, internal::GetTestingDescriptorPool(), + arena, const_folding); +} + template Value WrapMessageOrDie(const T& message, google::protobuf::Arena* absl_nonnull arena) { - auto value = extensions::ProtoMessageToValue( - message, internal::GetTestingDescriptorPool(), - internal::GetTestingMessageFactory(), arena); - ABSL_CHECK_OK(value.status()); - return std::move(value).value(); + return Value::FromMessage(message, google::protobuf::DescriptorPool::generated_pool(), + google::protobuf::MessageFactory::generated_factory(), arena); } // Benchmark test @@ -335,7 +349,23 @@ BENCHMARK(BM_PolicyNative); void BM_PolicySymbolic(benchmark::State& state) { google::protobuf::Arena arena; - ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, Parse(R"cel( + ASSERT_OK_AND_ASSIGN( + auto builder, + NewCompilerBuilder(cel::internal::GetSharedTestingDescriptorPool())); + ASSERT_THAT(builder->AddLibrary(StandardCompilerLibrary()), IsOk()); + ASSERT_THAT(builder->GetCheckerBuilder().AddVariable( + MakeVariableDecl("ip", StringType())), + IsOk()); + ASSERT_THAT(builder->GetCheckerBuilder().AddVariable( + MakeVariableDecl("path", StringType())), + IsOk()); + ASSERT_THAT(builder->GetCheckerBuilder().AddVariable( + MakeVariableDecl("token", StringType())), + IsOk()); + ASSERT_OK_AND_ASSIGN(auto compiler, builder->Build()); + + ASSERT_OK_AND_ASSIGN(ValidationResult validation_result, + compiler->Compile(R"cel( !(ip in ["10.0.1.4", "10.0.1.5", "10.0.1.6"]) && ((path.startsWith("v1") && token in ["v1", "v2", "admin"]) || (path.startsWith("v2") && token in ["v2", "admin"]) || @@ -343,13 +373,14 @@ void BM_PolicySymbolic(benchmark::State& state) { "10.0.1.1", "10.0.1.2", "10.0.1.3" ]) ))cel")); + ASSERT_TRUE(validation_result.IsValid()); + ASSERT_OK_AND_ASSIGN(auto ast, validation_result.ReleaseAst()); RuntimeOptions options = GetOptions(); auto runtime = StandardRuntimeOrDie(options, &arena, ConstFoldingEnabled::kYes); - ASSERT_OK_AND_ASSIGN(auto cel_expr, ProtobufRuntimeAdapter::CreateProgram( - *runtime, parsed_expr)); + ASSERT_OK_AND_ASSIGN(auto cel_expr, runtime->CreateProgram(std::move(ast))); Activation activation; activation.InsertOrAssignValue("ip", StringValue(&arena, kIP)); @@ -437,21 +468,31 @@ class RequestMapImpl : public CustomMapValueInterface { // Uses a lazily constructed map container for "ip", "path", and "token". void BM_PolicySymbolicMap(benchmark::State& state) { google::protobuf::Arena arena; - ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, Parse(R"cel( + ASSERT_OK_AND_ASSIGN( + auto builder, + NewCompilerBuilder(cel::internal::GetSharedTestingDescriptorPool())); + ASSERT_THAT(builder->AddLibrary(StandardCompilerLibrary()), IsOk()); + ASSERT_THAT(builder->GetCheckerBuilder().AddVariable(MakeVariableDecl( + "request", + cel::MapType(&arena, cel::StringType(), cel::StringType()))), + IsOk()); + ASSERT_OK_AND_ASSIGN(auto compiler, builder->Build()); + + ASSERT_OK_AND_ASSIGN(ValidationResult validation_result, + compiler->Compile(R"cel( !(request.ip in ["10.0.1.4", "10.0.1.5", "10.0.1.6"]) && ((request.path.startsWith("v1") && request.token in ["v1", "v2", "admin"]) || (request.path.startsWith("v2") && request.token in ["v2", "admin"]) || (request.path.startsWith("/admin") && request.token == "admin" && request.ip in ["10.0.1.1", "10.0.1.2", "10.0.1.3"]) ))cel")); + ASSERT_TRUE(validation_result.IsValid()); + ASSERT_OK_AND_ASSIGN(auto ast, validation_result.ReleaseAst()); RuntimeOptions options = GetOptions(); - auto runtime = StandardRuntimeOrDie(options); - SourceInfo source_info; - ASSERT_OK_AND_ASSIGN(auto cel_expr, ProtobufRuntimeAdapter::CreateProgram( - *runtime, parsed_expr)); + ASSERT_OK_AND_ASSIGN(auto cel_expr, runtime->CreateProgram(std::move(ast))); Activation activation; CustomMapValue map_value(google::protobuf::Arena::Create(&arena), @@ -472,21 +513,31 @@ BENCHMARK(BM_PolicySymbolicMap); // Uses a protobuf container for "ip", "path", and "token". void BM_PolicySymbolicProto(benchmark::State& state) { google::protobuf::Arena arena; - ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, Parse(R"cel( + ASSERT_OK_AND_ASSIGN( + auto builder, + NewCompilerBuilder(google::protobuf::DescriptorPool::generated_pool())); + ASSERT_THAT(builder->AddLibrary(StandardCompilerLibrary()), IsOk()); + ASSERT_THAT(builder->GetCheckerBuilder().AddVariable(MakeVariableDecl( + "request", cel::MessageType(RequestContext::descriptor()))), + IsOk()); + ASSERT_OK_AND_ASSIGN(auto compiler, builder->Build()); + + ASSERT_OK_AND_ASSIGN(ValidationResult validation_result, + compiler->Compile(R"cel( !(request.ip in ["10.0.1.4", "10.0.1.5", "10.0.1.6"]) && ((request.path.startsWith("v1") && request.token in ["v1", "v2", "admin"]) || (request.path.startsWith("v2") && request.token in ["v2", "admin"]) || (request.path.startsWith("/admin") && request.token == "admin" && request.ip in ["10.0.1.1", "10.0.1.2", "10.0.1.3"]) ))cel")); + ASSERT_TRUE(validation_result.IsValid()); + ASSERT_OK_AND_ASSIGN(auto ast, validation_result.ReleaseAst()); RuntimeOptions options = GetOptions(); + auto runtime = + StandardRuntimeOrDie(options, google::protobuf::DescriptorPool::generated_pool()); - auto runtime = StandardRuntimeOrDie(options); - - SourceInfo source_info; - ASSERT_OK_AND_ASSIGN(auto cel_expr, ProtobufRuntimeAdapter::CreateProgram( - *runtime, parsed_expr)); + ASSERT_OK_AND_ASSIGN(auto cel_expr, runtime->CreateProgram(std::move(ast))); Activation activation; RequestContext request; @@ -497,8 +548,7 @@ void BM_PolicySymbolicProto(benchmark::State& state) { for (auto _ : state) { ASSERT_OK_AND_ASSIGN(cel::Value result, cel_expr->Evaluate(&arena, activation)); - ASSERT_TRUE(InstanceOf(result) && - Cast(result).NativeValue()); + ASSERT_TRUE(result.IsBool() && result.GetBool().NativeValue()); } } diff --git a/runtime/internal/BUILD b/runtime/internal/BUILD index f1141ef08..6c96de1cf 100644 --- a/runtime/internal/BUILD +++ b/runtime/internal/BUILD @@ -194,6 +194,7 @@ cc_library( srcs = ["runtime_type_provider.cc"], hdrs = ["runtime_type_provider.h"], deps = [ + "//common:descriptor_pool_type_introspector", "//common:type", "//common:value", "//internal:status_macros", diff --git a/runtime/internal/runtime_type_provider.cc b/runtime/internal/runtime_type_provider.cc index e6fd55d2c..5314918bf 100644 --- a/runtime/internal/runtime_type_provider.cc +++ b/runtime/internal/runtime_type_provider.cc @@ -14,6 +14,7 @@ #include "runtime/internal/runtime_type_provider.h" +#include #include #include "absl/base/nullability.h" @@ -28,7 +29,6 @@ #include "common/value.h" #include "common/values/value_builder.h" #include "google/protobuf/arena.h" -#include "google/protobuf/descriptor.h" #include "google/protobuf/message.h" namespace cel::runtime_internal { @@ -48,9 +48,14 @@ absl::StatusOr> RuntimeTypeProvider::FindTypeImpl( if (type.has_value()) { return type; } - const auto* desc = descriptor_pool_->FindMessageTypeByName(name); - if (desc != nullptr) { - return MessageType(desc); + + auto result = descriptor_pool_provider_.FindType(name); + if (!result.ok()) { + return result; + } + + if (result->has_value() && !result->value().IsEnum()) { + return result; } if (const auto it = types_.find(name); it != types_.end()) { @@ -66,24 +71,7 @@ RuntimeTypeProvider::FindEnumConstantImpl(absl::string_view type, if (enum_constant.has_value()) { return enum_constant; } - const google::protobuf::EnumDescriptor* enum_desc = - descriptor_pool_->FindEnumTypeByName(type); - if (enum_desc == nullptr) { - return std::nullopt; - } - - // Note: we don't support strong enum typing at this time so only the fully - // qualified enum values are meaningful, so we don't provide any signal if the - // enum type is found but can't match the value name. - const google::protobuf::EnumValueDescriptor* value_desc = - enum_desc->FindValueByName(value); - if (value_desc == nullptr) { - return std::nullopt; - } - - return TypeIntrospector::EnumConstant{ - EnumType(enum_desc), enum_desc->full_name(), value_desc->name(), - value_desc->number()}; + return descriptor_pool_provider_.FindEnumConstant(type, value); } absl::StatusOr> @@ -93,18 +81,7 @@ RuntimeTypeProvider::FindStructTypeFieldByNameImpl( if (field.has_value()) { return field; } - const auto* desc = descriptor_pool_->FindMessageTypeByName(type); - if (desc == nullptr) { - return std::nullopt; - } - const auto* field_desc = desc->FindFieldByName(name); - if (field_desc == nullptr) { - field_desc = descriptor_pool_->FindExtensionByPrintableName(desc, name); - if (field_desc == nullptr) { - return std::nullopt; - } - } - return MessageTypeField(field_desc); + return descriptor_pool_provider_.FindStructTypeFieldByName(type, name); } absl::StatusOr diff --git a/runtime/internal/runtime_type_provider.h b/runtime/internal/runtime_type_provider.h index 3f418af4d..fc7163f6c 100644 --- a/runtime/internal/runtime_type_provider.h +++ b/runtime/internal/runtime_type_provider.h @@ -21,6 +21,7 @@ #include "absl/status/statusor.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" +#include "common/descriptor_pool_type_introspector.h" #include "common/type.h" #include "common/type_reflector.h" #include "common/value.h" @@ -34,7 +35,8 @@ class RuntimeTypeProvider final : public TypeReflector { public: explicit RuntimeTypeProvider( const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool) - : descriptor_pool_(descriptor_pool) {} + : descriptor_pool_(descriptor_pool), + descriptor_pool_provider_(descriptor_pool) {} absl::Status RegisterType(const OpaqueType& type); @@ -55,6 +57,7 @@ class RuntimeTypeProvider final : public TypeReflector { private: const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool_; + DescriptorPoolTypeIntrospector descriptor_pool_provider_; absl::flat_hash_map types_; }; diff --git a/runtime/runtime_options.h b/runtime/runtime_options.h index 7a61208a0..072daf26c 100644 --- a/runtime/runtime_options.h +++ b/runtime/runtime_options.h @@ -188,6 +188,19 @@ struct RuntimeOptions { // // If disabled, will use the legacy behavior of rounding to 6 decimal places. bool enable_precision_preserving_double_format = true; + + // When enabled, the planner will attempt to use a more performant execution + // path for field access when the type is known at plan time, instead of using + // the generic field access implementation. + // + // The runtime will try to verify that the field access is compatible with the + // actual type at evaluation time, and will fall back to the generic + // implementation if the value is not what was expected. + // + // This is not recommended if the values bound to the activation are typically + // not what the planner expected (e.g. a map that was declared as a proto or + // a different message with matching field names). + bool enable_typed_field_access = false; }; // LINT.ThenChange(//depot/google3/eval/public/cel_options.h)