Copilot commented on code in PR #902:
URL: https://github.com/apache/iceberg-cpp/pull/902#discussion_r3871577277
##########
src/iceberg/table_metadata.cc:
##########
@@ -122,6 +123,30 @@ Result<std::shared_ptr<SortOrder>> FreshSortOrder(int32_t
order_id,
return SortOrder::Make(order_id, std::move(sort_fields));
}
+Status ValidatePreservedFieldIds(const Schema& schema) {
+ std::unordered_set<int32_t> field_ids;
+ std::function<Status(const Type&)> validate_type = [&](const Type& type) ->
Status {
+ if (!type.is_nested()) {
+ return {};
+ }
+
+ const auto& nested = internal::checked_cast<const NestedType&>(type);
+ for (const auto& field : nested.fields()) {
+ if (field.field_id() <= Schema::kInitialColumnId) {
+ return InvalidSchema("Invalid field id {} for '{}'", field.field_id(),
+ field.name());
+ }
+ if (!field_ids.insert(field.field_id()).second) {
+ return InvalidSchema("Duplicate field id found: {}", field.field_id());
+ }
+ ICEBERG_RETURN_UNEXPECTED(validate_type(*field.type()));
+ }
+ return {};
+ };
+
+ return validate_type(schema);
Review Comment:
`validate_type` expects a `const Type&`, but this function passes `schema`
(a `Schema`) into it. Unless `Schema` is implicitly convertible to `Type` in
this codebase, this won’t compile / won’t validate the intended root struct. A
safer approach is to iterate from `schema.fields()` (validating each field id
and recursing into `*field.type()`) or to call `validate_type` on the schema’s
underlying root type/struct explicitly.
##########
src/iceberg/table_metadata.cc:
##########
@@ -237,6 +262,46 @@ Result<std::unique_ptr<TableMetadata>> TableMetadata::Make(
.Build();
}
+Result<std::unique_ptr<TableMetadata>> TableMetadata::MakeWithFieldIds(
+ const iceberg::Schema& schema, const iceberg::PartitionSpec& spec,
+ const iceberg::SortOrder& sort_order, const std::string& location,
+ const std::unordered_map<std::string, std::string>& properties, int
format_version) {
+ for (const auto& [key, _] : properties) {
+ if (TableProperties::reserved_properties().contains(key)) {
+ return InvalidArgument(
+ "Table properties should not contain reserved properties, but got
{}", key);
+ }
+ }
Review Comment:
The reserved-property validation logic is now duplicated between `Make` and
`MakeWithFieldIds` (and likely other creation paths). Consider extracting a
shared helper (e.g., `ValidateCreateProperties(properties)`) to avoid
divergence over time (e.g., if reserved keys change or more validation is added
later).
##########
src/iceberg/table_metadata.cc:
##########
@@ -122,6 +123,30 @@ Result<std::shared_ptr<SortOrder>> FreshSortOrder(int32_t
order_id,
return SortOrder::Make(order_id, std::move(sort_fields));
}
+Status ValidatePreservedFieldIds(const Schema& schema) {
+ std::unordered_set<int32_t> field_ids;
+ std::function<Status(const Type&)> validate_type = [&](const Type& type) ->
Status {
+ if (!type.is_nested()) {
+ return {};
+ }
+
+ const auto& nested = internal::checked_cast<const NestedType&>(type);
+ for (const auto& field : nested.fields()) {
+ if (field.field_id() <= Schema::kInitialColumnId) {
+ return InvalidSchema("Invalid field id {} for '{}'", field.field_id(),
+ field.name());
+ }
+ if (!field_ids.insert(field.field_id()).second) {
+ return InvalidSchema("Duplicate field id found: {}", field.field_id());
+ }
+ ICEBERG_RETURN_UNEXPECTED(validate_type(*field.type()));
+ }
+ return {};
+ };
+
+ return validate_type(schema);
Review Comment:
The duplicate-ID error message only reports the ID. For nested schemas,
debugging is easier if the message also includes the field name (and ideally a
path) for the duplicate occurrence (e.g., include `field.name()` or a
constructed path as you traverse).
##########
src/iceberg/table_metadata.cc:
##########
@@ -122,6 +123,30 @@ Result<std::shared_ptr<SortOrder>> FreshSortOrder(int32_t
order_id,
return SortOrder::Make(order_id, std::move(sort_fields));
}
+Status ValidatePreservedFieldIds(const Schema& schema) {
Review Comment:
The new preserved-ID validation claims to enforce global uniqueness across
nested fields, but the added tests only cover a top-level invalid (0) ID. Add
unit tests that exercise (1) duplicate IDs across fields, and (2)
invalid/duplicate IDs inside nested types (struct/list/map element fields) to
ensure the recursion and uniqueness checks behave as intended.
##########
src/iceberg/table_metadata.cc:
##########
@@ -122,6 +123,30 @@ Result<std::shared_ptr<SortOrder>> FreshSortOrder(int32_t
order_id,
return SortOrder::Make(order_id, std::move(sort_fields));
}
+Status ValidatePreservedFieldIds(const Schema& schema) {
+ std::unordered_set<int32_t> field_ids;
+ std::function<Status(const Type&)> validate_type = [&](const Type& type) ->
Status {
Review Comment:
Using `std::function` for recursion can introduce type-erasure overhead and
may heap-allocate depending on the implementation. This can be avoided by using
a self-recursive lambda pattern (passing itself as a parameter) so the
validator remains fully inlined and you can potentially drop the `<functional>`
include.
--
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]
