From 18789ecaf8cd193d8d060f76c2b50056cc8a4988 Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Tue, 13 Oct 2026 13:39:52 -0700 Subject: [PATCH] Add instruction for list steps. PiperOrigin-RevId: 998861772 --- eval/compiler/BUILD | 2 - eval/compiler/constant_folding_test.cc | 12 +- eval/compiler/flat_expr_builder.cc | 23 +++- eval/eval/BUILD | 28 +---- eval/eval/create_list_step.cc | 159 ++++++++++++++----------- eval/eval/create_list_step.h | 44 ++++++- eval/eval/create_list_step_test.cc | 12 +- eval/eval/evaluator_core.h | 30 +++++ 8 files changed, 189 insertions(+), 121 deletions(-) diff --git a/eval/compiler/BUILD b/eval/compiler/BUILD index 7edf542ae..8d9a5aa3b 100644 --- a/eval/compiler/BUILD +++ b/eval/compiler/BUILD @@ -102,7 +102,6 @@ cc_library( "//common:type_spec_resolver", "//common:value", "//eval/eval:container_access_step", - "//eval/eval:create_list_step", "//eval/eval:create_map_step", "//eval/eval:create_struct_step", "//eval/eval:evaluator_core", @@ -333,7 +332,6 @@ cc_test( "//base:ast", "//common:expr", "//common:value", - "//eval/eval:create_list_step", "//eval/eval:create_map_step", "//eval/eval:evaluator_core", "//extensions/protobuf:ast_converters", diff --git a/eval/compiler/constant_folding_test.cc b/eval/compiler/constant_folding_test.cc index 8c6fcf54c..d9ba91bfa 100644 --- a/eval/compiler/constant_folding_test.cc +++ b/eval/compiler/constant_folding_test.cc @@ -298,10 +298,8 @@ TEST_F(UpdatedConstantFoldingTest, CreatesList) { program_builder.ExitSubexpression(&elem_two); // createlist - ASSERT_OK_AND_ASSIGN(auto step, - CreateCreateListStep(create_list.list_expr())); - program_builder.AddStep( - ExpressionStep::MakeGenericStep(std::move(step), create_list.id())); + program_builder.AddStep(CreateCreateListStep( + create_list.list_expr().elements().size(), {}, create_list.id())); program_builder.ExitSubexpression(&create_list); std::shared_ptr arena; @@ -376,10 +374,8 @@ TEST_F(UpdatedConstantFoldingTest, CreatesLargeList) { program_builder.ExitSubexpression(&elem4); // createlist - ASSERT_OK_AND_ASSIGN(auto step_large, - CreateCreateListStep(create_list.list_expr())); - program_builder.AddStep( - ExpressionStep::MakeGenericStep(std::move(step_large), create_list.id())); + program_builder.AddStep(CreateCreateListStep( + create_list.list_expr().elements().size(), {}, create_list.id())); program_builder.ExitSubexpression(&create_list); std::shared_ptr arena; diff --git a/eval/compiler/flat_expr_builder.cc b/eval/compiler/flat_expr_builder.cc index fa0d556b6..5677f2a98 100644 --- a/eval/compiler/flat_expr_builder.cc +++ b/eval/compiler/flat_expr_builder.cc @@ -507,6 +507,17 @@ absl::flat_hash_set MakeOptionalIndicesSet( return optional_indices; } +absl::flat_hash_set MakeOptionalIndicesSet( + const cel::ListExpr& list_expr) { + absl::flat_hash_set optional_indices; + for (size_t i = 0; i < list_expr.elements().size(); ++i) { + if (list_expr.elements()[i].optional()) { + optional_indices.insert(i); + } + } + return optional_indices; +} + class FlatExprVisitor : public cel::AstVisitor { public: enum class CallHandlerResult { @@ -1350,7 +1361,17 @@ class FlatExprVisitor : public cel::AstVisitor { } } } - AddStep(CreateCreateListStep(list_expr), expr.id()); + absl::flat_hash_set optional_indices = + MakeOptionalIndicesSet(list_expr); + for (size_t index : optional_indices) { + if (!ValidateOrError(index < list_expr.elements().size(), + "Optional index out of range: ", index, + ", list size: ", list_expr.elements().size())) { + return; + } + } + AddStep(CreateCreateListStep(list_expr.elements().size(), + std::move(optional_indices), expr.id())); } // CreateStruct node handler. diff --git a/eval/eval/BUILD b/eval/eval/BUILD index f505954bf..55b1e5e29 100644 --- a/eval/eval/BUILD +++ b/eval/eval/BUILD @@ -36,6 +36,7 @@ cc_library( name = "evaluator_core", srcs = [ "comprehension_step.cc", + "create_list_step.cc", "equality_steps.cc", "evaluator_core.cc", "function_step.cc", @@ -45,6 +46,7 @@ cc_library( ], hdrs = [ "comprehension_step.h", + "create_list_step.h", "equality_steps.h", "evaluator_core.h", "function_step.h", @@ -65,6 +67,7 @@ cc_library( "//common:expr", "//common:function_descriptor", "//common:kind", + "//common:optional_ref", "//common:value", "//common:value_kind", "//eval/internal:errors", @@ -83,6 +86,7 @@ cc_library( "@com_google_absl//absl/base", "@com_google_absl//absl/base:core_headers", "@com_google_absl//absl/base:nullability", + "@com_google_absl//absl/container:flat_hash_set", "@com_google_absl//absl/log:absl_check", "@com_google_absl//absl/log:absl_log", "@com_google_absl//absl/status", @@ -302,29 +306,6 @@ cc_library( ], ) -cc_library( - name = "create_list_step", - srcs = [ - "create_list_step.cc", - ], - hdrs = [ - "create_list_step.h", - ], - deps = [ - ":attribute_utility", - ":evaluator_core", - ":expression_step_base", - ":expression_step_logic", - "//common:expr", - "//common:value", - "//internal:status_macros", - "@com_google_absl//absl/container:flat_hash_set", - "@com_google_absl//absl/status", - "@com_google_absl//absl/status:statusor", - "@com_google_absl//absl/types:optional", - ], -) - cc_library( name = "create_struct_step", srcs = [ @@ -658,7 +639,6 @@ cc_test( ], deps = [ ":cel_expression_flat_impl", - ":create_list_step", ":evaluator_core", "//base:attributes", "//base:data", diff --git a/eval/eval/create_list_step.cc b/eval/eval/create_list_step.cc index 20a9b4f37..d812486f2 100644 --- a/eval/eval/create_list_step.cc +++ b/eval/eval/create_list_step.cc @@ -4,20 +4,17 @@ #include #include #include -#include #include "absl/container/flat_hash_set.h" +#include "absl/log/absl_check.h" #include "absl/status/status.h" -#include "absl/status/statusor.h" #include "absl/types/optional.h" -#include "common/expr.h" +#include "absl/types/span.h" +#include "common/optional_ref.h" #include "common/value.h" #include "common/values/list_value_builder.h" #include "eval/eval/attribute_utility.h" #include "eval/eval/evaluator_core.h" -#include "eval/eval/expression_step_base.h" -#include "eval/eval/expression_step_logic.h" -#include "internal/status_macros.h" namespace google::api::expr::runtime { @@ -29,69 +26,72 @@ using ::cel::UnknownValue; using ::cel::Value; using ::cel::common_internal::NewListValueBuilder; -class CreateListStep : public ExpressionStepBase { - public: - CreateListStep(int list_size, absl::flat_hash_set optional_indices) - : list_size_(list_size), optional_indices_(std::move(optional_indices)) {} +constexpr size_t kMaxSmallListSize = 58; - void Evaluate(ExecutionFrame* frame) const override; +template +struct StepTraits; - private: - absl::Status DoEvaluate(ExecutionFrame* frame, Value* result) const; +template <> +struct StepTraits { + static bool is_optional(const SmallListStepInfo& step, size_t index) { + ABSL_DCHECK_LT(index, kMaxSmallListSize); + return ((step.optional_bit_set >> index) & 1) == 1; + } - int list_size_; - absl::flat_hash_set optional_indices_; + static size_t num_elements(const SmallListStepInfo& step) { + return step.list_size; + } }; -void CreateListStep::Evaluate(ExecutionFrame* frame) const { - if (list_size_ < 0) { - frame->Abort(absl::InternalError("CreateListStep: list size is <0")); - return; +template <> +struct StepTraits { + static bool is_optional(const ListStepInfo& step, size_t index) { + return step.is_optional(index); } - if (!frame->value_stack().HasEnough(list_size_)) { - frame->Abort(absl::InternalError("CreateListStep: stack underflow")); - return; + static size_t num_elements(const ListStepInfo& step) { + return step.num_elements(); } +}; - Value result; - if (absl::Status status = DoEvaluate(frame, &result); !status.ok()) { - frame->Abort(std::move(status)); +template +void EvaluateListStepImpl(const StepInfo& step, ExecutionFrame& frame) { + const size_t num_elements = StepTraits::num_elements(step); + + if (!frame.value_stack().HasEnough(num_elements)) { + frame.Abort(absl::InternalError("CreateListStep: stack underflow")); return; } - frame->value_stack().PopAndPush(list_size_, std::move(result)); -} - -absl::Status CreateListStep::DoEvaluate(ExecutionFrame* frame, - Value* result) const { - auto args = frame->value_stack().GetSpan(list_size_); + absl::Span args = frame.value_stack().GetSpan(num_elements); - for (const auto& arg : args) { - if (arg.IsError()) { - *result = arg; - return absl::OkStatus(); + for (size_t i = 0; i < num_elements; ++i) { + if (args[i].IsError()) { + frame.value_stack().SwapAndPop(num_elements, i); + return; } } - if (frame->enable_unknowns()) { + if (frame.enable_unknowns()) { absl::optional unknown_set = - frame->attribute_utility().IdentifyAndMergeUnknowns( - args, frame->value_stack().GetAttributeSpan(list_size_), - /*use_partial=*/true, frame->arena()); + frame.attribute_utility().IdentifyAndMergeUnknowns( + args, frame.value_stack().GetAttributeSpan(num_elements), + /*use_partial=*/true, frame.arena()); if (unknown_set.has_value()) { - *result = std::move(*unknown_set); - return absl::OkStatus(); + frame.value_stack().PopAndPush(num_elements, std::move(*unknown_set)); + return; } } - ListValueBuilderPtr builder = NewListValueBuilder(frame->arena()); - builder->Reserve(args.size()); + ListValueBuilderPtr builder = NewListValueBuilder(frame.arena()); + builder->Reserve(num_elements); - for (size_t i = 0; i < args.size(); ++i) { - const auto& arg = args[i]; - if (optional_indices_.contains(static_cast(i))) { - if (auto optional_arg = arg.AsOptional(); optional_arg) { + for (size_t i = 0; i < num_elements; ++i) { + const Value& arg = args[i]; + if (StepTraits::is_optional(step, i)) { + if (cel::optional_ref optional_arg = + arg.AsOptional(); + optional_arg.has_value()) { if (!optional_arg->HasValue()) { continue; } @@ -99,42 +99,59 @@ absl::Status CreateListStep::DoEvaluate(ExecutionFrame* frame, optional_arg->Value(&optional_arg_value); if (optional_arg_value.IsError()) { // Error should never be in optional, but better safe than sorry. - *result = std::move(optional_arg_value); - return absl::OkStatus(); + frame.value_stack().PopAndPush(num_elements, + std::move(optional_arg_value)); + return; + } + if (absl::Status status = builder->Add(std::move(optional_arg_value)); + !status.ok()) { + frame.Abort(std::move(status)); + return; } - CEL_RETURN_IF_ERROR(builder->Add(std::move(optional_arg_value))); } else { - *result = cel::TypeConversionError(arg.GetTypeName(), "optional_type", - frame->arena()); - return absl::OkStatus(); + frame.value_stack().PopAndPush( + num_elements, + cel::TypeConversionError(arg.GetTypeName(), "optional_type", + frame.arena())); + return; } } else { - CEL_RETURN_IF_ERROR(builder->Add(arg)); + if (absl::Status status = builder->Add(arg); !status.ok()) { + frame.Abort(std::move(status)); + return; + } } } - *result = std::move(*builder).Build(); - return absl::OkStatus(); + frame.value_stack().PopAndPush(num_elements, std::move(*builder).Build()); } -absl::flat_hash_set MakeOptionalIndicesSet( - const cel::ListExpr& create_list_expr) { - absl::flat_hash_set optional_indices; - for (size_t i = 0; i < create_list_expr.elements().size(); ++i) { - if (create_list_expr.elements()[i].optional()) { - optional_indices.insert(static_cast(i)); - } - } - return optional_indices; +} // namespace + +void EvaluateListStep(const ListStepInfo& step, ExecutionFrame& frame) { + EvaluateListStepImpl(step, frame); } -} // namespace +void EvaluateSmallListStep(const SmallListStepInfo& step, + ExecutionFrame& frame) { + EvaluateListStepImpl(step, frame); +} -absl::StatusOr> CreateCreateListStep( - const cel::ListExpr& create_list_expr) { - return std::make_unique( - create_list_expr.elements().size(), - MakeOptionalIndicesSet(create_list_expr)); +ExpressionStep CreateCreateListStep( + size_t size, absl::flat_hash_set optional_indices, + int64_t expr_id) { + if (size <= kMaxSmallListSize) { + uint64_t optional_bit_set = 0; + for (size_t index : optional_indices) { + ABSL_DCHECK_LT(index, size); + optional_bit_set |= (uint64_t{1} << index); + } + return ExpressionStep::MakeCreateSmallListStep( + SmallListStepInfo{optional_bit_set, size}, expr_id); + } + return ExpressionStep::MakeCreateListStep( + std::make_unique(size, std::move(optional_indices)), + expr_id); } } // namespace google::api::expr::runtime diff --git a/eval/eval/create_list_step.h b/eval/eval/create_list_step.h index 9db49db32..dc8e57ed4 100644 --- a/eval/eval/create_list_step.h +++ b/eval/eval/create_list_step.h @@ -1,17 +1,49 @@ #ifndef THIRD_PARTY_CEL_CPP_EVAL_EVAL_CREATE_LIST_STEP_H_ #define THIRD_PARTY_CEL_CPP_EVAL_EVAL_CREATE_LIST_STEP_H_ -#include +#include +#include +#include -#include "absl/status/statusor.h" -#include "common/expr.h" -#include "eval/eval/expression_step_logic.h" +#include "absl/container/flat_hash_set.h" namespace google::api::expr::runtime { +class ExecutionFrame; +class ExpressionStep; + +// Small list step info is used for lists with 58 or fewer elements with 58 bits +// used as a bitset to indicate which elements are optional. +// List literals are generally small so this covers most cases. +struct SmallListStepInfo { + uint64_t optional_bit_set : 58; + size_t list_size : 6; +}; + +class ListStepInfo { + public: + explicit ListStepInfo(size_t num_elements) : num_elements_(num_elements) {} + ListStepInfo(size_t num_elements, absl::flat_hash_set opt_fields) + : num_elements_(num_elements), opt_fields_(std::move(opt_fields)) {} + + bool is_optional(size_t index) const { return opt_fields_.contains(index); } + + size_t num_elements() const { return num_elements_; } + + private: + size_t num_elements_; + absl::flat_hash_set opt_fields_; +}; + +void EvaluateListStep(const ListStepInfo& step, ExecutionFrame& frame); +void EvaluateSmallListStep(const SmallListStepInfo& step, + ExecutionFrame& frame); + // Factory method for CreateList which constructs an immutable list. -absl::StatusOr> CreateCreateListStep( - const cel::ListExpr& create_list_expr); +// The optional indices are assumed to be valid and in range [0, size). +ExpressionStep CreateCreateListStep( + size_t size, absl::flat_hash_set optional_indices = {}, + int64_t expr_id = -1); } // namespace google::api::expr::runtime diff --git a/eval/eval/create_list_step_test.cc b/eval/eval/create_list_step_test.cc index a8f02b5d8..56ac204b5 100644 --- a/eval/eval/create_list_step_test.cc +++ b/eval/eval/create_list_step_test.cc @@ -62,9 +62,7 @@ absl::StatusOr RunExpression( cel::interop_internal::CreateIntValue(value))); } - CEL_ASSIGN_OR_RETURN(auto step, CreateCreateListStep(create_list)); - path.push_back( - ExpressionStep::MakeGenericStep(std::move(step), dummy_expr.id())); + path.push_back(CreateCreateListStep(values.size(), {}, dummy_expr.id())); cel::RuntimeOptions options; if (enable_unknowns) { options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; @@ -101,9 +99,7 @@ absl::StatusOr RunExpressionWithCelValues( activation.InsertValue(var_name, value); } - CEL_ASSIGN_OR_RETURN(auto step0, CreateCreateListStep(create_list)); - path.push_back( - ExpressionStep::MakeGenericStep(std::move(step0), dummy_expr.id())); + path.push_back(CreateCreateListStep(values.size(), {}, dummy_expr.id())); cel::RuntimeOptions options; if (enable_unknowns) { @@ -137,9 +133,7 @@ TEST(CreateListStepTest, TestCreateListStackUnderflow) { auto& expr0 = create_list.mutable_elements().emplace_back().mutable_expr(); expr0.mutable_const_expr().set_int64_value(1); - ASSERT_OK_AND_ASSIGN(auto step0, CreateCreateListStep(create_list)); - path.push_back( - ExpressionStep::MakeGenericStep(std::move(step0), dummy_expr.id())); + path.push_back(CreateCreateListStep(1, {}, dummy_expr.id())); auto env = NewTestingRuntimeEnv(); CelExpressionFlatImpl cel_expr( diff --git a/eval/eval/evaluator_core.h b/eval/eval/evaluator_core.h index 07d985804..b389c38a6 100644 --- a/eval/eval/evaluator_core.h +++ b/eval/eval/evaluator_core.h @@ -39,6 +39,7 @@ #include "eval/eval/attribute_utility.h" #include "eval/eval/comprehension_slots.h" #include "eval/eval/comprehension_step.h" +#include "eval/eval/create_list_step.h" #include "eval/eval/equality_steps.h" #include "eval/eval/evaluator_stack.h" #include "eval/eval/expression_step_logic.h" @@ -112,6 +113,9 @@ enum class ExpressionStepKind : uint16_t { // Special built-in steps for mutable lists implementing map/filter. kNewMutableList = 31, kMutableListAppend = 32, + // Create list step. + kCreateList = 33, + kCreateSmallList = 34, }; struct BoolJumpStepInfo { @@ -336,6 +340,20 @@ class ExpressionStep { return step; } + static ExpressionStep MakeCreateListStep( + std::unique_ptr step_impl, int64_t id = -1) { + ExpressionStep step(ExpressionStepKind::kCreateList, id); + step.u_.create_list_step = step_impl.release(); + return step; + } + + static ExpressionStep MakeCreateSmallListStep(SmallListStepInfo info, + int64_t id = -1) { + ExpressionStep step(ExpressionStepKind::kCreateSmallList, id); + step.u_.create_small_list_step = info; + return step; + } + private: static ABSL_ATTRIBUTE_ALWAYS_INLINE inline void EvaluateReadSlotStep( size_t slot_index, ExecutionFrame& frame); @@ -406,6 +424,8 @@ class ExpressionStep { EagerFunctionStep* eager_function_step; LazyFunctionStep* lazy_function_step; std::string* identifier; + ListStepInfo* create_list_step; + SmallListStepInfo create_small_list_step; Data() : empty(nullptr) {} ~Data() {} @@ -982,6 +1002,9 @@ inline ExpressionStep::~ExpressionStep() { case ExpressionStepKind::kLazyFunction: delete u_.lazy_function_step; break; + case ExpressionStepKind::kCreateList: + delete u_.create_list_step; + break; case ExpressionStepKind::kMovedFrom: case ExpressionStepKind::kIntConstant: case ExpressionStepKind::kBoolConstant: @@ -1010,6 +1033,7 @@ inline ExpressionStep::~ExpressionStep() { case ExpressionStepKind::kFastNotEqual: case ExpressionStepKind::kNewMutableList: case ExpressionStepKind::kMutableListAppend: + case ExpressionStepKind::kCreateSmallList: break; default: ABSL_UNREACHABLE(); @@ -1202,6 +1226,12 @@ inline void ExpressionStep::Evaluate(ExecutionFrame& frame) const { case ExpressionStepKind::kMutableListAppend: EvaluateMutableListAppendStep(frame); break; + case ExpressionStepKind::kCreateList: + EvaluateListStep(*u_.create_list_step, frame); + break; + case ExpressionStepKind::kCreateSmallList: + EvaluateSmallListStep(u_.create_small_list_step, frame); + break; case ExpressionStepKind::kMovedFrom: frame.Abort(absl::InternalError( "ExpressionStep::Evaluate called on moved-from step"));