refactor(schema): normalize default values via a fallible SchemaField::Make factory

huan233usc · huan233usc · commit c58050f96e0e · 2026-06-27T15:54:05.000-07:00
Move default-value normalization out of the constructor (which cannot report errors)
into a fallible factory, per review feedback. SchemaField::Make casts a cross-type
default to the field type and propagates the cast error / rejects out-of-range values
instead of swallowing them; it is a no-op when the default already matches the field
type. The constructor stores defaults verbatim, and ValidateDefault requires a stored
default to match the field type exactly (Make is the cast-accepting path). This keeps
the cross-type-accept behavior while ensuring stored defaults are always field-typed,
so projection and JSON round-trip equality are correct.
diff --git a/src/iceberg/schema_field.cc b/src/iceberg/schema_field.cc
@@ -33,25 +33,27 @@ namespace iceberg {
 
 namespace {
 
-// Normalizes a default value to the field type. A cross-type default (e.g. an int
-// literal on a long field) is accepted, so it is cast to the field type up front;
-// otherwise projection, JSON round-trip and equality would observe a literal whose type
-// differs from the field. A value that already matches the field type is returned as-is
-// (no needless copy), and a value that cannot be cast is left unchanged so Validate()
-// can report it.
-std::shared_ptr<const Literal> NormalizeDefault(std::shared_ptr<const Literal> value,
-                                                const std::shared_ptr<Type>& field_type) {
+// Normalizes a default value to the field type so the stored value always matches the
+// field. A cross-type default (e.g. an int literal on a long field) is cast to the field
+// type; a value that already matches the field type, or a null, is returned unchanged.
+// The cast error is propagated and an out-of-range value is rejected rather than
+// swallowed.
+Result<std::shared_ptr<const Literal>> NormalizeDefault(
+    std::shared_ptr<const Literal> value, const std::shared_ptr<Type>& field_type) {
   if (value == nullptr || field_type == nullptr || !field_type->is_primitive()) {
     return value;
   }
   if (*value->type() == *field_type) {
     return value;
   }
-  auto cast = value->CastTo(internal::checked_pointer_cast<PrimitiveType>(field_type));
-  if (!cast.has_value() || cast->IsAboveMax() || cast->IsBelowMin()) {
-    return value;
+  ICEBERG_ASSIGN_OR_RAISE(
+      auto cast,
+      value->CastTo(internal::checked_pointer_cast<PrimitiveType>(field_type)));
+  if (cast.IsAboveMax() || cast.IsBelowMin()) {
+    return InvalidSchema("default value of type {} is out of range for {}",
+                         *value->type(), *field_type);
   }
-  return std::make_shared<const Literal>(std::move(cast).value());
+  return std::make_shared<const Literal>(std::move(cast));
 }
 
 }  // namespace
@@ -65,8 +67,21 @@ SchemaField::SchemaField(int32_t field_id, std::string_view name,
       type_(std::move(type)),
       optional_(optional),
       doc_(doc),
-      initial_default_(NormalizeDefault(std::move(initial_default), type_)),
-      write_default_(NormalizeDefault(std::move(write_default), type_)) {}
+      initial_default_(std::move(initial_default)),
+      write_default_(std::move(write_default)) {}
+
+Result<SchemaField> SchemaField::Make(int32_t field_id, std::string_view name,
+                                      std::shared_ptr<Type> type, bool optional,
+                                      std::string_view doc,
+                                      std::shared_ptr<const Literal> initial_default,
+                                      std::shared_ptr<const Literal> write_default) {
+  ICEBERG_ASSIGN_OR_RAISE(initial_default,
+                          NormalizeDefault(std::move(initial_default), type));
+  ICEBERG_ASSIGN_OR_RAISE(write_default,
+                          NormalizeDefault(std::move(write_default), type));
+  return SchemaField(field_id, name, std::move(type), optional, doc,
+                     std::move(initial_default), std::move(write_default));
+}
 
 SchemaField SchemaField::MakeOptional(int32_t field_id, std::string_view name,
                                       std::shared_ptr<Type> type, std::string_view doc) {
@@ -128,15 +143,11 @@ Status ValidateDefault(const SchemaField& field, const Literal& value,
         "Invalid {} value for {}: default values are only supported for primitive types",
         kind, field.name());
   }
-  // Match Java (Types.NestedField), which casts the default literal to the field type
-  // instead of requiring an exact type match (e.g. an int default on a long field, or
-  // a string default on a date/timestamp/uuid field). Reject only defaults that cannot
-  // be cast to the field type or fall outside its range (CastTo signals out-of-range as
-  // an above-max/below-min sentinel).
-  auto field_type = internal::checked_pointer_cast<PrimitiveType>(field.type());
-  ICEBERG_ASSIGN_OR_RAISE(auto cast, value.CastTo(field_type));
-  if (cast.IsAboveMax() || cast.IsBelowMin()) {
-    return InvalidSchema("{} of field {} ({}) is out of range for {}", kind, field.name(),
+  // A default is normalized to the field type by SchemaField::Make (which casts a
+  // cross-type default and reports any cast error), so a stored default whose type does
+  // not match the field type is invalid here.
+  if (*value.type() != *field.type()) {
+    return InvalidSchema("{} of field {} has type {} but expected {}", kind, field.name(),
                          *value.type(), *field.type());
   }
   return {};
diff --git a/src/iceberg/schema_field.h b/src/iceberg/schema_field.h
@@ -64,6 +64,17 @@ class ICEBERG_EXPORT SchemaField : public iceberg::util::Formattable {
   static SchemaField MakeRequired(int32_t field_id, std::string_view name,
                                   std::shared_ptr<Type> type, std::string_view doc = {});
 
+  /// \brief Construct a field, normalizing the default values to the field type.
+  ///
+  /// Unlike the constructor (which stores the defaults verbatim), this casts a default
+  /// whose literal type differs from the field type to the field type — e.g. an int
+  /// default on a long field — matching the spec/Java cast behavior. Returns an error if
+  /// a default cannot be cast to the field type or falls outside its range.
+  static Result<SchemaField> Make(
+      int32_t field_id, std::string_view name, std::shared_ptr<Type> type, bool optional,
+      std::string_view doc = {}, std::shared_ptr<const Literal> initial_default = nullptr,
+      std::shared_ptr<const Literal> write_default = nullptr);
+
   /// \brief Get the field ID.
   [[nodiscard]] int32_t field_id() const;
 
diff --git a/src/iceberg/test/schema_json_test.cc b/src/iceberg/test/schema_json_test.cc
@@ -242,16 +242,17 @@ TEST(SchemaJsonTest, NestedFieldWithDefaultValuesRoundTrip) {
 }
 
 TEST(SchemaJsonTest, CrossTypeDefaultNormalizedRoundTrip) {
-  // A programmatically-constructed cross-type default (an int literal on a long field) is
-  // normalized to the field type at construction, so it survives a ToJson/SchemaFromJson
-  // round-trip as an equal schema (an un-normalized int literal would compare unequal to
-  // the re-parsed long literal).
-  Schema original({SchemaField(1, "id", int64(), /*optional=*/false, /*doc=*/{},
-                               std::make_shared<const Literal>(Literal::Int(34)))});
-  const auto& field = original.fields()[0];
+  // A cross-type default (an int literal on a long field) built via SchemaField::Make is
+  // normalized to the field type, so it survives a ToJson/SchemaFromJson round-trip as an
+  // equal schema (an un-normalized int literal would compare unequal to the re-parsed
+  // long literal).
+  ICEBERG_UNWRAP_OR_FAIL(
+      auto field, SchemaField::Make(1, "id", int64(), /*optional=*/false, /*doc=*/{},
+                                    std::make_shared<const Literal>(Literal::Int(34))));
   ASSERT_TRUE(field.initial_default().has_value());
   EXPECT_EQ(field.initial_default()->get(), Literal::Long(34));
 
+  Schema original({std::move(field)});
   ICEBERG_UNWRAP_OR_FAIL(auto json, ToJson(original));
   ICEBERG_UNWRAP_OR_FAIL(auto reparsed, SchemaFromJson(json));
   EXPECT_EQ(original, *reparsed);
diff --git a/src/iceberg/test/schema_test.cc b/src/iceberg/test/schema_test.cc
@@ -158,38 +158,39 @@ TEST(SchemaTest, ValidateDoesNotVersionGateWriteDefault) {
 }
 
 TEST(SchemaTest, ValidateRejectsMismatchedDefaultValue) {
+  // The constructor stores defaults verbatim (normalization happens in
+  // SchemaField::Make), so a stored default whose type differs from the field type is
+  // rejected by Validate.
   iceberg::Schema schema({iceberg::SchemaField(
       1, "id", iceberg::int32(), false, /*doc=*/{}, /*initial_default=*/nullptr,
       std::make_shared<const iceberg::Literal>(iceberg::Literal::String("oops")))});
 
-  // The default literal is cast to the field type; an uncastable type surfaces the
-  // original cast error.
   auto status = schema.Validate(iceberg::TableMetadata::kSupportedTableFormatVersion);
-  ASSERT_THAT(status, iceberg::IsError(iceberg::ErrorKind::kNotSupported));
-  EXPECT_THAT(status, iceberg::HasErrorMessage("Cast from String"));
+  ASSERT_THAT(status, iceberg::IsError(iceberg::ErrorKind::kInvalidSchema));
+  EXPECT_THAT(status, iceberg::HasErrorMessage("write-default"));
 }
 
-TEST(SchemaTest, ValidateCastsDefaultToFieldType) {
-  // Matching Java, the default literal is cast to the field type rather than required to
-  // match exactly: an int default on a long field is accepted (int -> long).
-  iceberg::Schema widened({iceberg::SchemaField(
-      1, "id", iceberg::int64(), false, /*doc=*/{},
-      std::make_shared<const iceberg::Literal>(iceberg::Literal::Int(34)))});
-  EXPECT_THAT(widened.Validate(iceberg::TableMetadata::kSupportedTableFormatVersion),
-              iceberg::IsOk());
-  // The stored default is normalized to the field type (long), not kept as the narrower
-  // int literal, so projection/serialization/equality all observe a long.
-  const auto& widened_field = widened.fields()[0];
-  ASSERT_TRUE(widened_field.initial_default().has_value());
-  EXPECT_EQ(widened_field.initial_default()->get(), iceberg::Literal::Long(34));
-
-  // A default outside the field type's range is rejected (CastTo returns an
-  // above-max/below-min sentinel).
-  iceberg::Schema out_of_range({iceberg::SchemaField(
-      1, "id", iceberg::int32(), false, /*doc=*/{},
-      std::make_shared<const iceberg::Literal>(iceberg::Literal::Long(5'000'000'000)))});
-  EXPECT_THAT(out_of_range.Validate(iceberg::TableMetadata::kSupportedTableFormatVersion),
+TEST(SchemaTest, MakeNormalizesDefaultToFieldType) {
+  // Make casts a cross-type default to the field type (int -> long) and stores the
+  // normalized value, so projection/serialization/equality all observe a long.
+  ICEBERG_UNWRAP_OR_FAIL(
+      auto field, iceberg::SchemaField::Make(1, "id", iceberg::int64(), false, /*doc=*/{},
+                                             std::make_shared<const iceberg::Literal>(
+                                                 iceberg::Literal::Int(34))));
+  ASSERT_TRUE(field.initial_default().has_value());
+  EXPECT_EQ(field.initial_default()->get(), iceberg::Literal::Long(34));
+
+  // A default outside the field type's range is rejected by Make.
+  EXPECT_THAT(iceberg::SchemaField::Make(1, "id", iceberg::int32(), false, /*doc=*/{},
+                                         std::make_shared<const iceberg::Literal>(
+                                             iceberg::Literal::Long(5'000'000'000))),
               iceberg::IsError(iceberg::ErrorKind::kInvalidSchema));
+
+  // A default whose type cannot be cast to the field type surfaces the cast error.
+  EXPECT_THAT(iceberg::SchemaField::Make(1, "id", iceberg::int32(), false, /*doc=*/{},
+                                         std::make_shared<const iceberg::Literal>(
+                                             iceberg::Literal::String("oops"))),
+              iceberg::IsError(iceberg::ErrorKind::kNotSupported));
 }
 
 TEST(SchemaTest, ReassignIdsPreservesDefaultValues) {
