huan233usc commented on code in PR #809:
URL: https://github.com/apache/iceberg-cpp/pull/809#discussion_r3624631069
##########
src/iceberg/schema_field.cc:
##########
@@ -168,6 +170,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<Decimal>(value.value())) {
+ const auto& decimal_type = internal::checked_cast<const
DecimalType&>(*field.type());
+ if
(!std::get<Decimal>(value.value()).FitsInPrecision(decimal_type.precision())) {
+ return InvalidSchema("{} of field {} does not fit precision {}", kind,
field.name(),
+ decimal_type.precision());
+ }
+ }
Review Comment:
Fixed in 33dddddc: switched to `std::get_if<Decimal>` so we only touch the
variant once (also covers @WZhuo's suggestion).
##########
src/iceberg/test/schema_field_test.cc:
##########
@@ -170,4 +172,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<const Literal>(Literal::Decimal(999, 2,
1)));
+ EXPECT_THAT(field.Validate(), IsError(ErrorKind::kInvalidSchema));
+ EXPECT_THAT(field.Validate(), HasErrorMessage("does not fit precision"));
Review Comment:
Fixed in 33dddddc: validate once and assert on the captured `Status`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
