From af0c011ba5ee02e04b8f33242ba0bcd1ef6fcf33 Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Mon, 12 Oct 2026 13:33:06 -0700 Subject: [PATCH] Map reused identifiers to slots and memoize. Adjust iter stack count estimate. PiperOrigin-RevId: 998122413 --- eval/compiler/BUILD | 3 + eval/compiler/constant_folding.cc | 5 +- eval/compiler/flat_expr_builder.cc | 113 +++++++++++++++++- eval/compiler/flat_expr_builder_test.cc | 149 ++++++++++++++++++++++++ eval/eval/BUILD | 2 + eval/eval/comprehension_slots.h | 2 +- eval/eval/comprehension_step_test.cc | 9 +- eval/eval/container_access_step_test.cc | 15 +-- eval/eval/create_list_step_test.cc | 15 ++- eval/eval/create_map_step_test.cc | 19 +-- eval/eval/create_struct_step_test.cc | 11 +- eval/eval/equality_steps_test.cc | 8 +- eval/eval/evaluator_core.cc | 10 +- eval/eval/evaluator_core.h | 76 +++++++----- eval/eval/evaluator_core_test.cc | 11 +- eval/eval/function_step_test.cc | 62 +++++----- eval/eval/ident_step.cc | 28 ++++- eval/eval/ident_step.h | 17 ++- eval/eval/ident_step_test.cc | 100 ++++++++++++---- eval/eval/iterator_stack.h | 3 +- eval/eval/lazy_init_step_test.cc | 3 +- eval/eval/logic_step_test.cc | 11 +- eval/eval/select_step_test.cc | 46 +++++--- eval/eval/shadowable_value_step_test.cc | 2 +- eval/eval/ternary_step_test.cc | 11 +- 25 files changed, 558 insertions(+), 173 deletions(-) diff --git a/eval/compiler/BUILD b/eval/compiler/BUILD index d75e6e50d..e35e74918 100644 --- a/eval/compiler/BUILD +++ b/eval/compiler/BUILD @@ -150,7 +150,10 @@ cc_test( "//common:function_descriptor", "//common:kind", "//common:value", + "//eval/eval:cel_expression_flat_impl", + "//eval/eval:evaluator_core", "//eval/public:activation", + "//eval/public:base_activation", "//eval/public:builtin_func_registrar", "//eval/public:cel_attribute", "//eval/public:cel_expr_builder_factory", diff --git a/eval/compiler/constant_folding.cc b/eval/compiler/constant_folding.cc index fb58d7d48..4a8d55c31 100644 --- a/eval/compiler/constant_folding.cc +++ b/eval/compiler/constant_folding.cc @@ -75,8 +75,9 @@ class ConstantFoldingExtension : public ProgramOptimizer { const TypeProvider& type_provider) : shared_arena_(std::move(shared_arena)), shared_message_factory_(std::move(shared_message_factory)), - state_(kDefaultStackLimit, kComprehensionSlotCount, type_provider, - descriptor_pool, message_factory, arena) {} + state_(kDefaultStackLimit, kComprehensionSlotCount, + /*iterator_stack_size=*/0, type_provider, descriptor_pool, + message_factory, arena) {} absl::Status OnPreVisit(google::api::expr::runtime::PlannerContext& context, const Expr& node) override; diff --git a/eval/compiler/flat_expr_builder.cc b/eval/compiler/flat_expr_builder.cc index a305dec17..a35554638 100644 --- a/eval/compiler/flat_expr_builder.cc +++ b/eval/compiler/flat_expr_builder.cc @@ -70,6 +70,7 @@ #include "eval/eval/evaluator_core.h" #include "eval/eval/expression_step_logic.h" #include "eval/eval/function_step.h" +#include "eval/eval/ident_step.h" #include "eval/eval/optional_or_step.h" #include "eval/eval/select_step.h" #include "eval/eval/shadowable_value_step.h" @@ -105,7 +106,12 @@ class FlatExprVisitor; // Helper for bookkeeping variables mapped to indexes. class IndexManager { public: - IndexManager() : next_free_slot_(0), max_slot_count_(0) {} + IndexManager() + : next_free_slot_(0), + max_slot_count_(0), + iter_stack_count_(0), + max_iter_stack_count_(0), + lazy_iter_stack_count_(0) {} size_t ReserveSlots(size_t n) { size_t result = next_free_slot_; @@ -121,11 +127,30 @@ class IndexManager { return next_free_slot_; } + void ReserveIterStack() { + ++iter_stack_count_; + max_iter_stack_count_ = std::max(max_iter_stack_count_, iter_stack_count_); + } + void ReleaseIterStack() { --iter_stack_count_; } + void AddLazyIterStack() { ++lazy_iter_stack_count_; } + size_t max_slot_count() const { return max_slot_count_; } + // Returns the maximum number of iterator stack slots required to evaluate the + // expression by tracking the maximum comprehension nesting in the main + // expression and dedicating a slot for any comprehensions evaluated lazily. + // This is not as tight as possible, but is a correct upper bound estimate and + // is tight for small expressions. + size_t IterStackRequirement() const { + return max_iter_stack_count_ + lazy_iter_stack_count_; + } + private: size_t next_free_slot_; size_t max_slot_count_; + size_t iter_stack_count_; + size_t max_iter_stack_count_; + size_t lazy_iter_stack_count_; }; // Helper for computing jump offsets. @@ -844,8 +869,12 @@ class FlatExprVisitor : public cel::AstVisitor { return; } + if (PlanningSuppressed()) { + return; + } absl::string_view ident_name = absl::StripPrefix(ident_expr.name(), "."); - AddStep(ExpressionStep::MakeIdentifierStep(ident_name, expr.id())); + size_t ident_index = AddOrUpdateIdentifier(ident_name); + AddStep(ExpressionStep::MakeIdentifierStep(ident_index, expr.id())); } void PreVisitSelect(const cel::Expr& expr, @@ -1121,21 +1150,34 @@ class FlatExprVisitor : public cel::AstVisitor { slot_count = 0; } } + // If this is in the scope of an optimized bind accu-init, account the slots // to the outermost bind-init scope. // // The init expression is effectively inlined at the first usage in the // critical path (which is unknown at plan time), so the used slots need to // be dedicated for the entire scope of that bind. + bool in_bind_init = false; for (ComprehensionStackRecord& record : comprehension_stack_) { if (record.in_accu_init && record.is_optimizable_bind) { record.slot_count += slot_count; slot_count = 0; + in_bind_init = true; break; } // If no bind init subexpression, account normally. } + const bool is_stack_releasable = + !in_bind_init && (!block_.has_value() || !block_->in); + if (!is_bind) { + if (is_stack_releasable) { + index_manager_.AddLazyIterStack(); + } else { + index_manager_.ReserveIterStack(); + } + } + comprehension_stack_.push_back( {&expr, &comprehension, iter_slot, iter2_slot, accu_slot, slot_count, /*subexpression=*/-1, @@ -1146,6 +1188,7 @@ class FlatExprVisitor : public cel::AstVisitor { IsOptimizableMapInsert(&comprehension, options_.enable_comprehension_mutable_map), /*.is_optimizable_bind=*/is_bind, + /*.is_stack_releasable=*/is_stack_releasable, /*.iter_var_in_scope=*/false, /*.iter_var2_in_scope=*/false, /*.accu_var_in_scope=*/false, @@ -1172,6 +1215,9 @@ class FlatExprVisitor : public cel::AstVisitor { record.visitor->PostVisit(&expr); index_manager_.ReleaseSlots(record.slot_count); + if (record.is_stack_releasable) { + index_manager_.ReleaseIterStack(); + } comprehension_stack_.pop_back(); } @@ -1463,6 +1509,23 @@ class FlatExprVisitor : public cel::AstVisitor { size_t slot_count() const { return index_manager_.max_slot_count(); } + size_t iter_stack_count() const { + return index_manager_.IterStackRequirement(); + } + + IdentifierTable BuildIdentifierTable(size_t& total_slot_count) && { + IdentifierTable table; + table.reserve(identifiers_.size()); + for (IdentifierRecord& record : identifiers_) { + size_t slot_index = Identifier::kNoSlot; + if (record.in_comprehension || record.count > 1) { + slot_index = total_slot_count++; + } + table.push_back(Identifier{std::move(record.name), slot_index}); + } + return table; + } + void AddOptimizer(std::unique_ptr optimizer) { program_optimizers_.push_back(std::move(optimizer)); } @@ -1482,6 +1545,13 @@ class FlatExprVisitor : public cel::AstVisitor { } private: + struct IdentifierRecord { + std::string name; + size_t ident_index; + size_t count = 0; + bool in_comprehension = false; + }; + struct ComprehensionStackRecord { const cel::Expr* expr; const cel::ComprehensionExpr* comprehension; @@ -1494,6 +1564,7 @@ class FlatExprVisitor : public cel::AstVisitor { bool is_optimizable_list_append; bool is_optimizable_map_insert; bool is_optimizable_bind; + bool is_stack_releasable; bool iter_var_in_scope; bool iter_var2_in_scope; bool accu_var_in_scope; @@ -1613,6 +1684,8 @@ class FlatExprVisitor : public cel::AstVisitor { void MaybeResolveType(const cel::Expr& expr); + size_t AddOrUpdateIdentifier(absl::string_view name); + const Resolver& resolver_; const cel::TypeProvider& type_provider_; absl::Status progress_status_; @@ -1641,6 +1714,8 @@ class FlatExprVisitor : public cel::AstVisitor { ProgramBuilder& program_builder_; PlannerContext& extension_context_; IndexManager index_manager_; + std::vector identifiers_; + absl::flat_hash_map identifier_map_; bool enable_optional_types_; std::optional block_; @@ -1787,6 +1862,28 @@ void FlatExprVisitor::MaybeResolveType(const cel::Expr& expr) { SetResolvedType(expr, *type); } +size_t FlatExprVisitor::AddOrUpdateIdentifier(absl::string_view name) { + auto it = identifier_map_.find(name); + if (it == identifier_map_.end()) { + size_t index = identifiers_.size(); + it = identifier_map_.emplace(name, index).first; + identifiers_.push_back( + IdentifierRecord{std::string(name), index, 0, false}); + } + IdentifierRecord& record = identifiers_[it->second]; + record.count++; + if (record.in_comprehension) { + return record.ident_index; + } + for (const ComprehensionStackRecord& comp : comprehension_stack_) { + if (!comp.is_optimizable_bind) { + record.in_comprehension = true; + break; + } + } + return record.ident_index; +} + void LogicalCondVisitor::PreVisit(const cel::Expr* expr) { visitor_->ValidateOrError( !expr->call_expr().has_target() && expr->call_expr().args().size() >= 2, @@ -2244,7 +2341,12 @@ absl::StatusOr FlatExprBuilder::CreateExpressionImpl( return visitor.progress_status(); } - if (visitor.slot_count() > std::numeric_limits::max() || + size_t total_slot_count = visitor.slot_count(); + size_t iterator_stack_size = visitor.iter_stack_count(); + IdentifierTable identifiers = + std::move(visitor).BuildIdentifierTable(total_slot_count); + + if (total_slot_count > std::numeric_limits::max() || program_builder.ExtractedSubexpressionCount() > std::numeric_limits::max()) { // Impractical to trigger (we'd run out of memory first), but assuming this @@ -2262,8 +2364,9 @@ absl::StatusOr FlatExprBuilder::CreateExpressionImpl( FlattenExpressionTable(program_builder, execution_path); return FlatExpression(std::move(execution_path), std::move(subexpressions), - visitor.slot_count(), GetTypeProvider(), options_, - std::move(arena)); + total_slot_count, iterator_stack_size, + GetTypeProvider(), options_, std::move(arena), + std::move(identifiers)); } const cel::TypeProvider& FlatExprBuilder::GetTypeProvider() const { return use_legacy_type_provider_ diff --git a/eval/compiler/flat_expr_builder_test.cc b/eval/compiler/flat_expr_builder_test.cc index 8611de388..d6bda6437 100644 --- a/eval/compiler/flat_expr_builder_test.cc +++ b/eval/compiler/flat_expr_builder_test.cc @@ -41,7 +41,11 @@ #include "eval/compiler/cel_expression_builder_flat_impl.h" #include "eval/compiler/constant_folding.h" #include "eval/compiler/qualified_reference_resolver.h" +#include "eval/eval/cel_expression_flat_impl.h" +#include "eval/eval/evaluator_core.h" +#include "eval/eval/ident_step.h" #include "eval/public/activation.h" +#include "eval/public/base_activation.h" #include "eval/public/builtin_func_registrar.h" #include "eval/public/cel_attribute.h" #include "eval/public/cel_expr_builder_factory.h" @@ -3016,6 +3020,151 @@ INSTANTIATE_TEST_SUITE_P( VariadicLogicalEvalTestCase{"All_Unknown", "[a, b, c].all(x, x)", "true", "unknown1", "true", "unknown"})); +class CountingActivation : public BaseActivation { + public: + void InsertInt(absl::string_view name, int64_t val) { + values_[name] = CelValue::CreateInt64(val); + } + + void InsertBool(absl::string_view name, bool val) { + values_[name] = CelValue::CreateBool(val); + } + + std::vector FindFunctionOverloads( + absl::string_view) const override { + return {}; + } + + absl::optional FindValue(absl::string_view name, + google::protobuf::Arena*) const override { + lookup_counts_[name]++; + auto it = values_.find(name); + if (it != values_.end()) { + return it->second; + } + return absl::nullopt; + } + + int LookupCount(absl::string_view name) const { + auto it = lookup_counts_.find(name); + return it != lookup_counts_.end() ? it->second : 0; + } + + private: + absl::flat_hash_map values_; + mutable absl::flat_hash_map lookup_counts_; +}; + +TEST(FlatExprBuilderTest, IdentifierTableRepeatedRootIdentifierReceivesSlot) { + ASSERT_OK_AND_ASSIGN(ParsedExpr parsed_expr, parser::Parse("x + y + x")); + CelExpressionBuilderFlatImpl builder(NewTestingRuntimeEnv()); + ASSERT_THAT(RegisterBuiltinFunctions(builder.GetRegistry()), IsOk()); + ASSERT_OK_AND_ASSIGN(std::unique_ptr cel_expr, + builder.CreateExpression(&parsed_expr.expr(), + &parsed_expr.source_info())); + + const FlatExpression& flat_expr = + static_cast(cel_expr.get()) + ->flat_expression(); + ASSERT_THAT(flat_expr.identifiers(), SizeIs(2)); + EXPECT_EQ(flat_expr.identifiers()[0].name, "x"); + EXPECT_EQ(flat_expr.identifiers()[0].slot_index, 0); + EXPECT_EQ(flat_expr.identifiers()[1].name, "y"); + EXPECT_EQ(flat_expr.identifiers()[1].slot_index, Identifier::kNoSlot); + EXPECT_EQ(flat_expr.comprehension_slots_size(), 1); + EXPECT_EQ(flat_expr.iterator_stack_size(), 0); + + CountingActivation activation; + activation.InsertInt("x", 10); + activation.InsertInt("y", 20); + google::protobuf::Arena arena; + ASSERT_OK_AND_ASSIGN(CelValue result, cel_expr->Evaluate(activation, &arena)); + EXPECT_THAT(result, test::IsCelInt64(40)); + EXPECT_EQ(activation.LookupCount("x"), 1); + EXPECT_EQ(activation.LookupCount("y"), 1); +} + +TEST(FlatExprBuilderTest, + IdentifierTableInComprehensionReceivesSlotAboveComprehensionSlots) { + ASSERT_OK_AND_ASSIGN( + ParsedExpr parsed_expr, + parser::Parse("[1, 2, 3].all(i, i < limit) && single_var")); + CelExpressionBuilderFlatImpl builder(NewTestingRuntimeEnv()); + ASSERT_THAT(RegisterBuiltinFunctions(builder.GetRegistry()), IsOk()); + ASSERT_OK_AND_ASSIGN(std::unique_ptr cel_expr, + builder.CreateExpression(&parsed_expr.expr(), + &parsed_expr.source_info())); + + const FlatExpression& flat_expr = + static_cast(cel_expr.get()) + ->flat_expression(); + // Local comprehension variables (i, @result) are not in the root identifier + // table; only "limit" and "single_var" are present. + ASSERT_THAT(flat_expr.identifiers(), SizeIs(2)); + EXPECT_EQ(flat_expr.identifiers()[0].name, "limit"); + // Comprehension uses 2 slots (accu_var at 0, iter_var at 1), so "limit" gets + // slot 2. + EXPECT_EQ(flat_expr.identifiers()[0].slot_index, 2); + EXPECT_EQ(flat_expr.identifiers()[1].name, "single_var"); + EXPECT_EQ(flat_expr.identifiers()[1].slot_index, Identifier::kNoSlot); + EXPECT_EQ(flat_expr.comprehension_slots_size(), 3); + EXPECT_EQ(flat_expr.iterator_stack_size(), 1); + + CountingActivation activation; + activation.InsertInt("limit", 5); + activation.InsertBool("single_var", true); + google::protobuf::Arena arena; + ASSERT_OK_AND_ASSIGN(CelValue result, cel_expr->Evaluate(activation, &arena)); + EXPECT_THAT(result, test::IsCelBool(true)); + EXPECT_EQ(activation.LookupCount("limit"), 1); + EXPECT_EQ(activation.LookupCount("single_var"), 1); + EXPECT_EQ(activation.LookupCount("i"), 0); +} + +TEST(FlatExprBuilderTest, IdentifierTableInBindComprehensionNotSlotted) { + ParsedExpr parsed_expr; + ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString( + R"pb( + expr: { + comprehension_expr: { + iter_var: "#unused" + iter_range: { list_expr: {} } + accu_var: "bound_var" + accu_init: { ident_expr: { name: "x" } } + loop_condition: { const_expr: { bool_value: false } } + loop_step: { ident_expr: { name: "bound_var" } } + result: { + call_expr: { + function: "_+_" + args: { ident_expr: { name: "bound_var" } } + args: { ident_expr: { name: "y" } } + } + } + } + } + )pb", + &parsed_expr)); + + CelExpressionBuilderFlatImpl builder(NewTestingRuntimeEnv()); + ASSERT_THAT(RegisterBuiltinFunctions(builder.GetRegistry()), IsOk()); + ASSERT_OK_AND_ASSIGN(std::unique_ptr cel_expr, + builder.CreateExpression(&parsed_expr.expr(), + &parsed_expr.source_info())); + + const FlatExpression& flat_expr = + static_cast(cel_expr.get()) + ->flat_expression(); + ASSERT_THAT(flat_expr.identifiers(), SizeIs(2)); + EXPECT_EQ(flat_expr.identifiers()[0].name, "x"); + EXPECT_EQ(flat_expr.identifiers()[0].slot_index, Identifier::kNoSlot); + EXPECT_EQ(flat_expr.identifiers()[1].name, "y"); + EXPECT_EQ(flat_expr.identifiers()[1].slot_index, Identifier::kNoSlot); + // Only the 1 bind slot is allocated; neither single-use root identifier gets + // a slot. + EXPECT_EQ(flat_expr.comprehension_slots_size(), 1); + EXPECT_EQ(flat_expr.iterator_stack_size(), 0); +} + } // namespace } // namespace google::api::expr::runtime diff --git a/eval/eval/BUILD b/eval/eval/BUILD index 13efe4c0e..46c1a7900 100644 --- a/eval/eval/BUILD +++ b/eval/eval/BUILD @@ -82,6 +82,7 @@ cc_library( "//runtime/standard:equality_functions", "@com_google_absl//absl/base", "@com_google_absl//absl/base:core_headers", + "@com_google_absl//absl/base:no_destructor", "@com_google_absl//absl/base:nullability", "@com_google_absl//absl/log:absl_check", "@com_google_absl//absl/log:absl_log", @@ -948,6 +949,7 @@ cc_library( deps = [ "//common:value", "@com_google_absl//absl/base:nullability", + "@com_google_absl//absl/container:inlined_vector", "@com_google_absl//absl/log:absl_check", ], ) diff --git a/eval/eval/comprehension_slots.h b/eval/eval/comprehension_slots.h index 5a7ce21cc..fe0fbd615 100644 --- a/eval/eval/comprehension_slots.h +++ b/eval/eval/comprehension_slots.h @@ -150,7 +150,7 @@ class ComprehensionSlots final { size_t size() const { return slots_.size(); } private: - absl::FixedArray slots_; + absl::FixedArray slots_; }; } // namespace google::api::expr::runtime diff --git a/eval/eval/comprehension_step_test.cc b/eval/eval/comprehension_step_test.cc index 5964421ed..120143914 100644 --- a/eval/eval/comprehension_step_test.cc +++ b/eval/eval/comprehension_step_test.cc @@ -51,7 +51,8 @@ class ListKeysStepTest : public testing::Test { return std::make_unique( env, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, IdentifierTable{{"var"}})); } private: @@ -75,7 +76,7 @@ MATCHER_P(CelStringValue, val, "") { TEST_F(ListKeysStepTest, MapPartiallyUnknown) { ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("var")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); auto init_step = std::make_unique(/*iter_slot=*/0, /*accu_slot=*/0); init_step->set_error_jump_offset(1); @@ -115,7 +116,7 @@ TEST_F(ListKeysStepTest, MapPartiallyUnknown) { TEST_F(ListKeysStepTest, ErrorPassedThrough) { ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("var")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); auto init_step = std::make_unique(/*iter_slot=*/0, /*accu_slot=*/0); init_step->set_error_jump_offset(1); @@ -140,7 +141,7 @@ TEST_F(ListKeysStepTest, ErrorPassedThrough) { TEST_F(ListKeysStepTest, UnknownSetPassedThrough) { ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("var")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); auto init_step = std::make_unique(/*iter_slot=*/0, /*accu_slot=*/0); init_step->set_error_jump_offset(1); diff --git a/eval/eval/container_access_step_test.cc b/eval/eval/container_access_step_test.cc index a298c28e3..d99ac0c17 100644 --- a/eval/eval/container_access_step_test.cc +++ b/eval/eval/container_access_step_test.cc @@ -75,18 +75,19 @@ CelValue EvaluateAttributeHelper( container_expr.mutable_ident_expr().set_name("container"); key_expr.mutable_ident_expr().set_name("key"); - path.push_back(ExpressionStep::MakeIdentifierStep("container", 1)); - path.push_back(ExpressionStep::MakeIdentifierStep("key", 2)); - path.push_back(ExpressionStep::MakeGenericStep( - std::move(CreateContainerAccessStep(call).value()), 3)); + path.push_back(ExpressionStep::MakeIdentifierStep(0, 1)); + path.push_back(ExpressionStep::MakeIdentifierStep(1, 2)); + path.push_back(ExpressionStep::MakeGenericStep( + std::move(CreateContainerAccessStep(call).value()), 3)); cel::RuntimeOptions options; options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; options.enable_heterogeneous_equality = false; CelExpressionFlatImpl cel_expr( - env, - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, + IdentifierTable{{"container"}, {"key"}})); Activation activation; activation.InsertValue("container", container); diff --git a/eval/eval/create_list_step_test.cc b/eval/eval/create_list_step_test.cc index a8f02b5d8..7eb63f35e 100644 --- a/eval/eval/create_list_step_test.cc +++ b/eval/eval/create_list_step_test.cc @@ -74,7 +74,8 @@ absl::StatusOr RunExpression( FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, {})); Activation activation; return cel_expr.Evaluate(activation, arena); @@ -89,6 +90,7 @@ absl::StatusOr RunExpressionWithCelValues( Expr dummy_expr; Activation activation; + IdentifierTable identifiers; auto& create_list = dummy_expr.mutable_list_expr(); int ind = 0; for (auto value : values) { @@ -97,7 +99,8 @@ absl::StatusOr RunExpressionWithCelValues( expr0.set_id(ind); expr0.mutable_ident_expr().set_name(var_name); - path.push_back(ExpressionStep::MakeIdentifierStep(var_name)); + path.push_back(ExpressionStep::MakeIdentifierStep(identifiers.size())); + identifiers.push_back(Identifier{var_name}); activation.InsertValue(var_name, value); } @@ -111,9 +114,9 @@ absl::StatusOr RunExpressionWithCelValues( } CelExpressionFlatImpl cel_expr( - env, - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, std::move(identifiers))); return cel_expr.Evaluate(activation, arena); } @@ -145,7 +148,7 @@ TEST(CreateListStepTest, TestCreateListStackUnderflow) { CelExpressionFlatImpl cel_expr( env, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, env->type_registry.GetComposedTypeProvider(), - cel::RuntimeOptions{})); + cel::RuntimeOptions{}, /*arena=*/nullptr, {})); Activation activation; google::protobuf::Arena arena; diff --git a/eval/eval/create_map_step_test.cc b/eval/eval/create_map_step_test.cc index f9f11516c..21537ed3b 100644 --- a/eval/eval/create_map_step_test.cc +++ b/eval/eval/create_map_step_test.cc @@ -30,6 +30,7 @@ #include "common/expr.h" #include "eval/eval/cel_expression_flat_impl.h" #include "eval/eval/evaluator_core.h" +#include "eval/eval/ident_step.h" #include "eval/public/activation.h" #include "eval/public/cel_value.h" #include "eval/public/unknown_set.h" @@ -54,7 +55,7 @@ using ::google::protobuf::Arena; absl::StatusOr CreateStackMachineProgram( const std::vector>& values, - Activation& activation) { + Activation& activation, IdentifierTable& identifiers) { ExecutionPath path; Expr expr1; @@ -69,8 +70,10 @@ absl::StatusOr CreateStackMachineProgram( std::string key_name = absl::StrCat("key", index); std::string value_name = absl::StrCat("value", index); - path.push_back(ExpressionStep::MakeIdentifierStep(key_name)); - path.push_back(ExpressionStep::MakeIdentifierStep(value_name)); + path.push_back(ExpressionStep::MakeIdentifierStep(identifiers.size())); + identifiers.push_back(Identifier{key_name}); + path.push_back(ExpressionStep::MakeIdentifierStep(identifiers.size())); + identifiers.push_back(Identifier{value_name}); activation.InsertValue(key_name, item.first); activation.InsertValue(value_name, item.second); @@ -93,18 +96,20 @@ absl::StatusOr RunCreateMapExpression( const std::vector>& values, google::protobuf::Arena* arena, bool enable_unknowns, bool enable_recursive_program) { Activation activation; + IdentifierTable identifiers; ExecutionPath path; - CEL_ASSIGN_OR_RETURN(path, CreateStackMachineProgram(values, activation)); + CEL_ASSIGN_OR_RETURN( + path, CreateStackMachineProgram(values, activation, identifiers)); cel::RuntimeOptions options; if (enable_unknowns) { options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; } CelExpressionFlatImpl cel_expr( - env, - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, std::move(identifiers))); return cel_expr.Evaluate(activation, arena); } diff --git a/eval/eval/create_struct_step_test.cc b/eval/eval/create_struct_step_test.cc index c6fa6f036..7541b31f7 100644 --- a/eval/eval/create_struct_step_test.cc +++ b/eval/eval/create_struct_step_test.cc @@ -73,7 +73,7 @@ absl::StatusOr MakeStackMachinePath(absl::string_view field) { {std::string(field)}, /*optional_indices=*/{}); - path.push_back(ExpressionStep::MakeIdentifierStep("message")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); path.push_back(ExpressionStep::MakeGenericStep(std::move(step1))); return path; @@ -103,9 +103,9 @@ absl::StatusOr RunExpression( CEL_ASSIGN_OR_RETURN(path, MakeStackMachinePath(field)); CelExpressionFlatImpl cel_expr( - env, - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, IdentifierTable{{"message"}})); Activation activation; activation.InsertValue("message", value); @@ -189,7 +189,8 @@ TEST_P(CreateCreateStructStepTest, TestEmptyMessageCreation) { CelExpressionFlatImpl cel_expr( env_, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env_->type_registry.GetComposedTypeProvider(), options)); + env_->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, {})); Activation activation; ASSERT_OK_AND_ASSIGN(CelValue result, cel_expr.Evaluate(activation, &arena_)); diff --git a/eval/eval/equality_steps_test.cc b/eval/eval/equality_steps_test.cc index 4ebfe10ef..50970ca44 100644 --- a/eval/eval/equality_steps_test.cc +++ b/eval/eval/equality_steps_test.cc @@ -81,7 +81,7 @@ TEST(IterativeTest, PartialAttrUnknown) { FlatExpressionEvaluatorState state( /*value_stack_size=*/5, - /*comprehension_slot_count=*/0, type_provider, + /*comprehension_slot_count=*/0, /*iterator_stack_size=*/0, type_provider, cel::internal::GetTestingDescriptorPool(), cel::internal::GetTestingMessageFactory(), &arena); @@ -113,7 +113,7 @@ TEST(IterativeTest, PartialAttrUnknownDisabled) { FlatExpressionEvaluatorState state( /*value_stack_size=*/5, - /*comprehension_slot_count=*/0, type_provider, + /*comprehension_slot_count=*/0, /*iterator_stack_size=*/0, type_provider, cel::internal::GetTestingDescriptorPool(), cel::internal::GetTestingMessageFactory(), &arena); @@ -183,7 +183,7 @@ TEST_P(EqualsTest, Iterative) { FlatExpressionEvaluatorState state( /*value_stack_size=*/5, - /*comprehension_slot_count=*/0, type_provider, + /*comprehension_slot_count=*/0, /*iterator_stack_size=*/0, type_provider, cel::internal::GetTestingDescriptorPool(), cel::internal::GetTestingMessageFactory(), &arena); @@ -328,7 +328,7 @@ TEST_P(InTest, Iterative) { FlatExpressionEvaluatorState state( /*value_stack_size=*/5, - /*comprehension_slot_count=*/0, type_provider, + /*comprehension_slot_count=*/0, /*iterator_stack_size=*/0, type_provider, cel::internal::GetTestingDescriptorPool(), cel::internal::GetTestingMessageFactory(), &arena); diff --git a/eval/eval/evaluator_core.cc b/eval/eval/evaluator_core.cc index 4314f4e38..5855eda8e 100644 --- a/eval/eval/evaluator_core.cc +++ b/eval/eval/evaluator_core.cc @@ -222,16 +222,16 @@ FlatExpressionEvaluatorState FlatExpression::MakeEvaluatorState( google::protobuf::MessageFactory* absl_nonnull message_factory, google::protobuf::Arena* absl_nonnull arena) const { return FlatExpressionEvaluatorState(path_.size(), comprehension_slots_size_, - type_provider_, descriptor_pool, - message_factory, arena); + iterator_stack_size_, type_provider_, + descriptor_pool, message_factory, arena); } FlatExpressionEvaluatorState FlatExpression::MakeEvaluatorState( const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, google::protobuf::MessageFactory* absl_nullable message_factory) const { return FlatExpressionEvaluatorState(path_.size(), comprehension_slots_size_, - type_provider_, descriptor_pool, - message_factory); + iterator_stack_size_, type_provider_, + descriptor_pool, message_factory); } absl::StatusOr FlatExpression::EvaluateWithCallback( @@ -241,7 +241,7 @@ absl::StatusOr FlatExpression::EvaluateWithCallback( state.Reset(); ExecutionFrame frame(subexpressions_, activation, options_, state, - std::move(listener), embedder_context); + std::move(listener), embedder_context, identifiers_); return frame.Evaluate(frame.callback()); } diff --git a/eval/eval/evaluator_core.h b/eval/eval/evaluator_core.h index d23dfd045..f22e68be7 100644 --- a/eval/eval/evaluator_core.h +++ b/eval/eval/evaluator_core.h @@ -19,11 +19,11 @@ #include #include #include -#include #include #include #include "absl/base/attributes.h" +#include "absl/base/no_destructor.h" #include "absl/base/nullability.h" #include "absl/base/optimization.h" #include "absl/log/absl_check.h" @@ -32,6 +32,7 @@ #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "absl/types/span.h" +#include "base/attribute.h" #include "base/type_provider.h" #include "common/value.h" #include "common/value_kind.h" @@ -290,16 +291,15 @@ class ExpressionStep { return step; } - static ExpressionStep MakeIdentifierStep(absl::string_view identifier, + static ExpressionStep MakeIdentifierStep(size_t ident_index, int64_t id = -1) { ExpressionStep step(ExpressionStepKind::kIdentifier, id); - step.u_.identifier = new std::string(identifier); + step.u_.ident_index = ident_index; return step; } - static ExpressionStep MakeIdentStep(absl::string_view identifier, - int64_t id = -1) { - return MakeIdentifierStep(identifier, id); + static ExpressionStep MakeIdentStep(size_t ident_index, int64_t id = -1) { + return MakeIdentifierStep(ident_index, id); } static ExpressionStep MakeFastInStep(int64_t id = -1) { @@ -405,7 +405,7 @@ class ExpressionStep { FixedJumpStepInfo fixed_jump_step; EagerFunctionStep* eager_function_step; LazyFunctionStep* lazy_function_step; - std::string* identifier; + size_t ident_index; Data() : empty(nullptr) {} ~Data() {} @@ -430,14 +430,11 @@ class FlatExpressionEvaluatorState { public: FlatExpressionEvaluatorState( size_t value_stack_size, size_t comprehension_slot_count, - const cel::TypeProvider& type_provider, + size_t iterator_stack_size, const cel::TypeProvider& type_provider, const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, google::protobuf::MessageFactory* absl_nullable message_factory = nullptr) : value_stack_(value_stack_size), - // We currently use comprehension_slot_count because it is less of an - // over estimate than value_stack_size. In future we should just - // calculate the correct capacity. - iterator_stack_(comprehension_slot_count), + iterator_stack_(iterator_stack_size), comprehension_slots_(comprehension_slot_count), type_provider_(type_provider), descriptor_pool_(descriptor_pool), @@ -445,15 +442,12 @@ class FlatExpressionEvaluatorState { FlatExpressionEvaluatorState( size_t value_stack_size, size_t comprehension_slot_count, - const cel::TypeProvider& type_provider, + size_t iterator_stack_size, const cel::TypeProvider& type_provider, const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool, google::protobuf::MessageFactory* absl_nonnull message_factory, google::protobuf::Arena* absl_nonnull arena) : value_stack_(value_stack_size), - // We currently use comprehension_slot_count because it is less of an - // over estimate than value_stack_size. In future we should just - // calculate the correct capacity. - iterator_stack_(comprehension_slot_count), + iterator_stack_(iterator_stack_size), comprehension_slots_(comprehension_slot_count), type_provider_(type_provider), descriptor_pool_(descriptor_pool), @@ -692,7 +686,8 @@ class ExecutionFrame : public ExecutionFrameBase { ExecutionPathView flat, const cel::ActivationInterface& activation, const cel::RuntimeOptions& options, FlatExpressionEvaluatorState& state, EvaluationListener callback = EvaluationListener(), - const cel::EmbedderContext* absl_nullable embedder_context = nullptr) + const cel::EmbedderContext* absl_nullable embedder_context = nullptr, + const IdentifierTable& identifiers = EmptyIdentifierTable()) : ExecutionFrameBase(activation, std::move(callback), options, state.type_provider(), state.descriptor_pool(), state.message_factory(), state.arena(), @@ -701,14 +696,16 @@ class ExecutionFrame : public ExecutionFrameBase { execution_path_(flat), value_stack_(&state.value_stack()), iterator_stack_(&state.iterator_stack()), - subexpressions_(&execution_path_, 1) {} + subexpressions_(&execution_path_, 1), + identifiers_(&identifiers) {} ExecutionFrame( absl::Span subexpressions, const cel::ActivationInterface& activation, const cel::RuntimeOptions& options, FlatExpressionEvaluatorState& state, EvaluationListener callback = EvaluationListener(), - const cel::EmbedderContext* absl_nullable embedder_context = nullptr) + const cel::EmbedderContext* absl_nullable embedder_context = nullptr, + const IdentifierTable& identifiers = EmptyIdentifierTable()) : ExecutionFrameBase(activation, std::move(callback), options, state.type_provider(), state.descriptor_pool(), state.message_factory(), state.arena(), @@ -717,7 +714,8 @@ class ExecutionFrame : public ExecutionFrameBase { execution_path_(subexpressions[0]), value_stack_(&state.value_stack()), iterator_stack_(&state.iterator_stack()), - subexpressions_(subexpressions) { + subexpressions_(subexpressions), + identifiers_(&identifiers) { ABSL_DCHECK(!subexpressions.empty()); } @@ -779,6 +777,8 @@ class ExecutionFrame : public ExecutionFrameBase { return *iterator_stack_; } + const IdentifierTable& identifiers() const { return *identifiers_; } + bool enable_attribute_tracking() const { return attribute_tracking_enabled(); } @@ -816,6 +816,11 @@ class ExecutionFrame : public ExecutionFrameBase { } private: + static const IdentifierTable& EmptyIdentifierTable() { + static const absl::NoDestructor kEmpty; + return *kEmpty; + } + struct SubFrame { size_t return_pc; size_t slot_index; @@ -843,6 +848,7 @@ class ExecutionFrame : public ExecutionFrameBase { EvaluatorStack* absl_nonnull const value_stack_; cel::runtime_internal::IteratorStack* absl_nonnull const iterator_stack_; absl::Span subexpressions_; + const IdentifierTable* absl_nonnull identifiers_; std::vector call_stack_; }; @@ -855,26 +861,32 @@ class FlatExpression { FlatExpression(ExecutionPath path, size_t comprehension_slots_size, const cel::TypeProvider& type_provider, const cel::RuntimeOptions& options, - absl_nullable std::shared_ptr arena = nullptr) + absl_nullable std::shared_ptr arena, + IdentifierTable identifiers) : path_(std::move(path)), subexpressions_({path_}), comprehension_slots_size_(comprehension_slots_size), + iterator_stack_size_(comprehension_slots_size), type_provider_(type_provider), options_(options), - arena_(std::move(arena)) {} + arena_(std::move(arena)), + identifiers_(std::move(identifiers)) {} FlatExpression(ExecutionPath path, std::vector subexpressions, - size_t comprehension_slots_size, + size_t comprehension_slots_size, size_t iterator_stack_size, const cel::TypeProvider& type_provider, const cel::RuntimeOptions& options, - absl_nullable std::shared_ptr arena = nullptr) + absl_nullable std::shared_ptr arena, + IdentifierTable identifiers) : path_(std::move(path)), subexpressions_(std::move(subexpressions)), comprehension_slots_size_(comprehension_slots_size), + iterator_stack_size_(iterator_stack_size), type_provider_(type_provider), options_(options), - arena_(std::move(arena)) {} + arena_(std::move(arena)), + identifiers_(std::move(identifiers)) {} // Move-only FlatExpression(FlatExpression&&) = default; @@ -910,21 +922,27 @@ class FlatExpression { return subexpressions_; } + const IdentifierTable& identifiers() const { return identifiers_; } + const cel::RuntimeOptions& options() const { return options_; } size_t comprehension_slots_size() const { return comprehension_slots_size_; } + size_t iterator_stack_size() const { return iterator_stack_size_; } + const cel::TypeProvider& type_provider() const { return type_provider_; } private: ExecutionPath path_; std::vector subexpressions_; size_t comprehension_slots_size_; + size_t iterator_stack_size_; const cel::TypeProvider& type_provider_; cel::RuntimeOptions options_; // Arena used during planning phase, may hold constant values so should be // kept alive. absl_nullable std::shared_ptr arena_; + IdentifierTable identifiers_; }; // Helper functions for checking ExpressionStep kinds. Used for program @@ -974,9 +992,6 @@ inline ExpressionStep::~ExpressionStep() { case ExpressionStepKind::kOtherConstant: delete u_.other_val; break; - case ExpressionStepKind::kIdentifier: - delete u_.identifier; - break; case ExpressionStepKind::kEagerFunction: delete u_.eager_function_step; break; @@ -1006,6 +1021,7 @@ inline ExpressionStep::~ExpressionStep() { case ExpressionStepKind::kBooleanAndJump: case ExpressionStepKind::kTernaryJump: case ExpressionStepKind::kFixedJump: + case ExpressionStepKind::kIdentifier: case ExpressionStepKind::kFastIn: case ExpressionStepKind::kFastEqual: case ExpressionStepKind::kFastNotEqual: @@ -1178,7 +1194,7 @@ inline void ExpressionStep::Evaluate(ExecutionFrame& frame) const { frame.JumpToOrAbort(u_.fixed_jump_step.offset); break; case ExpressionStepKind::kIdentifier: - EvaluateIdentifierStep(*u_.identifier, frame); + EvaluateIdentifierStep(u_.ident_index, frame); break; case ExpressionStepKind::kEagerFunction: u_.eager_function_step->Evaluate(frame); diff --git a/eval/eval/evaluator_core_test.cc b/eval/eval/evaluator_core_test.cc index c67dc8b58..4b62f1024 100644 --- a/eval/eval/evaluator_core_test.cc +++ b/eval/eval/evaluator_core_test.cc @@ -84,7 +84,7 @@ TEST(EvaluatorCoreTest, ExecutionFrameNext) { cel::Activation activation; FlatExpressionEvaluatorState state( path.size(), - /*comprehension_slots_size=*/0, type_provider, + /*comprehension_slot_count=*/0, /*iterator_stack_size=*/0, type_provider, cel::internal::GetTestingDescriptorPool(), cel::internal::GetTestingMessageFactory(), &arena); ExecutionFrame frame(path, activation, options, state); @@ -111,7 +111,7 @@ TEST(EvaluatorCoreTest, JumpOutOfRangeAborts) { cel::Activation activation; FlatExpressionEvaluatorState state( path.size(), - /*comprehension_slots_size=*/0, type_provider, + /*comprehension_slot_count=*/0, /*iterator_stack_size=*/0, type_provider, cel::internal::GetTestingDescriptorPool(), cel::internal::GetTestingMessageFactory(), &arena); ExecutionFrame frame(path, activation, options, state); @@ -190,7 +190,7 @@ TEST(EvaluatorCoreTest, SimpleEvaluatorTest) { CelExpressionFlatImpl impl( env, FlatExpression(std::move(path), 0, env->type_registry.GetComposedTypeProvider(), - cel::RuntimeOptions{})); + cel::RuntimeOptions{}, /*arena=*/nullptr, {})); Activation activation; google::protobuf::Arena arena; @@ -214,7 +214,8 @@ TEST(EvaluatorCoreTest, MakeConstant) { [&](const cel::Value& value) -> absl::StatusOr { ExecutionPath path; path.push_back(ExpressionStep::MakeConstant(value)); - FlatExpression expr(std::move(path), 0, type_provider, options); + FlatExpression expr(std::move(path), 0, type_provider, options, + /*arena=*/nullptr, {}); auto state = expr.MakeEvaluatorState( cel::internal::GetTestingDescriptorPool(), cel::internal::GetTestingMessageFactory(), &arena); @@ -279,7 +280,7 @@ TEST(EvaluatorCoreTest, TraceFilterById) { CelExpressionFlatImpl impl( env, FlatExpression(std::move(path), 0, env->type_registry.GetComposedTypeProvider(), - cel::RuntimeOptions{})); + cel::RuntimeOptions{}, /*arena=*/nullptr, {})); Activation activation; google::protobuf::Arena arena; diff --git a/eval/eval/function_step_test.cc b/eval/eval/function_step_test.cc index 9b9ce6a36..4b165e5a5 100644 --- a/eval/eval/function_step_test.cc +++ b/eval/eval/function_step_test.cc @@ -236,7 +236,8 @@ class FunctionStepTest return std::make_unique( env, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, {})); } }; @@ -604,7 +605,8 @@ INSTANTIATE_TEST_SUITE_P( class FunctionStepTestUnknowns : public testing::TestWithParam { public: - std::unique_ptr GetExpression(ExecutionPath&& path) { + std::unique_ptr GetExpression( + ExecutionPath&& path, IdentifierTable identifiers = {}) { cel::RuntimeOptions options; options.unknown_processing = GetParam(); @@ -612,7 +614,8 @@ class FunctionStepTestUnknowns return std::make_unique( env, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, std::move(identifiers))); } }; @@ -654,13 +657,14 @@ TEST_P(FunctionStepTestUnknowns, PartialUnknownHandlingTest) { IdentExpr ident1; ident1.set_name("param"); CallExpr call1 = SinkFunction::MakeCall(); - auto step0 = ExpressionStep::MakeIdentifierStep("param", GetExprId()); + ExpressionStep step0 = ExpressionStep::MakeIdentifierStep(0, GetExprId()); ASSERT_OK_AND_ASSIGN(auto step1, MakeTestFunctionStep(call1, registry)); path.push_back(std::move(step0)); path.push_back(std::move(step1)); - std::unique_ptr impl = GetExpression(std::move(path)); + std::unique_ptr impl = + GetExpression(std::move(path), IdentifierTable{{"param"}}); Activation activation; TestMessage msg; @@ -748,10 +752,10 @@ TEST(FunctionStepTestUnknownFunctionResults, CaptureArgs) { cel::UnknownProcessingOptions::kAttributeAndFunction; auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( - env, - FlatExpression(std::move(path), - /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), + /*comprehension_slot_count=*/0, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, {})); Activation activation; google::protobuf::Arena arena; @@ -798,10 +802,10 @@ TEST(FunctionStepTestUnknownFunctionResults, MergeDownCaptureArgs) { cel::UnknownProcessingOptions::kAttributeAndFunction; auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( - env, - FlatExpression(std::move(path), - /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), + /*comprehension_slot_count=*/0, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, {})); Activation activation; google::protobuf::Arena arena; @@ -848,10 +852,10 @@ TEST(FunctionStepTestUnknownFunctionResults, MergeCaptureArgs) { cel::UnknownProcessingOptions::kAttributeAndFunction; auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( - env, - FlatExpression(std::move(path), - /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), + /*comprehension_slot_count=*/0, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, {})); Activation activation; google::protobuf::Arena arena; @@ -893,10 +897,10 @@ TEST(FunctionStepTestUnknownFunctionResults, UnknownVsErrorPrecedenceTest) { cel::UnknownProcessingOptions::kAttributeAndFunction; auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( - env, - FlatExpression(std::move(path), - /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), + /*comprehension_slot_count=*/0, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, {})); Activation activation; google::protobuf::Arena arena; @@ -983,10 +987,10 @@ TEST(FunctionStepStrictnessTest, cel::UnknownProcessingOptions::kAttributeAndFunction; auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( - env, - FlatExpression(std::move(path), - /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), + /*comprehension_slot_count=*/0, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, {})); Activation activation; google::protobuf::Arena arena; ASSERT_OK_AND_ASSIGN(CelValue value, impl.Evaluate(activation, &arena)); @@ -1015,10 +1019,10 @@ TEST(FunctionStepStrictnessTest, IfFunctionNonStrictAndGivenUnknownInvokesIt) { cel::UnknownProcessingOptions::kAttributeAndFunction; auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( - env, - FlatExpression(std::move(path), - /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), + /*comprehension_slot_count=*/0, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, {})); Activation activation; google::protobuf::Arena arena; ASSERT_OK_AND_ASSIGN(CelValue value, impl.Evaluate(activation, &arena)); diff --git a/eval/eval/ident_step.cc b/eval/eval/ident_step.cc index 0bfbb9211..93d639dad 100644 --- a/eval/eval/ident_step.cc +++ b/eval/eval/ident_step.cc @@ -1,15 +1,17 @@ #include "eval/eval/ident_step.h" +#include #include #include +#include "absl/base/optimization.h" #include "absl/status/status.h" #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "common/value.h" #include "eval/eval/attribute_trail.h" +#include "eval/eval/comprehension_slots.h" #include "eval/eval/evaluator_core.h" -#include "eval/eval/expression_step_logic.h" #include "eval/internal/errors.h" #include "internal/status_macros.h" @@ -40,7 +42,7 @@ absl::Status LookupIdent(absl::string_view name, ExecutionFrameBase& frame, } CEL_ASSIGN_OR_RETURN( - auto found, frame.activation().FindVariable(name, frame.descriptor_pool(), + bool found, frame.activation().FindVariable(name, frame.descriptor_pool(), frame.message_factory(), frame.arena(), &result)); @@ -58,16 +60,32 @@ absl::Status LookupIdent(absl::string_view name, ExecutionFrameBase& frame, } // namespace -void EvaluateIdentifierStep(absl::string_view identifier, - ExecutionFrame& frame) { +void EvaluateIdentifierStep(size_t ident_index, ExecutionFrame& frame) { + if (ABSL_PREDICT_FALSE(ident_index >= frame.identifiers().size())) { + frame.Abort(absl::InternalError( + absl::StrCat("Identifier index out of range: ", ident_index))); + return; + } + const Identifier& ident = frame.identifiers()[ident_index]; + ComprehensionSlot* slot = nullptr; + if (ident.slot_index != Identifier::kNoSlot) { + slot = frame.comprehension_slots().Get(ident.slot_index); + if (slot->Has()) { + frame.value_stack().Push(slot->value(), slot->attribute()); + return; + } + } frame.value_stack().Push(cel::NullValue()); if (absl::Status status = - LookupIdent(identifier, frame, frame.value_stack().Peek(), + LookupIdent(ident.name, frame, frame.value_stack().Peek(), frame.value_stack().PeekAttribute()); !status.ok()) { frame.Abort(std::move(status)); return; } + if (slot != nullptr) { + slot->Set(frame.value_stack().Peek(), frame.value_stack().PeekAttribute()); + } } } // namespace google::api::expr::runtime diff --git a/eval/eval/ident_step.h b/eval/eval/ident_step.h index 2ce50c58e..ecb621d44 100644 --- a/eval/eval/ident_step.h +++ b/eval/eval/ident_step.h @@ -1,14 +1,25 @@ #ifndef THIRD_PARTY_CEL_CPP_EVAL_EVAL_IDENT_STEP_H_ #define THIRD_PARTY_CEL_CPP_EVAL_EVAL_IDENT_STEP_H_ -#include "absl/strings/string_view.h" +#include +#include +#include +#include namespace google::api::expr::runtime { class ExecutionFrame; -void EvaluateIdentifierStep(absl::string_view identifier, - ExecutionFrame& frame); +struct Identifier { + static constexpr size_t kNoSlot = std::numeric_limits::max(); + + std::string name; + size_t slot_index = kNoSlot; +}; + +using IdentifierTable = std::vector; + +void EvaluateIdentifierStep(size_t ident_index, ExecutionFrame& frame); } // namespace google::api::expr::runtime diff --git a/eval/eval/ident_step_test.cc b/eval/eval/ident_step_test.cc index 49ef870d6..9ad82d9cb 100644 --- a/eval/eval/ident_step_test.cc +++ b/eval/eval/ident_step_test.cc @@ -32,20 +32,21 @@ using ::testing::Eq; TEST(IdentStepTest, TestIdentStep) { ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("name0")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( env, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, env->type_registry.GetComposedTypeProvider(), - cel::RuntimeOptions{})); + cel::RuntimeOptions{}, /*arena=*/nullptr, + IdentifierTable{{"name0"}})); Activation activation; Arena arena; std::string value("test"); activation.InsertValue("name0", CelValue::CreateString(&value)); - auto status0 = impl.Evaluate(activation, &arena); + absl::StatusOr status0 = impl.Evaluate(activation, &arena); ASSERT_THAT(status0, IsOk()); CelValue result = status0.value(); @@ -54,21 +55,72 @@ TEST(IdentStepTest, TestIdentStep) { EXPECT_THAT(result.StringOrDie().value(), Eq("test")); } +TEST(IdentStepTest, TestIdentStepWithSlotCachesLookup) { + ExecutionPath path; + // Evaluate "name0" twice and check equality (or return the second after + // popping the first viaComprehensionFinish / ReadSlot). Here we read the + // identifier step twice and then read slot 1 directly. + path.push_back(ExpressionStep::MakeIdentifierStep(0)); + path.push_back(ExpressionStep::MakeAssignSlotAndPopStep(0)); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); + + auto env = NewTestingRuntimeEnv(); + CelExpressionFlatImpl impl( + env, FlatExpression(std::move(path), /*comprehension_slot_count=*/2, + env->type_registry.GetComposedTypeProvider(), + cel::RuntimeOptions{}, /*arena=*/nullptr, + IdentifierTable{{"name0", /*slot_index=*/1}})); + + class CountingActivation : public BaseActivation { + public: + CountingActivation(int* counter, const std::string* val) + : counter_(counter), val_(val) {} + std::vector FindFunctionOverloads( + absl::string_view) const override { + return {}; + } + absl::optional FindValue(absl::string_view name, + Arena*) const override { + if (name == "name0") { + (*counter_)++; + return CelValue::CreateString(val_); + } + return absl::nullopt; + } + + private: + int* counter_; + const std::string* val_; + }; + + Arena arena; + int lookup_calls = 0; + std::string value("cached_value"); + CountingActivation activation(&lookup_calls, &value); + + absl::StatusOr status0 = impl.Evaluate(activation, &arena); + ASSERT_THAT(status0, IsOk()); + ASSERT_TRUE(status0->IsString()); + EXPECT_THAT(status0->StringOrDie().value(), Eq("cached_value")); + EXPECT_EQ(lookup_calls, 1); +} + TEST(IdentStepTest, TestIdentStepNameNotFound) { ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("name0")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( env, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, env->type_registry.GetComposedTypeProvider(), - cel::RuntimeOptions{})); + cel::RuntimeOptions{}, /*arena=*/nullptr, + IdentifierTable{{"name0"}})); Activation activation; Arena arena; std::string value("test"); - auto status0 = impl.Evaluate(activation, &arena); + absl::StatusOr status0 = impl.Evaluate(activation, &arena); ASSERT_THAT(status0, IsOk()); CelValue result = status0.value(); @@ -77,22 +129,22 @@ TEST(IdentStepTest, TestIdentStepNameNotFound) { TEST(IdentStepTest, DisableMissingAttributeErrorsOK) { ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("name0")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); cel::RuntimeOptions options; options.unknown_processing = cel::UnknownProcessingOptions::kDisabled; auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( - env, - FlatExpression(std::move(path), - /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), + /*comprehension_slot_count=*/0, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, IdentifierTable{{"name0"}})); Activation activation; Arena arena; std::string value("test"); activation.InsertValue("name0", CelValue::CreateString(&value)); - auto status0 = impl.Evaluate(activation, &arena); + absl::StatusOr status0 = impl.Evaluate(activation, &arena); ASSERT_THAT(status0, IsOk()); CelValue result = status0.value(); @@ -111,7 +163,7 @@ TEST(IdentStepTest, DisableMissingAttributeErrorsOK) { TEST(IdentStepTest, TestIdentStepMissingAttributeErrors) { ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("name0")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); cel::RuntimeOptions options; options.unknown_processing = cel::UnknownProcessingOptions::kDisabled; @@ -119,17 +171,17 @@ TEST(IdentStepTest, TestIdentStepMissingAttributeErrors) { auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( - env, - FlatExpression(std::move(path), - /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), + /*comprehension_slot_count=*/0, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, IdentifierTable{{"name0"}})); Activation activation; Arena arena; std::string value("test"); activation.InsertValue("name0", CelValue::CreateString(&value)); - auto status0 = impl.Evaluate(activation, &arena); + absl::StatusOr status0 = impl.Evaluate(activation, &arena); ASSERT_THAT(status0, IsOk()); CelValue result = status0.value(); @@ -149,17 +201,17 @@ TEST(IdentStepTest, TestIdentStepMissingAttributeErrors) { TEST(IdentStepTest, TestIdentStepUnknownAttribute) { ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("name0")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); // Expression with unknowns enabled. cel::RuntimeOptions options; options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl impl( - env, - FlatExpression(std::move(path), - /*comprehension_slot_count=*/0, - env->type_registry.GetComposedTypeProvider(), options)); + env, FlatExpression(std::move(path), + /*comprehension_slot_count=*/0, + env->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, IdentifierTable{{"name0"}})); Activation activation; Arena arena; @@ -170,7 +222,7 @@ TEST(IdentStepTest, TestIdentStepUnknownAttribute) { unknown_patterns.push_back(CelAttributePattern("name_bad", {})); ASSERT_THAT(activation.SetUnknownAttributePatterns(unknown_patterns), IsOk()); - auto status0 = impl.Evaluate(activation, &arena); + absl::StatusOr status0 = impl.Evaluate(activation, &arena); ASSERT_THAT(status0, IsOk()); CelValue result = status0.value(); diff --git a/eval/eval/iterator_stack.h b/eval/eval/iterator_stack.h index 0b0de14aa..ccb377dd3 100644 --- a/eval/eval/iterator_stack.h +++ b/eval/eval/iterator_stack.h @@ -20,6 +20,7 @@ #include #include "absl/base/nullability.h" +#include "absl/container/inlined_vector.h" #include "absl/log/absl_check.h" #include "common/value.h" @@ -92,7 +93,7 @@ class IteratorStack final { } private: - std::vector entries_; + absl::InlinedVector entries_; size_t max_size_; }; diff --git a/eval/eval/lazy_init_step_test.cc b/eval/eval/lazy_init_step_test.cc index 3b5282d21..d7211e597 100644 --- a/eval/eval/lazy_init_step_test.cc +++ b/eval/eval/lazy_init_step_test.cc @@ -46,7 +46,8 @@ class LazyInitStepTest : public testing::Test { public: LazyInitStepTest() : type_provider_(cel::internal::GetTestingDescriptorPool()), - evaluator_state_(kValueStack, kComprehensionSlotCount, type_provider_, + evaluator_state_(kValueStack, kComprehensionSlotCount, + /*iterator_stack_size=*/0, type_provider_, cel::internal::GetTestingDescriptorPool(), cel::internal::GetTestingMessageFactory(), &arena_) {} diff --git a/eval/eval/logic_step_test.cc b/eval/eval/logic_step_test.cc index 59888b20a..354772804 100644 --- a/eval/eval/logic_step_test.cc +++ b/eval/eval/logic_step_test.cc @@ -39,8 +39,8 @@ class LogicStepTest : public testing::TestWithParam { absl::Status EvaluateLogic(CelValue arg0, CelValue arg1, bool is_or, CelValue* result, bool enable_unknown) { ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("name0")); - path.push_back(ExpressionStep::MakeIdentifierStep("name1")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); + path.push_back(ExpressionStep::MakeIdentifierStep(1)); path.push_back( (is_or) ? ExpressionStep::MakeBooleanOrStep(/*num_args=*/2, /*id=*/2) : ExpressionStep::MakeBooleanAndStep(/*num_args=*/2, /*id=*/2)); @@ -52,9 +52,10 @@ class LogicStepTest : public testing::TestWithParam { cel::UnknownProcessingOptions::kAttributeOnly; } CelExpressionFlatImpl impl( - env_, - FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env_->type_registry.GetComposedTypeProvider(), options)); + env_, FlatExpression( + std::move(path), /*comprehension_slot_count=*/0, + env_->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, IdentifierTable{{"name0"}, {"name1"}})); Activation activation; activation.InsertValue("name0", arg0); diff --git a/eval/eval/select_step_test.cc b/eval/eval/select_step_test.cc index 4963550b4..038a370b1 100644 --- a/eval/eval/select_step_test.cc +++ b/eval/eval/select_step_test.cc @@ -85,8 +85,7 @@ class SelectStepTest : public testing::Test { CreateSelectStep(select.field(), select.test_only(), options.enable_wrapper_type_null_unboxing)); - path.push_back( - ExpressionStep::MakeIdentifierStep(ident.name(), expr0.id())); + path.push_back(ExpressionStep::MakeIdentifierStep(0, expr0.id())); path.push_back( ExpressionStep::MakeGenericStep(std::move(step1), expr.id())); @@ -100,7 +99,8 @@ class SelectStepTest : public testing::Test { CelExpressionFlatImpl cel_expr( env_, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, env_->type_registry.GetComposedTypeProvider(), - runtime_options)); + runtime_options, /*arena=*/nullptr, + IdentifierTable{{ident.name()}})); Activation activation; activation.InsertValue("target", target); @@ -273,14 +273,15 @@ TEST_F(SelectStepTest, MapPresenseIsErrorTest) { /*enable_wrapper_type_null_unboxing=*/false)); ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep(ident.name(), expr0.id())); + path.push_back(ExpressionStep::MakeIdentifierStep(0, expr0.id())); path.push_back(ExpressionStep::MakeGenericStep(std::move(step1), expr1.id())); path.push_back( ExpressionStep::MakeGenericStep(std::move(step2), select_expr.id())); CelExpressionFlatImpl cel_expr( env_, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, env_->type_registry.GetComposedTypeProvider(), - cel::RuntimeOptions{})); + cel::RuntimeOptions{}, /*arena=*/nullptr, + IdentifierTable{{ident.name()}})); Activation activation; activation.InsertValue("target", CelProtoWrapper::CreateMessage(&message, &arena_)); @@ -708,7 +709,7 @@ TEST_P(SelectStepConformanceTest, CelErrorAsArgument) { CreateSelectStep(select.field(), select.test_only(), /*enable_wrapper_type_null_unboxing=*/false)); - path.push_back(ExpressionStep::MakeIdentifierStep(ident.name(), expr0.id())); + path.push_back(ExpressionStep::MakeIdentifierStep(0, expr0.id())); path.push_back( ExpressionStep::MakeGenericStep(std::move(step1), dummy_expr.id())); @@ -721,7 +722,8 @@ TEST_P(SelectStepConformanceTest, CelErrorAsArgument) { CelExpressionFlatImpl cel_expr( env_, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env_->type_registry.GetComposedTypeProvider(), options)); + env_->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, IdentifierTable{{ident.name()}})); Activation activation; activation.InsertValue("message", CelValue::CreateError(&error)); @@ -749,14 +751,15 @@ TEST_F(SelectStepTest, DisableMissingAttributeOK) { CreateSelectStep(select.field(), select.test_only(), /*enable_wrapper_type_null_unboxing=*/false)); - path.push_back(ExpressionStep::MakeIdentifierStep(ident.name(), expr0.id())); + path.push_back(ExpressionStep::MakeIdentifierStep(0, expr0.id())); path.push_back( ExpressionStep::MakeGenericStep(std::move(step1), dummy_expr.id())); CelExpressionFlatImpl cel_expr( env_, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, env_->type_registry.GetComposedTypeProvider(), - cel::RuntimeOptions{})); + cel::RuntimeOptions{}, /*arena=*/nullptr, + IdentifierTable{{ident.name()}})); Activation activation; activation.InsertValue("message", CelProtoWrapper::CreateMessage(&message, &arena_)); @@ -791,7 +794,7 @@ TEST_F(SelectStepTest, UnrecoverableUnknownValueProducesError) { CreateSelectStep(select.field(), select.test_only(), /*enable_wrapper_type_null_unboxing=*/false)); - path.push_back(ExpressionStep::MakeIdentifierStep(ident.name(), expr0.id())); + path.push_back(ExpressionStep::MakeIdentifierStep(0, expr0.id())); path.push_back( ExpressionStep::MakeGenericStep(std::move(step1), dummy_expr.id())); @@ -800,7 +803,8 @@ TEST_F(SelectStepTest, UnrecoverableUnknownValueProducesError) { CelExpressionFlatImpl cel_expr( env_, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env_->type_registry.GetComposedTypeProvider(), options)); + env_->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, IdentifierTable{{ident.name()}})); Activation activation; activation.InsertValue("message", CelProtoWrapper::CreateMessage(&message, &arena_)); @@ -840,7 +844,7 @@ TEST_F(SelectStepTest, UnknownPatternResolvesToUnknown) { ASSERT_THAT(step1_status, IsOk()); - path.push_back(ExpressionStep::MakeIdentifierStep(ident.name(), expr0.id())); + path.push_back(ExpressionStep::MakeIdentifierStep(0, expr0.id())); path.push_back(ExpressionStep::MakeGenericStep(std::move(*step1_status), dummy_expr.id())); @@ -849,7 +853,8 @@ TEST_F(SelectStepTest, UnknownPatternResolvesToUnknown) { CelExpressionFlatImpl cel_expr( env_, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env_->type_registry.GetComposedTypeProvider(), options)); + env_->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, IdentifierTable{{ident.name()}})); { std::vector unknown_patterns; @@ -947,7 +952,7 @@ TEST_P(SelectStepConformanceTest, TypedSelectStepTest) { /*enable_optional_types=*/false)); ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("message")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); path.push_back(ExpressionStep::MakeGenericStep(std::move(step1))); cel::RuntimeOptions options; if (GetParam()) { @@ -956,7 +961,8 @@ TEST_P(SelectStepConformanceTest, TypedSelectStepTest) { CelExpressionFlatImpl cel_expr( env_, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env_->type_registry.GetComposedTypeProvider(), options)); + env_->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, IdentifierTable{{"message"}})); TestAllTypes message; message.set_single_int64(42); @@ -985,7 +991,7 @@ TEST_P(SelectStepConformanceTest, TypedSelectStepPropagatesUnknown) { /*enable_optional_types=*/false)); ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("message")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); path.push_back(ExpressionStep::MakeGenericStep(std::move(step1))); cel::RuntimeOptions options; if (GetParam()) { @@ -994,7 +1000,8 @@ TEST_P(SelectStepConformanceTest, TypedSelectStepPropagatesUnknown) { CelExpressionFlatImpl cel_expr( env_, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env_->type_registry.GetComposedTypeProvider(), options)); + env_->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, IdentifierTable{{"message"}})); UnknownSet unknown_set(UnknownAttributeSet({CelAttribute("message", {})})); Activation activation; @@ -1020,14 +1027,15 @@ TEST_F(SelectStepTest, TypedSelectStepUnknownPatternResolvesToUnknown) { /*enable_optional_types=*/false)); ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("message")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); path.push_back(ExpressionStep::MakeGenericStep(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)); + env_->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, IdentifierTable{{"message"}})); TestAllTypes message; message.set_single_int64(42); diff --git a/eval/eval/shadowable_value_step_test.cc b/eval/eval/shadowable_value_step_test.cc index 42234b515..46721f100 100644 --- a/eval/eval/shadowable_value_step_test.cc +++ b/eval/eval/shadowable_value_step_test.cc @@ -44,7 +44,7 @@ absl::StatusOr RunShadowableExpression( CelExpressionFlatImpl impl( env, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, env->type_registry.GetComposedTypeProvider(), - cel::RuntimeOptions{})); + cel::RuntimeOptions{}, /*arena=*/nullptr, {})); return impl.Evaluate(activation, arena); } diff --git a/eval/eval/ternary_step_test.cc b/eval/eval/ternary_step_test.cc index d1011f247..3ee81ec04 100644 --- a/eval/eval/ternary_step_test.cc +++ b/eval/eval/ternary_step_test.cc @@ -9,6 +9,7 @@ #include "base/type_provider.h" #include "eval/eval/cel_expression_flat_impl.h" #include "eval/eval/evaluator_core.h" +#include "eval/eval/ident_step.h" #include "eval/public/activation.h" #include "eval/public/cel_value.h" #include "internal/testing.h" @@ -36,9 +37,9 @@ class LogicStepTest : public testing::TestWithParam { CelValue* result, bool enable_unknown) { ExecutionPath path; - path.push_back(ExpressionStep::MakeIdentifierStep("name0")); - path.push_back(ExpressionStep::MakeIdentifierStep("name1")); - path.push_back(ExpressionStep::MakeIdentifierStep("name2")); + path.push_back(ExpressionStep::MakeIdentifierStep(0)); + path.push_back(ExpressionStep::MakeIdentifierStep(1)); + path.push_back(ExpressionStep::MakeIdentifierStep(2)); path.push_back(ExpressionStep::MakeGenericStep(CreateTernaryStep(), 4)); cel::RuntimeOptions options; @@ -49,7 +50,9 @@ class LogicStepTest : public testing::TestWithParam { CelExpressionFlatImpl impl( env_, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, - env_->type_registry.GetComposedTypeProvider(), options)); + env_->type_registry.GetComposedTypeProvider(), options, + /*arena=*/nullptr, + IdentifierTable{{"name0"}, {"name1"}, {"name2"}})); Activation activation; std::string value("test");