From 0bd1e81859078e1ad6da0d1694e296e3d710d6ac Mon Sep 17 00:00:00 2001 From: Xin Huang Date: Fri, 3 Jul 2026 14:09:21 -0700 Subject: [PATCH 1/2] validate decimal default fits the field precision --- src/iceberg/schema_field.cc | 13 +++++++++++++ src/iceberg/test/schema_field_test.cc | 18 ++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/src/iceberg/schema_field.cc b/src/iceberg/schema_field.cc index d09852a04..a3659a30a 100644 --- a/src/iceberg/schema_field.cc +++ b/src/iceberg/schema_field.cc @@ -29,6 +29,7 @@ #include "iceberg/result.h" #include "iceberg/type.h" #include "iceberg/util/checked_cast.h" +#include "iceberg/util/decimal.h" #include "iceberg/util/formatter.h" // IWYU pragma: keep #include "iceberg/util/macros.h" @@ -185,6 +186,18 @@ Status ValidateDefault(const SchemaField& field, const Literal& value, return InvalidSchema("Invalid {} value for {}: value must be finite", kind, field.name()); } + // A decimal literal carries a precision in its type but stores only an unscaled value, + // so a value that does not fit the field precision would pass the type check above yet + // be rejected when the metadata is parsed back. Reject it here to keep the default + // consistent with what the reader accepts. + if (field.type()->type_id() == TypeId::kDecimal && + std::holds_alternative(value.value())) { + const auto& decimal_type = internal::checked_cast(*field.type()); + if (!std::get(value.value()).FitsInPrecision(decimal_type.precision())) { + return InvalidSchema("{} of field {} does not fit precision {}", kind, field.name(), + decimal_type.precision()); + } + } return {}; } diff --git a/src/iceberg/test/schema_field_test.cc b/src/iceberg/test/schema_field_test.cc index 434742ab6..844b98f0a 100644 --- a/src/iceberg/test/schema_field_test.cc +++ b/src/iceberg/test/schema_field_test.cc @@ -203,4 +203,22 @@ TEST(SchemaFieldTest, ValidateAcceptsFiniteFloatingDefault) { EXPECT_THAT(field.Validate(), IsOk()); } +TEST(SchemaFieldTest, ValidateRejectsDecimalDefaultExceedingPrecision) { + // A decimal default whose unscaled value needs more digits than the field precision has + // the matching literal type (decimal(2, 1)) but does not fit; Validate must reject it + // so it is never serialized to metadata the reader would later refuse to parse. + SchemaField field(/*field_id=*/1, /*name=*/"d", decimal(2, 1), + /*optional=*/true, /*doc=*/"", + std::make_shared(Literal::Decimal(999, 2, 1))); + EXPECT_THAT(field.Validate(), IsError(ErrorKind::kInvalidSchema)); + EXPECT_THAT(field.Validate(), HasErrorMessage("does not fit precision")); +} + +TEST(SchemaFieldTest, ValidateAcceptsDecimalDefaultWithinPrecision) { + SchemaField field(/*field_id=*/1, /*name=*/"d", decimal(4, 1), + /*optional=*/true, /*doc=*/"", + std::make_shared(Literal::Decimal(999, 4, 1))); + EXPECT_THAT(field.Validate(), IsOk()); +} + } // namespace iceberg From 24f780790b4448aa8836400db5419d25825c400c Mon Sep 17 00:00:00 2001 From: Xin Huang Date: Tue, 21 Jul 2026 11:17:04 -0700 Subject: [PATCH 2/2] Address review: use std::get_if and capture Validate status once --- src/iceberg/schema_field.cc | 14 ++++++++------ src/iceberg/test/schema_field_test.cc | 5 +++-- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/iceberg/schema_field.cc b/src/iceberg/schema_field.cc index a3659a30a..7a6e90fc9 100644 --- a/src/iceberg/schema_field.cc +++ b/src/iceberg/schema_field.cc @@ -190,12 +190,14 @@ Status ValidateDefault(const SchemaField& field, const Literal& value, // so a value that does not fit the field precision would pass the type check above yet // be rejected when the metadata is parsed back. Reject it here to keep the default // consistent with what the reader accepts. - if (field.type()->type_id() == TypeId::kDecimal && - std::holds_alternative(value.value())) { - const auto& decimal_type = internal::checked_cast(*field.type()); - if (!std::get(value.value()).FitsInPrecision(decimal_type.precision())) { - return InvalidSchema("{} of field {} does not fit precision {}", kind, field.name(), - decimal_type.precision()); + if (field.type()->type_id() == TypeId::kDecimal) { + if (const auto* dec = std::get_if(&value.value())) { + const auto& decimal_type = + internal::checked_cast(*field.type()); + if (!dec->FitsInPrecision(decimal_type.precision())) { + return InvalidSchema("{} of field {} does not fit precision {}", kind, + field.name(), decimal_type.precision()); + } } } return {}; diff --git a/src/iceberg/test/schema_field_test.cc b/src/iceberg/test/schema_field_test.cc index 844b98f0a..7a66b9d34 100644 --- a/src/iceberg/test/schema_field_test.cc +++ b/src/iceberg/test/schema_field_test.cc @@ -210,8 +210,9 @@ TEST(SchemaFieldTest, ValidateRejectsDecimalDefaultExceedingPrecision) { SchemaField field(/*field_id=*/1, /*name=*/"d", decimal(2, 1), /*optional=*/true, /*doc=*/"", std::make_shared(Literal::Decimal(999, 2, 1))); - EXPECT_THAT(field.Validate(), IsError(ErrorKind::kInvalidSchema)); - EXPECT_THAT(field.Validate(), HasErrorMessage("does not fit precision")); + auto status = field.Validate(); + EXPECT_THAT(status, IsError(ErrorKind::kInvalidSchema)); + EXPECT_THAT(status, HasErrorMessage("does not fit precision")); } TEST(SchemaFieldTest, ValidateAcceptsDecimalDefaultWithinPrecision) {