tanmayrauth commented on code in PR #1329:
URL: https://github.com/apache/iceberg-go/pull/1329#discussion_r3486531274
##########
table/metadata.go:
##########
@@ -902,13 +902,17 @@ func (b *MetadataBuilder) RemoveSnapshotRef(name string)
error {
return nil
}
-func (b *MetadataBuilder) SetUUID(uuid uuid.UUID) error {
- if b.uuid == uuid {
+func (b *MetadataBuilder) SetUUID(newUUID uuid.UUID) error {
+ if newUUID == uuid.Nil {
+ return errors.New("cannot set uuid to null")
+ }
+
+ if b.uuid == newUUID {
Review Comment:
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?
##########
table/metadata.go:
##########
@@ -902,13 +902,17 @@ func (b *MetadataBuilder) RemoveSnapshotRef(name string)
error {
return nil
}
-func (b *MetadataBuilder) SetUUID(uuid uuid.UUID) error {
Review Comment:
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.
--
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]
