Copilot commented on code in PR #746:
URL: https://github.com/apache/iceberg-cpp/pull/746#discussion_r3437260655
##########
src/iceberg/schema.cc:
##########
@@ -447,7 +453,21 @@ Status Schema::Validate(int32_t format_version) const {
}
}
- // TODO(GuoTao.yu): Check default values when they are supported
+ // Only the initial-default is gated on format version: it changes how
existing
+ // data files are read (rows written before the column existed materialize
this
+ // value), so it requires the v3 reader contract. A write-default only
affects
+ // values written going forward and does not reinterpret existing data.
+ if (field.initial_default().has_value() &&
+ format_version < TableMetadata::kMinFormatVersionDefaultValues) {
+ return InvalidSchema(
+ "Invalid initial default for {}: non-null default ({}) is not
supported "
+ "until v{}",
+ field.name(), field.initial_default()->get(),
+ TableMetadata::kMinFormatVersionDefaultValues);
+ }
+ if (field.initial_default().has_value() ||
field.write_default().has_value()) {
+ ICEBERG_RETURN_UNEXPECTED(field.Validate());
+ }
Review Comment:
The PR description says `Schema::Validate` rejects default values below
format v3, but this implementation only version-gates `initial-default` and
explicitly allows `write-default` for format < v3 (and there’s a new test
asserting that behavior). Please align the implementation/tests with the
intended contract (either gate `write-default` too, or update the PR
description/constant naming to reflect the narrower gating).
##########
src/iceberg/schema_field.cc:
##########
@@ -55,13 +62,64 @@ bool SchemaField::optional() const { return optional_; }
std::string_view SchemaField::doc() const { return doc_; }
+std::optional<std::reference_wrapper<const Literal>>
SchemaField::initial_default()
+ const {
+ if (initial_default_ == nullptr) {
+ return std::nullopt;
+ }
+ return std::cref(*initial_default_);
+}
+
+std::optional<std::reference_wrapper<const Literal>>
SchemaField::write_default() const {
+ if (write_default_ == nullptr) {
+ return std::nullopt;
+ }
+ return std::cref(*write_default_);
+}
+
+const std::shared_ptr<const Literal>& SchemaField::initial_default_ptr() const
{
+ return initial_default_;
+}
+
+const std::shared_ptr<const Literal>& SchemaField::write_default_ptr() const {
+ return write_default_;
+}
+
+namespace {
+
+Status ValidateDefault(const SchemaField& field, const Literal& value,
+ std::string_view kind) {
+ if (value.IsNull() || value.IsAboveMax() || value.IsBelowMin()) {
+ return InvalidSchema("Invalid {} value for {}: must be a non-null value",
kind,
+ field.name());
+ }
+ if (field.type() == nullptr || !field.type()->is_primitive()) {
+ return InvalidSchema("Invalid {} value for {}: {} (must be null)", kind,
field.name(),
+ value);
+ }
Review Comment:
`ValidateDefault` returns an error message saying the default "(must be
null)" when the field type is non-primitive, but earlier in the same function
null defaults are rejected. This message is confusing and doesn’t reflect the
real constraint (defaults are only supported for primitive field types).
--
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]
