laskoviymishka commented on code in PR #1382:
URL: https://github.com/apache/iceberg-go/pull/1382#discussion_r3570254279
##########
table/table.go:
##########
@@ -531,7 +533,7 @@ func (t Table) doCommit(ctx context.Context, updates
[]Update, reqs []Requiremen
return nil, context.Cause(retryCtx)
}
- newMeta, newLoc, err = t.cat.CommitTable(retryCtx,
t.identifier, reqs, updates)
+ newMeta, newLoc, err = t.cat.CommitTable(retryCtx,
slices.Clone(t.identifier), reqs, updates)
Review Comment:
The straight-commit path here is covered nicely by
`TestDoCommitPassesIdentifierCopy` — it mutates `cat.commitIdentifier`
post-commit and confirms `tbl.Identifier()` holds. The one case we don't
exercise is an actual retry: the fake catalog always succeeds, so the loop
never re-enters and the clone on the retry attempt goes unchecked.
Not blocking — but if the fake rejected once before succeeding, we'd close
that gap too. wdyt?
##########
table/table.go:
##########
@@ -103,7 +103,9 @@ func (t Table) Equals(other Table) bool {
t.metadata.Equals(other.metadata)
}
-func (t Table) Identifier() Identifier { return
t.identifier }
+func (t Table) Identifier() Identifier {
Review Comment:
Tiny thing — the siblings right below (`Metadata()`, `MetadataLocation()`,
`FS()`) are all aligned one-liners, so the block body stands out. I'd keep it
on one line:
```go
func (t Table) Identifier() Identifier { return slices.Clone(t.identifier) }
```
##########
table/commit_test.go:
##########
@@ -123,7 +124,10 @@ func TestTableCommitReturnsCopies(t *testing.T) {
if len(tc1.Requirements) > 0 {
tc1.Requirements = tc1.Requirements[:0]
}
+ tc1.Identifier[0] = "different_db"
assert.NotEmpty(t, tc2.Requirements)
+ assert.Equal(t, original, tbl.Identifier())
+ assert.Equal(t, original, tc2.Identifier)
Review Comment:
Nice — this locks down the non-empty-updates return. The zero-updates branch
of `TableCommit()` got the same `slices.Clone` but isn't exercised here.
Totally optional, but a second case that commits with no updates and mutates
the returned `Identifier` would cover it symmetrically.
--
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]
