Skip to content

Rollforward: Opt in to new field selection implementation. #2240

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
copybara-service wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Rollforward: Opt in to new field selection implementation. #2240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions common/legacy_value.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,14 +121,6 @@ inline MapValue CreateLegacyMapValue(
return common_internal::LegacyMapValue(value);
}

inline Value CreateDurationValue(absl::Duration value, bool unchecked = false) {
return DurationValue{value};
}

inline TimestampValue CreateTimestampValue(absl::Time value) {
return TimestampValue{value};
}

Value LegacyValueToModernValueOrDie(
google::protobuf::Arena* arena, const google::api::expr::runtime::CelValue& value,
bool unchecked = false);
Expand Down
7 changes: 5 additions & 2 deletions common/value.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1146,10 +1146,13 @@ Value VistWellKnownTypeValue(float value) { return DoubleValue(value); }
Value VistWellKnownTypeValue(double value) { return DoubleValue(value); }

Value VistWellKnownTypeValue(absl::Duration value) {
return DurationValue(value);
// Tolerate out-of-range values.
return UnsafeDurationValue(value);
}

Value VistWellKnownTypeValue(absl::Time value) { return TimestampValue(value); }
Value VistWellKnownTypeValue(absl::Time value) {
return UnsafeTimestampValue(value);
}

