Copilot commented on code in PR #809:
URL: https://github.com/apache/iceberg-cpp/pull/809#discussion_r3619367983
##########
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:
`field.Validate()` is executed twice here. Capturing the `Status` once
avoids duplicated work and prevents surprises if validation ever becomes
non-trivial or gains side effects.
##########
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:
This block queries `value.value()` and extracts the `Decimal` twice. Storing
a reference to the variant and using `std::get_if` makes the code clearer and
avoids redundant variant access (while keeping the guard if the literal value
is unexpectedly non-decimal).
--
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]
