You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "LuciferYang (via GitHub)" <gi...@apache.org> on 2026/10/14 05:05:19 UTC
[PR] fix(metadata): drop removed schema ids from the builder's schema index [iceberg-cpp]
LuciferYang opened a new pull request, #1012:
URL: https://github.com/apache/iceberg-cpp/pull/1012
## What
`TableMetadataBuilder::Impl::RemoveSchemas` removed schemas from `metadata_.schemas` but left them in `schemas_by_id_`. In the same builder, `SetCurrentSchema` on a removed id then passed its existence check, and `Build()` produced metadata whose `current-schema-id` pointed outside `schemas`. Catalog commits apply the caller's update list on one builder and write the result, so a commit carrying `[remove-schemas, set-current-schema]` could write a metadata file that no longer loads.
Closes #1011.
## How
Erase the removed ids from `schemas_by_id_` alongside the existing filter of `metadata_.schemas`. Java's `Builder.removeSchemas` does not update `schemasById` either, so this is stricter than Java's builder.
## Testing
Two tests in `table_metadata_builder_test.cc`:
- `RemoveSchemasDropsIndexEntry`: after `RemoveSchemas({1})`, `SetCurrentSchema(1)` makes `Build()` fail with `Cannot set current schema to unknown schema: 1`. Without the fix `Build()` returns metadata with the dangling id.
- `AddSchemaAfterRemoveNotConfusedByStaleIndex`: re-adding the removed schema puts it back in `schemas`. Without the fix the stale index entry makes `AddSchema` skip it and `schemas` stays at one entry.
`RemovePartitionSpecs` has the same gap with `specs_by_id_`; I'll send that separately.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(metadata): drop removed schema ids from the builder's schema index [iceberg-cpp]
Posted by "shangxinli (via GitHub)" <gi...@apache.org>.
shangxinli commented on PR #1012:
URL: https://github.com/apache/iceberg-cpp/pull/1012#issuecomment-6222264753
lgtm
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(metadata): drop removed schema ids from the builder's schema index [iceberg-cpp]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #1012:
URL: https://github.com/apache/iceberg-cpp/pull/1012#discussion_r4344801643
##########
src/iceberg/table_metadata.cc:
##########
@@ -1006,6 +1006,9 @@ Status TableMetadataBuilder::Impl::RemoveSchemas(
return !schema_ids.contains(schema->schema_id());
}) |
std::ranges::to<std::vector<std::shared_ptr<Schema>>>();
+ for (const auto schema_id : schema_ids) {
+ schemas_by_id_.erase(schema_id);
Review Comment:
Yes, I'll take the Java side too. I want to reproduce it with a Java test first, then I'll open an issue and PR on apache/iceberg and link them here.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(metadata): drop removed schema ids from the builder's schema index [iceberg-cpp]
Posted by "shangxinli (via GitHub)" <gi...@apache.org>.
shangxinli commented on code in PR #1012:
URL: https://github.com/apache/iceberg-cpp/pull/1012#discussion_r4328473800
##########
src/iceberg/table_metadata.cc:
##########
@@ -1006,6 +1006,9 @@ Status TableMetadataBuilder::Impl::RemoveSchemas(
return !schema_ids.contains(schema->schema_id());
}) |
std::ranges::to<std::vector<std::shared_ptr<Schema>>>();
+ for (const auto schema_id : schema_ids) {
+ schemas_by_id_.erase(schema_id);
Review Comment:
This departs from Java - TableMetadata.java 1169-1183 leaves schemasById stale, so setCurrentSchema on a removed id still succeeds there. Looks right to me, just worth noting in the description.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(metadata): drop removed schema ids from the builder's schema index [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1012:
URL: https://github.com/apache/iceberg-cpp/pull/1012#discussion_r4339018241
##########
src/iceberg/table_metadata.cc:
##########
@@ -1006,6 +1006,9 @@ Status TableMetadataBuilder::Impl::RemoveSchemas(
return !schema_ids.contains(schema->schema_id());
}) |
std::ranges::to<std::vector<std::shared_ptr<Schema>>>();
+ for (const auto schema_id : schema_ids) {
+ schemas_by_id_.erase(schema_id);
Review Comment:
Do you want to fix this on the Java side as well? @LuciferYang
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(metadata): drop removed schema ids from the builder's schema index [iceberg-cpp]
Posted by "shangxinli (via GitHub)" <gi...@apache.org>.
shangxinli commented on code in PR #1012:
URL: https://github.com/apache/iceberg-cpp/pull/1012#discussion_r4328473800
##########
src/iceberg/table_metadata.cc:
##########
@@ -1006,6 +1006,9 @@ Status TableMetadataBuilder::Impl::RemoveSchemas(
return !schema_ids.contains(schema->schema_id());
}) |
std::ranges::to<std::vector<std::shared_ptr<Schema>>>();
+ for (const auto schema_id : schema_ids) {
+ schemas_by_id_.erase(schema_id);
Review Comment:
Worth calling out in the description that this departs from Java - TableMetadata.java lines 1169 to 1183 leaves schemasById untouched on removeSchemas, so there setCurrentSchema on a removed id still succeeds and emits a dangling currentSchemaId. The behavior here looks right to me, and the Java side is arguably a bug worth filing upstream.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(metadata): drop removed schema ids from the builder's schema index [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1012:
URL: https://github.com/apache/iceberg-cpp/pull/1012#issuecomment-6320477564
Thanks @LuciferYang for fixing this and @shangxinli for the review!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(metadata): drop removed schema ids from the builder's schema index [iceberg-cpp]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #1012:
URL: https://github.com/apache/iceberg-cpp/pull/1012#discussion_r4402183273
##########
src/iceberg/table_metadata.cc:
##########
@@ -1006,6 +1006,9 @@ Status TableMetadataBuilder::Impl::RemoveSchemas(
return !schema_ids.contains(schema->schema_id());
}) |
std::ranges::to<std::vector<std::shared_ptr<Schema>>>();
+ for (const auto schema_id : schema_ids) {
+ schemas_by_id_.erase(schema_id);
Review Comment:
Java side: apache/iceberg#18677 (issue) and apache/iceberg#18678 (PR).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(metadata): drop removed schema ids from the builder's schema index [iceberg-cpp]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #1012:
URL: https://github.com/apache/iceberg-cpp/pull/1012#discussion_r4333363973
##########
src/iceberg/table_metadata.cc:
##########
@@ -1006,6 +1006,9 @@ Status TableMetadataBuilder::Impl::RemoveSchemas(
return !schema_ids.contains(schema->schema_id());
}) |
std::ranges::to<std::vector<std::shared_ptr<Schema>>>();
+ for (const auto schema_id : schema_ids) {
+ schemas_by_id_.erase(schema_id);
Review Comment:
Updated the How section. Java's `build()` doesn't catch the dangling id either, so the whole sequence goes through there.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(metadata): drop removed schema ids from the builder's schema index [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac merged PR #1012:
URL: https://github.com/apache/iceberg-cpp/pull/1012
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org