Skip to content

Commit d259cf3

Browse files
jnthntatumcopybara-github
authored andcommitted
Add instruction for list steps.
PiperOrigin-RevId: 996169656
1 parent cffec7c commit d259cf3

8 files changed

Lines changed: 189 additions & 121 deletions

File tree

‎eval/compiler/BUILD‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,6 @@ cc_library(
102102
"//common:type_spec_resolver",
103103
"//common:value",
104104
"//eval/eval:container_access_step",
105-
"//eval/eval:create_list_step",
106105
"//eval/eval:create_map_step",
107106
"//eval/eval:create_struct_step",
108107
"//eval/eval:evaluator_core",
@@ -331,7 +330,6 @@ cc_test(
331330
"//base:ast",
332331
"//common:expr",
333332
"//common:value",
334-
"//eval/eval:create_list_step",
335333
"//eval/eval:create_map_step",
336334
"//eval/eval:evaluator_core",
337335
"//extensions/protobuf:ast_converters",

‎eval/compiler/constant_folding_test.cc‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -298,10 +298,8 @@ TEST_F(UpdatedConstantFoldingTest, CreatesList) {
298298
program_builder.ExitSubexpression(&elem_two);
299299

300300
// createlist
301-
ASSERT_OK_AND_ASSIGN(auto step,
302-
CreateCreateListStep(create_list.list_expr()));
303-
program_builder.AddStep(
304-
ExpressionStep::MakeGenericStep(std::move(step), create_list.id()));
301+
program_builder.AddStep(CreateCreateListStep(
302+
create_list.list_expr().elements().size(), {}, create_list.id()));
305303
program_builder.ExitSubexpression(&create_list);
306304

307305
std::shared_ptr<google::protobuf::Arena> arena;
@@ -376,10 +374,8 @@ TEST_F(UpdatedConstantFoldingTest, CreatesLargeList) {
376374
program_builder.ExitSubexpression(&elem4);
377375

378376
// createlist
379-
ASSERT_OK_AND_ASSIGN(auto step_large,
380-
CreateCreateListStep(create_list.list_expr()));
381-
program_builder.AddStep(
382-
ExpressionStep::MakeGenericStep(std::move(step_large), create_list.id()));
377+
program_builder.AddStep(CreateCreateListStep(
378+
create_list.list_expr().elements().size(), {}, create_list.id()));
383379
program_builder.ExitSubexpression(&create_list);
384380

385381
std::shared_ptr<google::protobuf::Arena> arena;

‎eval/compiler/flat_expr_builder.cc‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -472,6 +472,17 @@ absl::flat_hash_set<int32_t> MakeOptionalIndicesSet(
472472
return optional_indices;
473473
}
474474

475+
absl::flat_hash_set<size_t> MakeOptionalIndicesSet(
476+
const cel::ListExpr& list_expr) {
477+
absl::flat_hash_set<size_t> optional_indices;
478+
for (size_t i = 0; i < list_expr.elements().size(); ++i) {
479+
if (list_expr.elements()[i].optional()) {
480+
optional_indices.insert(i);
481+
}
482+
}
483+
return optional_indices;
484+
}
485+
475486
class FlatExprVisitor : public cel::AstVisitor {
476487
public:
477488
enum class CallHandlerResult {
@@ -1296,7 +1307,17 @@ class FlatExprVisitor : public cel::AstVisitor {
12961307
}
12971308
}
12981309
}
1299-
AddStep(CreateCreateListStep(list_expr), expr.id());
1310+
absl::flat_hash_set<size_t> optional_indices =
1311+
MakeOptionalIndicesSet(list_expr);
1312+
for (size_t index : optional_indices) {
1313+
if (!ValidateOrError(index < list_expr.elements().size(),
1314+
"Optional index out of range: ", index,
1315+
", list size: ", list_expr.elements().size())) {
1316+
return;
1317+
}
1318+
}
1319+
AddStep(CreateCreateListStep(list_expr.elements().size(),
1320+
std::move(optional_indices), expr.id()));
13001321
}
13011322

13021323
// CreateStruct node handler.

‎eval/eval/BUILD‎

Lines changed: 4 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ cc_library(
3636
name = "evaluator_core",
3737
srcs = [
3838
"comprehension_step.cc",
39+
"create_list_step.cc",
3940
"equality_steps.cc",
4041
"evaluator_core.cc",
4142
"function_step.cc",
@@ -45,6 +46,7 @@ cc_library(
4546
],
4647
hdrs = [
4748
"comprehension_step.h",
49+
"create_list_step.h",
4850
"equality_steps.h",
4951
"evaluator_core.h",
5052
"function_step.h",
@@ -65,6 +67,7 @@ cc_library(
6567
"//common:expr",
6668
"//common:function_descriptor",
6769
"//common:kind",
70+
"//common:optional_ref",
6871
"//common:value",
6972
"//common:value_kind",
7073
"//eval/internal:errors",
@@ -83,6 +86,7 @@ cc_library(
8386
"@com_google_absl//absl/base",
8487
"@com_google_absl//absl/base:core_headers",
8588
"@com_google_absl//absl/base:nullability",
89+
"@com_google_absl//absl/container:flat_hash_set",
8690
"@com_google_absl//absl/log:absl_check",
8791
"@com_google_absl//absl/log:absl_log",
8892
"@com_google_absl//absl/status",
@@ -302,29 +306,6 @@ cc_library(
302306
],
303307
)
304308

305-
cc_library(
306-
name = "create_list_step",
307-
srcs = [
308-
"create_list_step.cc",
309-
],
310-
hdrs = [
311-
"create_list_step.h",
312-
],
313-
deps = [
314-
":attribute_utility",
315-
":evaluator_core",
316-
":expression_step_base",
317-
":expression_step_logic",
318-
"//common:expr",
319-
"//common:value",
320-
"//internal:status_macros",
321-
"@com_google_absl//absl/container:flat_hash_set",
322-
"@com_google_absl//absl/status",
323-
"@com_google_absl//absl/status:statusor",
324-
"@com_google_absl//absl/types:optional",
325-
],
326-
)
327-
328309
cc_library(
329310
name = "create_struct_step",
330311
srcs = [
@@ -658,7 +639,6 @@ cc_test(
658639
],
659640
deps = [
660641
":cel_expression_flat_impl",
661-
":create_list_step",
662642
":evaluator_core",
663643
"//base:attributes",
664644
"//base:data",

‎eval/eval/create_list_step.cc‎

Lines changed: 88 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -4,20 +4,17 @@
44
#include <cstdint>
55
#include <memory>
66
#include <utility>
7-
#include <vector>
87

98
#include "absl/container/flat_hash_set.h"
9+
#include "absl/log/absl_check.h"
1010
#include "absl/status/status.h"
11-
#include "absl/status/statusor.h"
1211
#include "absl/types/optional.h"
13-
#include "common/expr.h"
12+
#include "absl/types/span.h"
13+
#include "common/optional_ref.h"
1414
#include "common/value.h"
1515
#include "common/values/list_value_builder.h"
1616
#include "eval/eval/attribute_utility.h"
1717
#include "eval/eval/evaluator_core.h"
18-
#include "eval/eval/expression_step_base.h"
19-
#include "eval/eval/expression_step_logic.h"
20-
#include "internal/status_macros.h"
2118

2219
namespace google::api::expr::runtime {
2320

@@ -29,112 +26,132 @@ using ::cel::UnknownValue;
2926
using ::cel::Value;
3027
using ::cel::common_internal::NewListValueBuilder;
3128

32-
class CreateListStep : public ExpressionStepBase {
33-
public:
34-
CreateListStep(int list_size, absl::flat_hash_set<int> optional_indices)
35-
: list_size_(list_size), optional_indices_(std::move(optional_indices)) {}
29+
constexpr size_t kMaxSmallListSize = 58;
3630

37-
void Evaluate(ExecutionFrame* frame) const override;
31+
template <typename T>
32+
struct StepTraits;
3833

39-
private:
40-
absl::Status DoEvaluate(ExecutionFrame* frame, Value* result) const;
34+
template <>
35+
struct StepTraits<SmallListStepInfo> {
36+
static bool is_optional(const SmallListStepInfo& step, size_t index) {
37+
ABSL_DCHECK_LT(index, kMaxSmallListSize);
38+
return ((step.optional_bit_set >> index) & 1) == 1;
39+
}
4140

42-
int list_size_;
43-
absl::flat_hash_set<int32_t> optional_indices_;
41+
static size_t num_elements(const SmallListStepInfo& step) {
42+
return step.list_size;
43+
}
4444
};
4545

46-
void CreateListStep::Evaluate(ExecutionFrame* frame) const {
47-
if (list_size_ < 0) {
48-
frame->Abort(absl::InternalError("CreateListStep: list size is <0"));
49-
return;
46+
template <>
47+
struct StepTraits<ListStepInfo> {
48+
static bool is_optional(const ListStepInfo& step, size_t index) {
49+
return step.is_optional(index);
5050
}
5151

52-
if (!frame->value_stack().HasEnough(list_size_)) {
53-
frame->Abort(absl::InternalError("CreateListStep: stack underflow"));
54-
return;
52+
static size_t num_elements(const ListStepInfo& step) {
53+
return step.num_elements();
5554
}
55+
};
5656

57-
Value result;
58-
if (absl::Status status = DoEvaluate(frame, &result); !status.ok()) {
59-
frame->Abort(std::move(status));
57+
template <typename StepInfo>
58+
void EvaluateListStepImpl(const StepInfo& step, ExecutionFrame& frame) {
59+
const size_t num_elements = StepTraits<StepInfo>::num_elements(step);
60+
61+
if (!frame.value_stack().HasEnough(num_elements)) {
62+
frame.Abort(absl::InternalError("CreateListStep: stack underflow"));
6063
return;
6164
}
6265

63-
frame->value_stack().PopAndPush(list_size_, std::move(result));
64-
}
65-
66-
absl::Status CreateListStep::DoEvaluate(ExecutionFrame* frame,
67-
Value* result) const {
68-
auto args = frame->value_stack().GetSpan(list_size_);
66+
absl::Span<const Value> args = frame.value_stack().GetSpan(num_elements);
6967

70-
for (const auto& arg : args) {
71-
if (arg.IsError()) {
72-
*result = arg;
73-
return absl::OkStatus();
68+
for (size_t i = 0; i < num_elements; ++i) {
69+
if (args[i].IsError()) {
70+
frame.value_stack().SwapAndPop(num_elements, i);
71+
return;
7472
}
7573
}
7674

77-
if (frame->enable_unknowns()) {
75+
if (frame.enable_unknowns()) {
7876
absl::optional<UnknownValue> unknown_set =
79-
frame->attribute_utility().IdentifyAndMergeUnknowns(
80-
args, frame->value_stack().GetAttributeSpan(list_size_),
81-
/*use_partial=*/true, frame->arena());
77+
frame.attribute_utility().IdentifyAndMergeUnknowns(
78+
args, frame.value_stack().GetAttributeSpan(num_elements),
79+
/*use_partial=*/true, frame.arena());
8280
if (unknown_set.has_value()) {
83-
*result = std::move(*unknown_set);
84-
return absl::OkStatus();
81+
frame.value_stack().PopAndPush(num_elements, std::move(*unknown_set));
82+
return;
8583
}
8684
}
8785

88-
ListValueBuilderPtr builder = NewListValueBuilder(frame->arena());
89-
builder->Reserve(args.size());
86+
ListValueBuilderPtr builder = NewListValueBuilder(frame.arena());
87+
builder->Reserve(num_elements);
9088

91-
for (size_t i = 0; i < args.size(); ++i) {
92-
const auto& arg = args[i];
93-
if (optional_indices_.contains(static_cast<int32_t>(i))) {
94-
if (auto optional_arg = arg.AsOptional(); optional_arg) {
89+
for (size_t i = 0; i < num_elements; ++i) {
90+
const Value& arg = args[i];
91+
if (StepTraits<StepInfo>::is_optional(step, i)) {
92+
if (cel::optional_ref<const cel::OptionalValue> optional_arg =
93+
arg.AsOptional();
94+
optional_arg.has_value()) {
9595
if (!optional_arg->HasValue()) {
9696
continue;
9797
}
9898
Value optional_arg_value;
9999
optional_arg->Value(&optional_arg_value);
100100
if (optional_arg_value.IsError()) {
101101
// Error should never be in optional, but better safe than sorry.
102-
*result = std::move(optional_arg_value);
103-
return absl::OkStatus();
102+
frame.value_stack().PopAndPush(num_elements,
103+
std::move(optional_arg_value));
104+
return;
105+
}
106+
if (absl::Status status = builder->Add(std::move(optional_arg_value));
107+
!status.ok()) {
108+
frame.Abort(std::move(status));
109+
return;
104110
}
105-
CEL_RETURN_IF_ERROR(builder->Add(std::move(optional_arg_value)));
106111
} else {
107-
*result = cel::TypeConversionError(arg.GetTypeName(), "optional_type",
108-
frame->arena());
109-
return absl::OkStatus();
112+
frame.value_stack().PopAndPush(
113+
num_elements,
114+
cel::TypeConversionError(arg.GetTypeName(), "optional_type",
115+
frame.arena()));
116+
return;
110117
}
111118
} else {
112-
CEL_RETURN_IF_ERROR(builder->Add(arg));
119+
if (absl::Status status = builder->Add(arg); !status.ok()) {
120+
frame.Abort(std::move(status));
121+
return;
122+
}
113123
}
114124
}
115125

116-
*result = std::move(*builder).Build();
117-
return absl::OkStatus();
126+
frame.value_stack().PopAndPush(num_elements, std::move(*builder).Build());
118127
}
119128

120-
absl::flat_hash_set<int32_t> MakeOptionalIndicesSet(
121-
const cel::ListExpr& create_list_expr) {
122-
absl::flat_hash_set<int32_t> optional_indices;
123-
for (size_t i = 0; i < create_list_expr.elements().size(); ++i) {
124-
if (create_list_expr.elements()[i].optional()) {
125-
optional_indices.insert(static_cast<int32_t>(i));
126-
}
127-
}
128-
return optional_indices;
129+
} // namespace
130+
131+
void EvaluateListStep(const ListStepInfo& step, ExecutionFrame& frame) {
132+
EvaluateListStepImpl(step, frame);
129133
}
130134

131-
} // namespace
135+
void EvaluateSmallListStep(const SmallListStepInfo& step,
136+
ExecutionFrame& frame) {
137+
EvaluateListStepImpl(step, frame);
138+
}
132139

133-
absl::StatusOr<std::unique_ptr<ExpressionStepLogic>> CreateCreateListStep(
134-
const cel::ListExpr& create_list_expr) {
135-
return std::make_unique<CreateListStep>(
136-
create_list_expr.elements().size(),
137-
MakeOptionalIndicesSet(create_list_expr));
140+
ExpressionStep CreateCreateListStep(
141+
size_t size, absl::flat_hash_set<size_t> optional_indices,
142+
int64_t expr_id) {
143+
if (size <= kMaxSmallListSize) {
144+
uint64_t optional_bit_set = 0;
145+
for (size_t index : optional_indices) {
146+
ABSL_DCHECK_LT(index, size);
147+
optional_bit_set |= (uint64_t{1} << index);
148+
}
149+
return ExpressionStep::MakeCreateSmallListStep(
150+
SmallListStepInfo{optional_bit_set, size}, expr_id);
151+
}
152+
return ExpressionStep::MakeCreateListStep(
153+
std::make_unique<ListStepInfo>(size, std::move(optional_indices)),
154+
expr_id);
138155
}
139156

140157
} // namespace google::api::expr::runtime

0 commit comments

Comments
 (0)