struct OwningWellKnownTypesValueVisitor {
google::protobuf::Arena* absl_nullable arena;
Expand Down
6 changes: 6 additions & 0 deletions common/values/duration_value.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,12 @@ class DurationValue final : private common_internal::ValueMixin<DurationValue> {
public:
static constexpr ValueKind kKind = ValueKind::kDuration;

// Constructs a `DurationValue` from an `absl::Duration`.
//
// DCHECK-fails if the value is not in the supported range.
//
// Prefer using `SafeDurationValue` or `UnsafeDurationValue` if the caller
// has already validated the value.
explicit DurationValue(absl::Duration value) noexcept
: DurationValue(absl::in_place, value) {
ABSL_DCHECK_OK(internal::ValidateDuration(value));
Expand Down
22 changes: 11 additions & 11 deletions common/values/struct_value_builder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,8 +254,8 @@ absl::StatusOr<absl::optional<ErrorValue>> ProtoMessageFromValueImpl(
if (auto duration_value = value.AsDuration(); duration_value) {
CEL_RETURN_IF_ERROR(
well_known_types->Duration().Initialize(message->GetDescriptor()));
CEL_RETURN_IF_ERROR(well_known_types->Duration().SetFromAbslDuration(
message, duration_value->NativeValue()));
well_known_types->Duration().UnsafeSetFromAbslDuration(
message, duration_value->NativeValue());
return std::nullopt;
}
return TypeConversionError(value.GetTypeName(), to_desc->full_name());
Expand All @@ -264,8 +264,8 @@ absl::StatusOr<absl::optional<ErrorValue>> ProtoMessageFromValueImpl(
if (auto timestamp_value = value.AsTimestamp(); timestamp_value) {
CEL_RETURN_IF_ERROR(
well_known_types->Timestamp().Initialize(message->GetDescriptor()));
CEL_RETURN_IF_ERROR(well_known_types->Timestamp().SetFromAbslTime(
message, timestamp_value->NativeValue()));
well_known_types->Timestamp().UnsafeSetFromAbslTime(
message, timestamp_value->NativeValue());
return std::nullopt;
}
return TypeConversionError(value.GetTypeName(), to_desc->full_name());
Expand Down Expand Up @@ -1304,11 +1304,11 @@ class MessageValueBuilderImpl {
if (auto duration_value = value.AsDuration(); duration_value) {
CEL_RETURN_IF_ERROR(well_known_types_.Duration().Initialize(
field->message_type()));
CEL_RETURN_IF_ERROR(
well_known_types_.Duration().SetFromAbslDuration(
reflection_->MutableMessage(message_, field,
message_factory_),
duration_value->NativeValue()));

well_known_types_.Duration().UnsafeSetFromAbslDuration(
reflection_->MutableMessage(message_, field,
message_factory_),
duration_value->NativeValue());
return std::nullopt;
}
return TypeConversionError(value.GetTypeName(),
Expand All @@ -1322,10 +1322,10 @@ class MessageValueBuilderImpl {
if (auto timestamp_value = value.AsTimestamp(); timestamp_value) {
CEL_RETURN_IF_ERROR(well_known_types_.Timestamp().Initialize(
field->message_type()));
CEL_RETURN_IF_ERROR(well_known_types_.Timestamp().SetFromAbslTime(
well_known_types_.Timestamp().UnsafeSetFromAbslTime(
reflection_->MutableMessage(message_, field,
message_factory_),
timestamp_value->NativeValue()));
timestamp_value->NativeValue());
return std::nullopt;
}
return TypeConversionError(value.GetTypeName(),
Expand Down
6 changes: 6 additions & 0 deletions common/values/timestamp_value.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,12 @@ class TimestampValue final
public:
static constexpr ValueKind kKind = ValueKind::kTimestamp;

// Constructs a `TimestampValue` from an `absl::Time`.
//
// DCHECK-fails if the value is not in the supported range.
//
// Prefer using `SafeTimestampValue` or `UnsafeTimestampValue` if the caller
// has already validated the value.
explicit TimestampValue(absl::Time value) noexcept
: TimestampValue(absl::in_place, value) {
ABSL_DCHECK_OK(internal::ValidateTimestamp(value));
Expand Down
2 changes: 1 addition & 1 deletion eval/public/cel_options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ struct InterpreterOptions {
// For google::api::expr::runtime::CelExpression, this will enable updated
// implementations for field access on protobuf messages, aligned with the
// cel::Value implementation.
bool enable_use_new_field_select_implementation = false;
bool enable_use_new_field_select_implementation = true;
};
// LINT.ThenChange(//depot/google3/runtime/runtime_options.h)

Expand Down
4 changes: 2 additions & 2 deletions internal/message_equality.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,11 +380,11 @@ absl::StatusOr<EquatableValue> AsEquatableValue(
case Descriptor::WELLKNOWNTYPE_DURATION:
CEL_RETURN_IF_ERROR(
reflection.duration_reflection.Initialize(descriptor));
return reflection.duration_reflection.ToAbslDuration(message);
return reflection.duration_reflection.UnsafeToAbslDuration(message);
case Descriptor::WELLKNOWNTYPE_TIMESTAMP:
CEL_RETURN_IF_ERROR(
reflection.timestamp_reflection.Initialize(descriptor));
return reflection.timestamp_reflection.ToAbslTime(message);
return reflection.timestamp_reflection.UnsafeToAbslTime(message);
case Descriptor::WELLKNOWNTYPE_ANY:
return EquatableAny(message);
default:
Expand Down
6 changes: 4 additions & 2 deletions internal/well_known_types.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2114,13 +2114,15 @@ absl::StatusOr<well_known_types::Value> AdaptFromMessage(
case Descriptor::WELLKNOWNTYPE_ANY:
// This is unreachable, as AdaptAny() above recursively unpacks.
ABSL_UNREACHABLE();
// Don't check that time values are in range on field access. Assume
// error will propagate if they are used in any arithmetic.
case Descriptor::WELLKNOWNTYPE_DURATION: {
CEL_ASSIGN_OR_RETURN(auto reflection, GetDurationReflection(descriptor));
return reflection.ToAbslDuration(*to_adapt);
return reflection.UnsafeToAbslDuration(*to_adapt);
}
case Descriptor::WELLKNOWNTYPE_TIMESTAMP: {
CEL_ASSIGN_OR_RETURN(auto reflection, GetTimestampReflection(descriptor));
return reflection.ToAbslTime(*to_adapt);
return reflection.UnsafeToAbslTime(*to_adapt);
}
case Descriptor::WELLKNOWNTYPE_VALUE: {
CEL_ASSIGN_OR_RETURN(auto reflection, GetValueReflection(descriptor));
Expand Down
93 changes: 83 additions & 10 deletions internal/well_known_types_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,44 @@ TEST_F(ReflectionTest, Duration_Dynamic) {
StatusIs(absl::StatusCode::kInvalidArgument));
}

TEST_F(ReflectionTest, Duration_ToAbslDuration) {
auto* value = MakeDynamic<google::protobuf::Duration>();
ASSERT_OK_AND_ASSIGN(
auto reflection,
GetDurationReflection(ABSL_DIE_IF_NULL(value->GetDescriptor())));

reflection.SetSeconds(value, 1);
reflection.SetNanos(value, 1);
EXPECT_THAT(reflection.ToAbslDuration(*value),
IsOkAndHolds(absl::Seconds(1) + absl::Nanoseconds(1)));
EXPECT_EQ(reflection.UnsafeToAbslDuration(*value),
absl::Seconds(1) + absl::Nanoseconds(1));

reflection.SetSeconds(value, 0x7fffffffffffffff);
reflection.SetNanos(value, 1);
EXPECT_THAT(reflection.ToAbslDuration(*value),
StatusIs(absl::StatusCode::kInvalidArgument,
HasSubstr("invalid duration seconds: ")));
EXPECT_EQ(reflection.UnsafeToAbslDuration(*value),
absl::Seconds(0x7fffffffffffffff) + absl::Nanoseconds(1));

reflection.SetSeconds(value, 1);
reflection.SetNanos(value, 0x7fffffff);
EXPECT_THAT(reflection.ToAbslDuration(*value),
StatusIs(absl::StatusCode::kInvalidArgument,
HasSubstr("invalid duration nanoseconds: ")));
EXPECT_EQ(reflection.UnsafeToAbslDuration(*value),
absl::Seconds(1) + absl::Nanoseconds(0x7fffffff));

reflection.SetSeconds(value, -1);
reflection.SetNanos(value, 1);
EXPECT_THAT(reflection.ToAbslDuration(*value),
StatusIs(absl::StatusCode::kInvalidArgument,
HasSubstr("duration sign mismatch: ")));
EXPECT_EQ(reflection.UnsafeToAbslDuration(*value),
absl::Seconds(-1) + absl::Nanoseconds(1));
}

TEST_F(ReflectionTest, Timestamp_Generated) {
auto* value = MakeGenerated<google::protobuf::Timestamp>();
EXPECT_EQ(TimestampReflection::GetSeconds(*value), 0);
Expand Down Expand Up @@ -389,6 +427,39 @@ TEST_F(ReflectionTest, Timestamp_Dynamic) {
StatusIs(absl::StatusCode::kInvalidArgument));
}

TEST_F(ReflectionTest, Timestamp_ToAbslTime) {
auto* value = MakeDynamic<google::protobuf::Timestamp>();
ASSERT_OK_AND_ASSIGN(
auto reflection,
GetTimestampReflection(ABSL_DIE_IF_NULL(value->GetDescriptor())));

reflection.SetSeconds(value, 1);
reflection.SetNanos(value, 1);
EXPECT_THAT(reflection.ToAbslTime(*value),
IsOkAndHolds(absl::UnixEpoch() + absl::Seconds(1) +
absl::Nanoseconds(1)));
EXPECT_EQ(reflection.UnsafeToAbslTime(*value),
absl::UnixEpoch() + absl::Seconds(1) + absl::Nanoseconds(1));

reflection.SetSeconds(value, 0x7fffffffffffffff);
reflection.SetNanos(value, 1);
EXPECT_THAT(reflection.ToAbslTime(*value),
StatusIs(absl::StatusCode::kInvalidArgument,
HasSubstr("invalid timestamp seconds: ")));
EXPECT_EQ(reflection.UnsafeToAbslTime(*value),
absl::UnixEpoch() + absl::Seconds(0x7fffffffffffffff) +
absl::Nanoseconds(1));

reflection.SetSeconds(value, 1);
reflection.SetNanos(value, 0x7fffffff);
EXPECT_THAT(reflection.ToAbslTime(*value),
StatusIs(absl::StatusCode::kInvalidArgument,
HasSubstr("invalid timestamp nanoseconds: ")));
EXPECT_EQ(
reflection.UnsafeToAbslTime(*value),
absl::UnixEpoch() + absl::Seconds(1) + absl::Nanoseconds(0x7fffffff));
}

TEST_F(ReflectionTest, Value_Generated) {
auto* value = MakeGenerated<google::protobuf::Value>();
EXPECT_EQ(ValueReflection::GetKindCase(*value),
Expand Down Expand Up @@ -698,25 +769,25 @@ TEST_F(AdaptFromMessageTest, Duration_SecondsOutOfRange) {
auto message = DynamicParseTextProto<google::protobuf::Duration>(
R"pb(seconds: 0x7fffffffffffffff nanos: 1)pb");
EXPECT_THAT(AdaptFromMessage(*message),
StatusIs(absl::StatusCode::kInvalidArgument,
HasSubstr("invalid duration seconds: ")));
IsOkAndHolds(VariantWith<absl::Duration>(
absl::Seconds(0x7fffffffffffffff) + absl::Nanoseconds(1))));
}

TEST_F(AdaptFromMessageTest, Duration_NanosOutOfRange) {
auto message = DynamicParseTextProto<google::protobuf::Duration>(
R"pb(seconds: 1 nanos: 0x7fffffff)pb");
EXPECT_THAT(AdaptFromMessage(*message),
StatusIs(absl::StatusCode::kInvalidArgument,
HasSubstr("invalid duration nanoseconds: ")));
IsOkAndHolds(VariantWith<absl::Duration>(
absl::Seconds(1) + absl::Nanoseconds(0x7fffffff))));
}

TEST_F(AdaptFromMessageTest, Duration_SignMismatch) {
auto message =
DynamicParseTextProto<google::protobuf::Duration>(R"pb(seconds: -1
nanos: 1)pb");
EXPECT_THAT(AdaptFromMessage(*message),
StatusIs(absl::StatusCode::kInvalidArgument,
HasSubstr("duration sign mismatch: ")));
IsOkAndHolds(VariantWith<absl::Duration>(absl::Seconds(-1) +
absl::Nanoseconds(1))));
}

TEST_F(AdaptFromMessageTest, Timestamp) {
Expand All @@ -733,16 +804,18 @@ TEST_F(AdaptFromMessageTest, Timestamp_SecondsOutOfRange) {
auto message = DynamicParseTextProto<google::protobuf::Timestamp>(
R"pb(seconds: 0x7fffffffffffffff nanos: 1)pb");
EXPECT_THAT(AdaptFromMessage(*message),
StatusIs(absl::StatusCode::kInvalidArgument,
HasSubstr("invalid timestamp seconds: ")));
IsOkAndHolds(VariantWith<absl::Time>(
absl::UnixEpoch() + absl::Seconds(0x7fffffffffffffff) +
absl::Nanoseconds(1))));
}

TEST_F(AdaptFromMessageTest, Timestamp_NanosOutOfRange) {
auto message = DynamicParseTextProto<google::protobuf::Timestamp>(
R"pb(seconds: 1 nanos: 0x7fffffff)pb");
EXPECT_THAT(AdaptFromMessage(*message),
StatusIs(absl::StatusCode::kInvalidArgument,
HasSubstr("invalid timestamp nanoseconds: ")));
IsOkAndHolds(
VariantWith<absl::Time>(absl::UnixEpoch() + absl::Seconds(1) +
absl::Nanoseconds(0x7fffffff))));
}

TEST_F(AdaptFromMessageTest, Value_NullValue) {
Expand Down
4 changes: 2 additions & 2 deletions runtime/internal/function_adapter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,13 +200,13 @@ struct AdaptedToValueVisitor {
absl::StatusOr<Value> operator()(absl::Time in) {
// Type matching may have already occurred. It's too late to change up the
// type and return an error.
return TimestampValue(in);
return UnsafeTimestampValue(in);
}

absl::StatusOr<Value> operator()(absl::Duration in) {
// Type matching may have already occurred. It's too late to change up the
// type and return an error.
return DurationValue(in);
return UnsafeDurationValue(in);
}

absl::StatusOr<Value> operator()(Value in) { return in; }
Expand Down
2 changes: 1 addition & 1 deletion runtime/runtime_options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ struct RuntimeOptions {
// For google::api::expr::runtime::CelExpression, this will enable updated
// implementations for field access on protobuf messages, aligned with the
// cel::Value implementation.
bool enable_use_new_field_select_implementation = false;
bool enable_use_new_field_select_implementation = true;
};
// LINT.ThenChange(//depot/google3/eval/public/cel_options.h)

Expand Down
6 changes: 4 additions & 2 deletions runtime/standard/type_conversion_functions.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,14 +403,16 @@ absl::Status RegisterTimeConversionFunctions(FunctionRegistry& registry,
CEL_RETURN_IF_ERROR(
(UnaryFunctionAdapter<Value, absl::Time>::RegisterGlobalOverload(
cel::builtin::kTimestamp,
[](absl::Time value) -> Value { return TimestampValue(value); },
[](absl::Time value) -> Value { return UnsafeTimestampValue(value); },
registry)));

// duration -> duration
CEL_RETURN_IF_ERROR(
(UnaryFunctionAdapter<Value, absl::Duration>::RegisterGlobalOverload(
cel::builtin::kDuration,
[](absl::Duration value) -> Value { return DurationValue(value); },
[](absl::Duration value) -> Value {
return UnsafeDurationValue(value);
},
registry)));

// timestamp() conversion from string.
Expand Down
Loading