Skip to content

Commit 0b4d452

Browse files
committed
fix(schema): address review nits on default-value validation/serde
- ValidateDefault: use internal::checked_pointer_cast and ICEBERG_ASSIGN_OR_RAISE so an uncastable default surfaces the original CastTo error instead of a generic message; keep the out-of-range (sentinel) case as an invalid-schema error. - json_serde: error out directly when a timestamptz default is not a string, drop the offset-specific wording from the UTC error message (it also accepts -00:00), and use auto for the parsed default JSON. - Update schema tests for the propagated cast error and the out-of-range case.
1 parent 925611a commit 0b4d452

3 files changed

Lines changed: 20 additions & 17 deletions

File tree

src/iceberg/json_serde.cc

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -578,15 +578,14 @@ Status ValidateTimestamptzDefaultIsUtc(const Type& type, const nlohmann::json& v
578578
return {};
579579
}
580580
if (!value.is_string()) {
581-
// Let LiteralFromJson report the type mismatch.
582-
return {};
581+
return JsonParseError("Invalid timestamptz default {} for {}: expected a string",
582+
SafeDumpJson(value), type.ToString());
583583
}
584584
const auto str = value.get<std::string>();
585585
ICEBERG_ASSIGN_OR_RAISE(bool is_utc, TemporalUtils::IsUtcOffset(str));
586586
if (!is_utc) {
587587
return JsonParseError(
588-
"Invalid timestamptz default '{}' for {}: default values must use UTC "
589-
"(offset 'Z' or '+00:00')",
588+
"Invalid timestamptz default '{}' for {}: default values must use a UTC offset",
590589
str, type.ToString());
591590
}
592591
return {};
@@ -601,9 +600,9 @@ Result<std::unique_ptr<SchemaField>> FieldFromJson(const nlohmann::json& json) {
601600
ICEBERG_ASSIGN_OR_RAISE(auto name, GetJsonValue<std::string>(json, kName));
602601
ICEBERG_ASSIGN_OR_RAISE(auto required, GetJsonValue<bool>(json, kRequired));
603602
ICEBERG_ASSIGN_OR_RAISE(auto doc, GetJsonValueOrDefault<std::string>(json, kDoc));
604-
ICEBERG_ASSIGN_OR_RAISE(std::optional<nlohmann::json> initial_default_json,
603+
ICEBERG_ASSIGN_OR_RAISE(auto initial_default_json,
605604
GetJsonValueOptional<nlohmann::json>(json, kInitialDefault));
606-
ICEBERG_ASSIGN_OR_RAISE(std::optional<nlohmann::json> write_default_json,
605+
ICEBERG_ASSIGN_OR_RAISE(auto write_default_json,
607606
GetJsonValueOptional<nlohmann::json>(json, kWriteDefault));
608607

609608
std::shared_ptr<const Literal> initial_default;

src/iceberg/schema_field.cc

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525

2626
#include "iceberg/expression/literal.h"
2727
#include "iceberg/type.h"
28+
#include "iceberg/util/checked_cast.h"
2829
#include "iceberg/util/formatter.h" // IWYU pragma: keep
2930
#include "iceberg/util/macros.h"
3031

@@ -107,11 +108,11 @@ Status ValidateDefault(const SchemaField& field, const Literal& value,
107108
// a string default on a date/timestamp/uuid field). Reject only defaults that cannot
108109
// be cast to the field type or fall outside its range (CastTo signals out-of-range as
109110
// an above-max/below-min sentinel).
110-
auto field_type = std::static_pointer_cast<PrimitiveType>(field.type());
111-
auto cast = value.CastTo(field_type);
112-
if (!cast.has_value() || cast->IsAboveMax() || cast->IsBelowMin()) {
113-
return InvalidSchema("{} of field {} has type {} that cannot be cast to {}", kind,
114-
field.name(), *value.type(), *field.type());
111+
auto field_type = internal::checked_pointer_cast<PrimitiveType>(field.type());
112+
ICEBERG_ASSIGN_OR_RAISE(auto cast, value.CastTo(field_type));
113+
if (cast.IsAboveMax() || cast.IsBelowMin()) {
114+
return InvalidSchema("{} of field {} ({}) is out of range for {}", kind, field.name(),
115+
*value.type(), *field.type());
115116
}
116117
return {};
117118
}

src/iceberg/test/schema_test.cc

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -162,9 +162,11 @@ TEST(SchemaTest, ValidateRejectsMismatchedDefaultValue) {
162162
1, "id", iceberg::int32(), false, /*doc=*/{}, /*initial_default=*/nullptr,
163163
std::make_shared<const iceberg::Literal>(iceberg::Literal::String("oops")))});
164164

165+
// The default literal is cast to the field type; an uncastable type surfaces the
166+
// original cast error.
165167
auto status = schema.Validate(iceberg::TableMetadata::kSupportedTableFormatVersion);
166-
ASSERT_THAT(status, iceberg::IsError(iceberg::ErrorKind::kInvalidSchema));
167-
EXPECT_THAT(status, iceberg::HasErrorMessage("write-default"));
168+
ASSERT_THAT(status, iceberg::IsError(iceberg::ErrorKind::kNotSupported));
169+
EXPECT_THAT(status, iceberg::HasErrorMessage("Cast from String"));
168170
}
169171

170172
TEST(SchemaTest, ValidateCastsDefaultToFieldType) {
@@ -176,11 +178,12 @@ TEST(SchemaTest, ValidateCastsDefaultToFieldType) {
176178
EXPECT_THAT(widened.Validate(iceberg::TableMetadata::kSupportedTableFormatVersion),
177179
iceberg::IsOk());
178180

179-
// A default whose type cannot be cast to the field type is still rejected.
180-
iceberg::Schema uncastable({iceberg::SchemaField(
181+
// A default outside the field type's range is rejected (CastTo returns an
182+
// above-max/below-min sentinel).
183+
iceberg::Schema out_of_range({iceberg::SchemaField(
181184
1, "id", iceberg::int32(), false, /*doc=*/{},
182-
std::make_shared<const iceberg::Literal>(iceberg::Literal::String("oops")))});
183-
EXPECT_THAT(uncastable.Validate(iceberg::TableMetadata::kSupportedTableFormatVersion),
185+
std::make_shared<const iceberg::Literal>(iceberg::Literal::Long(5'000'000'000)))});
186+
EXPECT_THAT(out_of_range.Validate(iceberg::TableMetadata::kSupportedTableFormatVersion),
184187
iceberg::IsError(iceberg::ErrorKind::kInvalidSchema));
185188
}
186189

0 commit comments

Comments
 (0)