laskoviymishka commented on code in PR #1592:
URL: https://github.com/apache/iceberg-go/pull/1592#discussion_r3680745795
##########
table/metadata.go:
##########
@@ -2277,6 +2281,18 @@ func (c *commonMetadata) validate() error {
return nil
}
+func (c *commonMetadata) checkSnapshots() error {
Review Comment:
one thing worth calling out: this makes us stricter than the other clients
on the read path. PyIceberg and iceberg-rust both silently accept duplicate
snapshot IDs (last-one-wins), and Java only rejects at write time in the
builder — its parser doesn't check. So after this, iceberg-go will refuse to
open a metadata file that py/rust will happily read.
I actually think rejecting is the better behavior — it stops the
order-dependent `SnapshotByID()` ambiguity from ever entering the system, which
is the whole point of the fix. But it's a deliberate divergence, and it changes
the observable behavior of `ParseMetadata` for callers who previously tolerated
these files, so I'd note it in the PR description / changelog so it's a
conscious choice rather than a surprise.
If we'd rather stay lenient-on-read like the others, the alternative is to
log a warning here and keep the hard error in the builder's
`addSnapshotInternal`. I lean toward keeping the hard reject — just want it to
be intentional. wdyt?
##########
table/metadata_internal_test.go:
##########
@@ -564,6 +564,44 @@ func TestRejectInvalidSchemaEntries(t *testing.T) {
})
}
+func TestRejectDuplicateSnapshotIDs(t *testing.T) {
+ var metadata map[string]any
+ decoder := json.NewDecoder(strings.NewReader(ExampleTableMetadataV2))
+ decoder.UseNumber()
+ require.NoError(t, decoder.Decode(&metadata))
+ snapshots := metadata["snapshots"].([]any)
+ current := snapshots[len(snapshots)-1].(map[string]any)
+
+ tests := []struct {
+ name string
+ duplicate map[string]any
+ }{
+ {name: "identical snapshot", duplicate: maps.Clone(current)},
+ {
+ name: "different manifest list",
Review Comment:
a one-line comment here would help — at a glance this case looks identical
to the first, and the point (that ID equality alone drives rejection,
regardless of `manifest-list` or other fields) is worth stating so nobody
deletes it later as a dup.
while we're here, the IIFE is a bit opaque — the sibling
`TestRejectInvalidSchemaEntries` builds per-case state inside the `t.Run` body.
Building the duplicate there instead would read more naturally, but that's
taste.
##########
table/metadata_internal_test.go:
##########
@@ -564,6 +564,44 @@ func TestRejectInvalidSchemaEntries(t *testing.T) {
})
}
+func TestRejectDuplicateSnapshotIDs(t *testing.T) {
+ var metadata map[string]any
+ decoder := json.NewDecoder(strings.NewReader(ExampleTableMetadataV2))
+ decoder.UseNumber()
+ require.NoError(t, decoder.Decode(&metadata))
+ snapshots := metadata["snapshots"].([]any)
+ current := snapshots[len(snapshots)-1].(map[string]any)
+
+ tests := []struct {
+ name string
+ duplicate map[string]any
+ }{
+ {name: "identical snapshot", duplicate: maps.Clone(current)},
+ {
+ name: "different manifest list",
+ duplicate: func() map[string]any {
+ duplicate := maps.Clone(current)
+ duplicate["manifest-list"] =
"s3://bucket/metadata/different.avro"
+
+ return duplicate
+ }(),
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ invalid := maps.Clone(metadata)
+ invalid["snapshots"] = append(slices.Clone(snapshots),
tt.duplicate)
+ data, err := json.Marshal(invalid)
+ require.NoError(t, err)
+
+ _, err = ParseMetadataBytes(data)
+ require.ErrorIs(t, err, ErrInvalidMetadata)
+ assert.ErrorContains(t, err, "duplicate snapshot ID
3055729675574597004")
Review Comment:
this hard-codes the current snapshot's ID from `ExampleTableMetadataV2`. If
that fixture ever changes, the assertion goes stale silently — a false negative
instead of a compile error, and unlike the schema-ID sibling test where `0` is
structurally fixed, these IDs are arbitrary large ints.
since you're already decoding with `UseNumber()` (good call — these IDs are
> 2^53 and plain `Unmarshal` would corrupt them as float64), you can pull the
expected value straight from the fixture:
```go
assert.ErrorContains(t, err, fmt.Sprintf("duplicate snapshot ID %v",
current["snapshot-id"]))
```
`current["snapshot-id"]` is a `json.Number`, so `%v` prints the raw integer
with no precision loss.
##########
table/metadata_internal_test.go:
##########
@@ -564,6 +564,44 @@ func TestRejectInvalidSchemaEntries(t *testing.T) {
})
}
+func TestRejectDuplicateSnapshotIDs(t *testing.T) {
+ var metadata map[string]any
+ decoder := json.NewDecoder(strings.NewReader(ExampleTableMetadataV2))
Review Comment:
the check lives in `commonMetadata.validate()` so it covers all three
versions, but the test only exercises V2. V3's `UnmarshalJSON` is structurally
different — it runs `checkNextRowID` and `checkLastSequenceNumber` before
`validate()` — so a regression in that wrapper that skipped `validate()`
wouldn't be caught here.
could we add a `t.Run` with `ExampleTableMetadataV3` injecting a duplicate?
V1 would be nice too, but V3 is the one under active development.
--
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]
