fix(schema): review follow-ups — lenient float default parse, must-be-null types, tests

- LiteralFromJson accepts an integer JSON token for float/double defaults (Java writes a
  float default as a bare integer), so Java-produced v3 metadata parses in C++.
- ValidateDefault rejects a non-null default on unknown/variant/geometry/geography fields
  (the spec requires those to default to null).
- Tests: full TableMetadata-level v3 default round-trip; non-primitive + must-be-null
  default rejection; SchemaField equality distinguishes default values; present-null
  write-default dropped to absence; integer-encoded float default parses.

Author: huan233usc

src/iceberg/expression/json_serde.cc

@@ -342,13 +342,15 @@ Result<Literal> LiteralFromJson(const nlohmann::json& json, const Type* type) {
       return Literal::Long(json.get<int64_t>());
 
     case TypeId::kFloat:
-      if (!json.is_number_float()) [[unlikely]] {
+      // Accept an integer JSON token too (e.g. `1`), matching Java's lenient numeric
+      // parsing; otherwise a float default written as a bare integer fails to read.
+      if (!json.is_number()) [[unlikely]] {
         return JsonParseError("Cannot parse {} as a float value", SafeDumpJson(json));
       }
       return Literal::Float(json.get<float>());
 
     case TypeId::kDouble:
-      if (!json.is_number_float()) [[unlikely]] {
+      if (!json.is_number()) [[unlikely]] {
         return JsonParseError("Cannot parse {} as a double value", SafeDumpJson(json));
       }
       return Literal::Double(json.get<double>());

src/iceberg/schema_field.cc

@@ -131,11 +131,27 @@ Status ValidateDefault(const SchemaField& field, const Literal& value,
     return InvalidSchema("Invalid {} value for {}: value is out of range", kind,
                          field.name());
   }
-  // Defaults are only supported on primitive fields. The spec also permits JSON
-  // single-value defaults for struct/list/map (e.g. an empty struct `{}` whose
-  // sub-field defaults live in field metadata); that matches the current Java model's
-  // gap and is left as a follow-up.
-  if (field.type() == nullptr || !field.type()->is_primitive()) {
+  if (field.type() == nullptr) {
+    return InvalidSchema("Invalid {} value for {}: field has no type", kind,
+                         field.name());
+  }
+  // The spec requires unknown/variant/geometry/geography columns to default to null, so a
+  // non-null default on them is invalid (a null default was already dropped as absence).
+  switch (field.type()->type_id()) {
+    case TypeId::kUnknown:
+    case TypeId::kVariant:
+    case TypeId::kGeometry:
+    case TypeId::kGeography:
+      return InvalidSchema("Invalid {} value for {}: {} fields must default to null",
+                           kind, field.name(), *field.type());
+    default:
+      break;
+  }
+  // Defaults are otherwise only supported on primitive fields. The spec also permits JSON
+  // single-value defaults for struct/list/map (e.g. an empty struct `{}` whose sub-field
+  // defaults live in field metadata); that matches the current Java model's gap and is
+  // left as a follow-up.
+  if (!field.type()->is_primitive()) {
     return InvalidSchema(
         "Invalid {} value for {}: default values are only supported for primitive types",
         kind, field.name());

src/iceberg/test/metadata_serde_test.cc

@@ -23,6 +23,7 @@
 #include <gtest/gtest.h>
 #include <nlohmann/json.hpp>
 
+#include "iceberg/expression/literal.h"
 #include "iceberg/json_serde_internal.h"
 #include "iceberg/partition_field.h"
 #include "iceberg/partition_spec.h"
@@ -445,6 +446,25 @@ TEST(MetadataSerdeTest, DeserializePartitionStatisticsFiles) {
   ASSERT_EQ(*metadata, expected);
 }
 
+TEST(MetadataSerdeTest, V3DefaultValuesRoundTrip) {
+  // Full TableMetadata path: a v3 schema field's initial/write defaults parse correctly
+  // (the v3 gate in Schema::Validate is satisfied) and survive a ToJson/FromJson round
+  // trip. The fixture's field is `x: long` with initial-default 1 and write-default 1.
+  ICEBERG_UNWRAP_OR_FAIL(
+      auto metadata, ReadTableMetadataFromResource("TableMetadataV3ValidMinimal.json"));
+  auto schema_result = metadata->Schema();
+  ASSERT_TRUE(schema_result.has_value());
+  const auto& field = schema_result.value()->fields()[0];
+  ASSERT_NE(field.initial_default(), nullptr);
+  EXPECT_EQ(*field.initial_default(), Literal::Long(1));
+  ASSERT_NE(field.write_default(), nullptr);
+  EXPECT_EQ(*field.write_default(), Literal::Long(1));
+
+  ICEBERG_UNWRAP_OR_FAIL(auto json, ToJson(*metadata));
+  ICEBERG_UNWRAP_OR_FAIL(auto reparsed, TableMetadataFromJson(json));
+  EXPECT_EQ(*reparsed, *metadata);
+}
+
 TEST(MetadataSerdeTest, DeserializeUnsupportedVersion) {
   ReadTableMetadataExpectError("TableMetadataUnsupportedVersion.json",
                                "Cannot read unsupported version");

src/iceberg/test/schema_json_test.cc

@@ -258,6 +258,17 @@ TEST(SchemaJsonTest, CrossTypeDefaultNormalizedRoundTrip) {
   EXPECT_EQ(original, *reparsed);
 }
 
+TEST(SchemaJsonTest, FloatDefaultAcceptsIntegerEncoding) {
+  // Java writes a float/double default as a bare integer token (e.g. 1, not 1.0); it must
+  // parse, matching Java's lenient numeric reading.
+  constexpr std::string_view json =
+      R"({"fields":[{"id":1,"initial-default":1,"name":"f","required":true,"type":"float"}],"schema-id":1,"type":"struct"})";
+  ICEBERG_UNWRAP_OR_FAIL(auto schema, SchemaFromJson(nlohmann::json::parse(json)));
+  const auto& field = schema->fields()[0];
+  ASSERT_NE(field.initial_default(), nullptr);
+  EXPECT_EQ(*field.initial_default(), Literal::Float(1.0F));
+}
+
 TEST(SchemaJsonTest, UnknownFieldRoundTrip) {
   constexpr std::string_view json =
       R"({"fields":[{"id":1,"name":"mystery","required":false,"type":"unknown"}],"schema-id":1,"type":"struct"})";

src/iceberg/test/schema_test.cc

@@ -199,8 +199,10 @@ TEST(SchemaTest, NullDefaultModeledAsAbsence) {
   // field with no default, and it validates cleanly.
   iceberg::SchemaField with_null(
       1, "id", iceberg::int32(), /*optional=*/true, /*doc=*/{},
+      std::make_shared<const iceberg::Literal>(iceberg::Literal::Null(iceberg::int32())),
       std::make_shared<const iceberg::Literal>(iceberg::Literal::Null(iceberg::int32())));
   EXPECT_EQ(with_null.initial_default(), nullptr);
+  EXPECT_EQ(with_null.write_default(), nullptr);
 
   iceberg::SchemaField no_default(1, "id", iceberg::int32(), /*optional=*/true);
   EXPECT_EQ(with_null, no_default);
@@ -210,6 +212,39 @@ TEST(SchemaTest, NullDefaultModeledAsAbsence) {
               iceberg::IsOk());
 }
 
+TEST(SchemaTest, EqualsDistinguishesDefaultValues) {
+  auto field = [](std::shared_ptr<const iceberg::Literal> d) {
+    return iceberg::SchemaField(1, "id", iceberg::int32(), /*optional=*/true, /*doc=*/{},
+                                std::move(d));
+  };
+  // Differ only in default value -> unequal; default vs no-default -> unequal.
+  EXPECT_NE(field(std::make_shared<const iceberg::Literal>(iceberg::Literal::Int(1))),
+            field(std::make_shared<const iceberg::Literal>(iceberg::Literal::Int(2))));
+  EXPECT_NE(field(std::make_shared<const iceberg::Literal>(iceberg::Literal::Int(1))),
+            field(nullptr));
+}
+
+TEST(SchemaTest, ValidateRejectsDefaultOnNonPrimitiveAndMustBeNullTypes) {
+  // A struct (non-primitive) field with a non-null default is rejected.
+  iceberg::Schema struct_default({iceberg::SchemaField(
+      1, "s",
+      MakeStructType(iceberg::SchemaField(2, "x", iceberg::int32(), /*optional=*/true)),
+      /*optional=*/true, /*doc=*/{},
+      std::make_shared<const iceberg::Literal>(iceberg::Literal::Int(1)))});
+  EXPECT_THAT(
+      struct_default.Validate(iceberg::TableMetadata::kSupportedTableFormatVersion),
+      iceberg::IsError(iceberg::ErrorKind::kInvalidSchema));
+
+  // unknown/geometry/geography must default to null: a non-null default is rejected.
+  iceberg::Schema geo_default({iceberg::SchemaField(
+      1, "g", iceberg::geometry(), /*optional=*/true, /*doc=*/{},
+      std::make_shared<const iceberg::Literal>(iceberg::Literal::Int(1)))});
+  auto status =
+      geo_default.Validate(iceberg::TableMetadata::kSupportedTableFormatVersion);
+  ASSERT_THAT(status, iceberg::IsError(iceberg::ErrorKind::kInvalidSchema));
+  EXPECT_THAT(status, iceberg::HasErrorMessage("must default to null"));
+}
+
 TEST(SchemaTest, ReassignIdsPreservesDefaultValues) {
   // Reassigning field IDs rebuilds each SchemaField, so the rebuild must carry the
   // default values over to the field with the new ID.