Repository navigation
fix(metadata): drop removed schema ids from the builder's schema index - #1012
Conversation
RemoveSchemas filtered metadata_.schemas but left schemas_by_id_ untouched, so within one builder session the removed ids stayed resolvable: SetCurrentSchema on a removed id passed its existence check and Build() emitted metadata whose current-schema-id dangles outside schemas, and a later AddSchema reuse of the id took the update path against a schema the metadata no longer contains. Erase the removed ids from the index alongside the vector filter.
|
lgtm |
| }) | | ||
| std::ranges::to<std::vector<std::shared_ptr<Schema>>>(); | ||
| for (const auto schema_id : schema_ids) { | ||
| schemas_by_id_.erase(schema_id); |
There was a problem hiding this 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.
There was a problem hiding this comment.
Updated the How section. Java's build() doesn't catch the dangling id either, so the whole sequence goes through there.
There was a problem hiding this comment.
Do you want to fix this on the Java side as well? @LuciferYang
There was a problem hiding this 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.
…n in one transaction
| }) | | ||
| std::ranges::to<std::vector<std::shared_ptr<Schema>>>(); | ||
| for (const auto schema_id : schema_ids) { | ||
| schemas_by_id_.erase(schema_id); |
There was a problem hiding this comment.
Do you want to fix this on the Java side as well? @LuciferYang
|
Thanks @LuciferYang for fixing this and @shangxinli for the review! |
What
TableMetadataBuilder::Impl::RemoveSchemasremoved schemas frommetadata_.schemasbut left them inschemas_by_id_, so later calls on the same builder still resolved the removed ids.Transactionapplies all of its updates to one builder, so this was reachable through the public API. Take a table whose current schema is 0 and whose schema 1 is unused, for example because a column was added and then dropped. In one transaction,ExpireSnapshotswithCleanExpiredMetadata(true)removes schema 1, andUpdateSchema::AddColumnthen produces a new schema that gets the freed id 1.AddSchemafinds id 1 in the stale index and does not add the new schema toschemas, andSetCurrentSchema(1)picks up the removed schema instead. Without this fix the commit succeeds and writescurrent-schema-id: 1with only schema 0 inschemas, and the nextLoadTablefails withCannot find schema with current-schema-id=1.The same stale entry let
SetCurrentSchemaaccept a removed id directly, so a catalog commit carrying[remove-schemas, set-current-schema]for that id wrote the same kind of file.Closes #1011.
How
Erase the removed ids from
schemas_by_id_alongside the existing filter ofmetadata_.schemas.This is stricter than Java. Java's
Builder.removeSchemasleavesschemasByIdstale, sosetCurrentSchemaon a removed id still succeeds there, andbuild()returns metadata whosecurrent-schema-idis not inschemas. In C++ the same sequence makesBuild()fail withCannot set current schema to unknown schema.Testing
ExpireUnusedSchemaThenAddColumnintransaction_test.ccruns the transaction above againstInMemoryCatalog. Without the fixCommit()succeeds and reloading the table fails withCannot find schema with current-schema-id=1. With the fix the reloaded current schema is 1 and has the added column.RemoveSchemasDropsIndexEntryintable_metadata_builder_test.cc: afterRemoveSchemas({1}),SetCurrentSchema(1)makesBuild()fail withCannot set current schema to unknown schema: 1. Without the fixBuild()returns metadata with the dangling id.AddSchemaAfterRemoveNotConfusedByStaleIndexintable_metadata_builder_test.cc: re-adding the removed schema puts it back inschemas. Without the fix the stale index entry makesAddSchemaskip it andschemasstays at one entry.RemovePartitionSpecshas the same gap withspecs_by_id_; I'll send that separately.