You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/07/28 23:39:14 UTC
[PR] fix(table): reject duplicate snapshot IDs [iceberg-go]
fallintoplace opened a new pull request, #1592:
URL: https://github.com/apache/iceberg-go/pull/1592
## What changed
Validate snapshot ID uniqueness before constructing or checking snapshot references. Add coverage for identical duplicate snapshots and duplicate IDs carrying different manifest lists.
## Why
Snapshot lookup returns the first matching entry. Accepting duplicate IDs made current-snapshot and reference resolution depend on JSON array order.
## Testing
- `go test ./table -run 'Test(RejectDuplicateSnapshotIDs|MetadataV[123]Parsing)$' -count=1`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): reject duplicate snapshot IDs [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): reject duplicate snapshot IDs [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1592:
URL: https://github.com/apache/iceberg-go/pull/1592
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org