From e44ac81e688b0660a7244db4c77ebf751913a5ec Mon Sep 17 00:00:00 2001 From: Xin Huang Date: Fri, 3 Jul 2026 14:09:21 -0700 Subject: [PATCH 1/3] validate decimal default fits the field precision --- src/iceberg/schema_field.cc | 13 +++++++++++++ src/iceberg/test/schema_field_test.cc | 19 +++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/src/iceberg/schema_field.cc b/src/iceberg/schema_field.cc index 80ea61bce..df5248b91 100644 --- a/src/iceberg/schema_field.cc +++ b/src/iceberg/schema_field.cc @@ -27,6 +27,7 @@ #include "iceberg/expression/literal.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" @@ -168,6 +169,18 @@ Status ValidateDefault(const SchemaField& field, const Literal& value, return InvalidSchema("{} of field {} has type {} but expected {}", kind, field.name(), *value.type(), *field.type()); } + // 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 d757cb0b2..7b15523af 100644 --- a/src/iceberg/test/schema_field_test.cc +++ b/src/iceberg/test/schema_field_test.cc @@ -25,6 +25,7 @@ #include #include "iceberg/expression/literal.h" +#include "iceberg/test/matchers.h" #include "iceberg/type.h" #include "iceberg/util/formatter.h" // IWYU pragma: keep @@ -170,4 +171,22 @@ TEST(SchemaFieldTest, CastDefaultValue) { } } +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 e25c74ef337ee9b1ae55246d9288b54fe88b2654 Mon Sep 17 00:00:00 2001 From: Xin Huang Date: Sun, 12 Jul 2026 15:09:30 -0700 Subject: [PATCH 2/3] add direct include for Result to satisfy include-cleaner --- src/iceberg/schema_field.cc | 1 + src/iceberg/test/schema_field_test.cc | 1 + 2 files changed, 2 insertions(+) diff --git a/src/iceberg/schema_field.cc b/src/iceberg/schema_field.cc index df5248b91..544e5421f 100644 --- a/src/iceberg/schema_field.cc +++ b/src/iceberg/schema_field.cc @@ -25,6 +25,7 @@ #include #include "iceberg/expression/literal.h" +#include "iceberg/result.h" #include "iceberg/type.h" #include "iceberg/util/checked_cast.h" #include "iceberg/util/decimal.h" diff --git a/src/iceberg/test/schema_field_test.cc b/src/iceberg/test/schema_field_test.cc index 7b15523af..f41131bf8 100644 --- a/src/iceberg/test/schema_field_test.cc +++ b/src/iceberg/test/schema_field_test.cc @@ -25,6 +25,7 @@ #include #include "iceberg/expression/literal.h" +#include "iceberg/result.h" #include "iceberg/test/matchers.h" #include "iceberg/type.h" #include "iceberg/util/formatter.h" // IWYU pragma: keep From 33dddddc67bcbf9eff999415bb7a63dba29ef556 Mon Sep 17 00:00:00 2001 From: Xin Huang Date: Tue, 21 Jul 2026 11:17:04 -0700 Subject: [PATCH 3/3] 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 544e5421f..7ef0566e1 100644 --- a/src/iceberg/schema_field.cc +++ b/src/iceberg/schema_field.cc @@ -174,12 +174,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 f41131bf8..2b71d2e46 100644 --- a/src/iceberg/test/schema_field_test.cc +++ b/src/iceberg/test/schema_field_test.cc @@ -179,8 +179,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) {