Add instruction for list steps.

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",

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<google::protobuf::Arena> 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<google::protobuf::Arena> arena;

eval/compiler/flat_expr_builder.cc

@@ -507,6 +507,17 @@ absl::flat_hash_set<int32_t> MakeOptionalIndicesSet(
   return optional_indices;
 }
 
+absl::flat_hash_set<size_t> MakeOptionalIndicesSet(
+    const cel::ListExpr& list_expr) {
+  absl::flat_hash_set<size_t> 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<size_t> 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.

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",

eval/eval/create_list_step.cc

@@ -4,20 +4,17 @@
 #include <cstdint>
 #include <memory>
 #include <utility>
-#include <vector>
 
 #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,112 +26,132 @@ using ::cel::UnknownValue;
 using ::cel::Value;
 using ::cel::common_internal::NewListValueBuilder;
 
-class CreateListStep : public ExpressionStepBase {
- public:
-  CreateListStep(int list_size, absl::flat_hash_set<int> optional_indices)
-      : list_size_(list_size), optional_indices_(std::move(optional_indices)) {}
+constexpr size_t kMaxSmallListSize = 58;
 
-  void Evaluate(ExecutionFrame* frame) const override;
+template <typename T>
+struct StepTraits;
 
- private:
-  absl::Status DoEvaluate(ExecutionFrame* frame, Value* result) const;
+template <>
+struct StepTraits<SmallListStepInfo> {
+  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<int32_t> 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<ListStepInfo> {
+  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 <typename StepInfo>
+void EvaluateListStepImpl(const StepInfo& step, ExecutionFrame& frame) {
+  const size_t num_elements = StepTraits<StepInfo>::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<const Value> 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<UnknownValue> 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<int32_t>(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<StepInfo>::is_optional(step, i)) {
+      if (cel::optional_ref<const cel::OptionalValue> optional_arg =
+              arg.AsOptional();
+          optional_arg.has_value()) {
         if (!optional_arg->HasValue()) {
           continue;
         }
         Value optional_arg_value;
         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<int32_t> MakeOptionalIndicesSet(
-    const cel::ListExpr& create_list_expr) {
-  absl::flat_hash_set<int32_t> 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<int32_t>(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<std::unique_ptr<ExpressionStepLogic>> CreateCreateListStep(
-    const cel::ListExpr& create_list_expr) {
-  return std::make_unique<CreateListStep>(
-      create_list_expr.elements().size(),
-      MakeOptionalIndicesSet(create_list_expr));
+ExpressionStep CreateCreateListStep(
+    size_t size, absl::flat_hash_set<size_t> 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<ListStepInfo>(size, std::move(optional_indices)),
+      expr_id);
 }
 
 }  // namespace google::api::expr::runtime