Skip to content

fix(table): reject nil uuid assignment updates #1329

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
laskoviymishka merged 2 commits into from
Jul 2, 2026
+45 −4
Merged

fix(table): reject nil uuid assignment updates #1329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6a39da3
fix(table): reject nil uuid assignment updates
fallintoplace Jun 27, 2026
33c5a22
fix(table): classify nil uuid assignment errors
fallintoplace Jun 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions table/metadata.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -902,13 +902,17 @@ func (b *MetadataBuilder) RemoveSnapshotRef(name string) error {
return nil
}

func (b *MetadataBuilder) SetUUID(uuid uuid.UUID) error {

@tanmayrauth tanmayrauth Jun 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, routing the guard through SetUUID means the assign-uuid update path (updates.go:177 -> SetUUID) is covered by the same check, so no second guard is needed. And the uuid -> newUUID rename is actually required here, since the old uuid param shadowed the package and uuid.Nil wasn't referenceable. Matches Java's assignUUID precondition too.

if b.uuid == uuid {
func (b *MetadataBuilder) SetUUID(newUUID uuid.UUID) error {
if newUUID == uuid.Nil {
return fmt.Errorf("%w: cannot set uuid to nil", iceberg.ErrInvalidArgument)
}

if b.uuid == newUUID {

@tanmayrauth tanmayrauth Jun 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional / possibly out of scope: Java's assignUUID enforces a second invariant beyond non-nul, "Cannot reassign uuid", i.e. once a non-nil UUID is set you can't change it to a different one. Here SetUUID
still allows reassigning to a different non-nil UUID. Given the rationale ("the table UUID uniquely identifies the table"), do you want to reject that case too, or keep it for a separate PR?

return nil
}

b.updates = append(b.updates, NewAssignUUIDUpdate(uuid))
b.uuid = uuid
b.updates = append(b.updates, NewAssignUUIDUpdate(newUUID))
b.uuid = newUUID

return nil
}
Expand Down
24 changes: 24 additions & 0 deletions table/metadata_builder_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2317,6 +2317,30 @@ func TestSetFormatVersionPreservesExistingUUID(t *testing.T) {
require.Equal(t, existingUUID, meta.TableUUID())
}

func TestSetUUIDRejectsNil(t *testing.T) {
builder := builderWithoutChanges(2)
originalUUID := builder.uuid

err := builder.SetUUID(uuid.Nil)
require.ErrorIs(t, err, iceberg.ErrInvalidArgument)
require.False(t, builder.HasChanges())
require.Equal(t, originalUUID, builder.uuid)

meta, err := builder.Build()
require.NoError(t, err)
require.Equal(t, originalUUID, meta.TableUUID())

@laskoviymishka laskoviymishka Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole design rests on NewMetadata/NewMetadataWithUUID converting uuid.Nil to uuid.New() before reaching the new guard, but nothing here asserts it. A future regression where that conversion gets dropped would now turn into a hard "cannot set uuid" error instead of a generated UUID, and we'd have no test catching it. Could we add a quick case calling NewMetadataWithUUID(..., uuid.Nil) and asserting meta.TableUUID() != uuid.Nil? Cheap, and it locks down the path the guard relies on.

}

func TestNewMetadataWithUUIDGeneratesUUIDForNil(t *testing.T) {
tableSchema := schema()
partSpec := partitionSpec()
order := sortOrder()

meta, err := NewMetadataWithUUID(&tableSchema, &partSpec, order, "s3://bucket/test/location", nil, uuid.Nil)
require.NoError(t, err)
require.NotEqual(t, uuid.Nil, meta.TableUUID())
}

func TestSetFormatVersionDowngradeNotAllowed(t *testing.T) {
builder := builderWithoutChanges(2)
err := builder.SetFormatVersion(1)
Expand Down
13 changes: 13 additions & 0 deletions table/updates_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,19 @@ func buildFromBase(t *testing.T) *MetadataBuilder {
return b
}

func TestAssignUUIDUpdate_ApplyRejectsNilUUID(t *testing.T) {
b := buildFromBase(t)
originalUUID := b.uuid

err := NewAssignUUIDUpdate(uuid.Nil).Apply(b)
require.ErrorIs(t, err, iceberg.ErrInvalidArgument)
require.False(t, b.HasChanges())

meta, err := b.Build()
require.NoError(t, err)
require.Equal(t, originalUUID, meta.TableUUID())
}

func TestSetStatisticsUpdate_Unmarshal(t *testing.T) {
data := []byte(`[{
"action": "set-statistics",
Expand Down
Loading