Repository navigation
Assert the last assigned field id when update_schema adds a schema - #4205
dragonknightk wants to merge 1 commit into
Conversation
Closes issue 4190 # Rationale for this change `UpdateSchema._commit` only sent `AssertCurrentSchemaId`, so a stale `update_schema` could commit after another writer had moved the current schema away and back. For example: - writer A adds a column (field id 2, schema 1), then drops it, which makes schema 0 current again - writer B, loaded before A's changes, adds a column: its `AssertCurrentSchemaId(0)` still passes, so it reuses field id 2 and adds a second schema with id 1 Java's `UpdateRequirements` adds `AssertLastAssignedFieldId(base.lastColumnId())` when a commit adds a schema (`update(MetadataUpdate.AddSchema)`). pyiceberg already defines `AssertLastAssignedFieldId` but never sent it. ### What changed - `UpdateSchema._commit` adds `AssertLastAssignedFieldId` with the transaction's `last_column_id` when it adds a new schema - this follows `UpdateSpec._commit`, which adds `AssertLastAssignedPartitionId` when it adds a spec - switching back to an existing schema adds no schema, so it only sends `AssertCurrentSchemaId`, as before With this change, writer B fails with `CommitFailedException` and the table stays unchanged (｀・ω・´) ## Are these changes tested? Yes. `test_concurrent_add_column_after_add_and_drop` in `tests/catalog/test_catalog_behaviors.py` runs the scenario from the issue against the memory and SQL catalogs: - with this change, writer B fails with `last assigned field id has changed: expected 3, found 4` - without it, the test fails with `DID NOT RAISE CommitFailedException` ## Are there any user-facing changes? Yes, a bug fix: a stale `update_schema` that adds a schema now raises `CommitFailedException` when the table's last assigned field id has changed, instead of reusing field ids. Refresh the table and retry, as with other requirement failures (・ω・)b ## AI Disclosure Human-led contribution. Grok Bot, working for dragonknightk, assisted with this change. All submitted content was reviewed by a human before submission.
rambleraptor
left a comment
rambleraptor
left a comment
There was a problem hiding this comment.
Looks pretty straight forward. There's some follow-ups (stale writer can create duplicate schema IDs). Thanks a lot!
Thanks for the review (´｡• ᵕ •｡`) And thanks for flagging the stale writer case — you're right that it's still open, a rename that gets reverted doesn't move the last assigned field id, so the next schema id can still collide. That one comes from where the next schema id is picked rather than from the assertion added here, so I've left it out of this PR to keep the change small. Will keep an eye on it separately ʕ•ᴥ•ʔ |
|
@kevinjqliu This small fix adds |
Closes #4190
Rationale for this change
UpdateSchema._commitonly sentAssertCurrentSchemaId, so a staleupdate_schemacould commit after another writer had moved the current schema away and back. For example:AssertCurrentSchemaId(0)still passes, so it reuses field id 2 and adds a second schema with id 1Java's
UpdateRequirementsaddsAssertLastAssignedFieldId(base.lastColumnId())when a commit adds a schema (update(MetadataUpdate.AddSchema)). pyiceberg already definesAssertLastAssignedFieldIdbut never sent it.What changed
UpdateSchema._commitaddsAssertLastAssignedFieldIdwith the transaction'slast_column_idwhen it adds a new schemaUpdateSpec._commit, which addsAssertLastAssignedPartitionIdwhen it adds a specAssertCurrentSchemaId, as beforeWith this change, writer B fails with
CommitFailedExceptionand the table stays unchanged (｀・ω・´)Are these changes tested?
Yes.
test_concurrent_add_column_after_add_and_dropintests/catalog/test_catalog_behaviors.pyruns the scenario from the issue against the memory and SQL catalogs:last assigned field id has changed: expected 3, found 4DID NOT RAISE CommitFailedExceptionAre there any user-facing changes?
Yes, a bug fix: a stale
update_schemathat adds a schema now raisesCommitFailedExceptionwhen the table's last assigned field id has changed, instead of reusing field ids. Refresh the table and retry, as with other requirement failures (・ω・)bAI Disclosure
Human-led contribution. Grok Bot, working for dragonknightk, assisted with this change. All submitted content was reviewed by a human before submission.