laskoviymishka commented on code in PR #2186:
URL: https://github.com/apache/iceberg-go/pull/2186#discussion_r4260907316
##########
table/metadata.go:
##########
@@ -515,11 +517,14 @@ func MetadataBuilderFromBase(metadata Metadata,
currentFileLocation string) (*Me
if b.props == nil {
b.props = iceberg.Properties{}
}
- snapshots, err := common.snapshotsForMarshal()
- if err != nil {
- return nil, err
+ b.snapshotList = cloneSnapshots(common.SnapshotList)
Review Comment:
This breaks expiry for deferred-loaded tables. `b.snapshotList` used to hold
the full history and now holds only the ref-reachable snapshots plus additions.
`Transaction.ExpireSnapshots` still finds its candidates by ranging over
`meta.snapshotList`, in both the unreferenced-cutoff loop and the
`snapsToDelete` loop. So after a commit in `SnapshotModeRefs`, the next
transaction's expire never sees old unreferenced snapshots and removes nothing
for them, with no error.
Expire needs the whole history anyway, so I'd add a builder helper that
materializes on purpose and use it in both loops. Then add a transaction-level
test: parse deferred metadata with an old unreferenced snapshot, call
`ExpireSnapshots`, and assert the ID lands in the `RemoveSnapshotsUpdate`.
##########
table/deferred_snapshots.go:
##########
@@ -41,6 +43,69 @@ type deferredSnapshotState struct {
err error
}
+// deferredSnapshotOverlay records mutations over an immutable deferred
+// snapshot state. Builders own a mutable overlay and clone it when producing
+// metadata so built metadata cannot observe subsequent builder changes.
+type deferredSnapshotOverlay struct {
+ additions []Snapshot
+ removed map[int64]struct{}
+}
+
+func (o *deferredSnapshotOverlay) clone() *deferredSnapshotOverlay {
+ if o == nil {
+ return nil
+ }
+
+ return &deferredSnapshotOverlay{
+ additions: cloneSnapshots(o.additions),
+ removed: maps.Clone(o.removed),
+ }
+}
+
+func (o *deferredSnapshotOverlay) isRemoved(id int64) bool {
+ if o == nil {
+ return false
+ }
+
+ _, ok := o.removed[id]
+
+ return ok
+}
+
+func (o *deferredSnapshotOverlay) add(snapshot Snapshot) {
+ o.additions = append(o.additions, snapshot)
+}
+
+func (o *deferredSnapshotOverlay) remove(ids map[int64]struct{}) {
+ if o.removed == nil {
+ o.removed = make(map[int64]struct{}, len(ids))
+ }
+ for id := range ids {
+ o.removed[id] = struct{}{}
Review Comment:
I'd only record an ID here when `deferredSnapshots.containsID(id)` is true.
Right now every requested ID goes in, including ones that never existed and
ones that were only builder additions. Those entries get carried through every
`clone()` and `Build`. A no-op remove also makes `removed` non-empty, so
`apply` loses its fast path for no reason.
##########
table/metadata.go:
##########
@@ -973,6 +990,9 @@ func (b *MetadataBuilder) addSnapshotInternal(snapshot
*Snapshot, preserveUpdate
b.snapshotList = append(b.snapshotList, *snapshot)
b.snapshotIndex.positions[snapshot.SnapshotID] = len(b.snapshotList) - 1
b.snapshotIndex.firstSnapshot = snapshotListFirst(b.snapshotList)
+ if b.snapshotOverlay != nil {
+ b.snapshotOverlay.add(*snapshot)
Review Comment:
An added snapshot now lives in `snapshotList`, `snapshotIndex` and
`overlay.additions`, and every mutator has to keep all three in sync by hand.
Any future path that writes `snapshotList` directly would drop or duplicate
snapshots at marshal time without failing a test.
I'd derive `additions` at `Build` time instead: the `snapshotList` entries
that aren't live base snapshots, meaning not in the deferred index, or present
in `removed` (the re-add case). That removes the second copy and the nil guards
around `add`/`remove` at the same time. If that turns out awkward, at minimum
add a comment on the fields stating the invariant.
##########
table/deferred_snapshots.go:
##########
@@ -41,6 +43,69 @@ type deferredSnapshotState struct {
err error
}
+// deferredSnapshotOverlay records mutations over an immutable deferred
+// snapshot state. Builders own a mutable overlay and clone it when producing
+// metadata so built metadata cannot observe subsequent builder changes.
+type deferredSnapshotOverlay struct {
+ additions []Snapshot
+ removed map[int64]struct{}
+}
+
+func (o *deferredSnapshotOverlay) clone() *deferredSnapshotOverlay {
+ if o == nil {
+ return nil
+ }
+
+ return &deferredSnapshotOverlay{
+ additions: cloneSnapshots(o.additions),
+ removed: maps.Clone(o.removed),
+ }
+}
+
+func (o *deferredSnapshotOverlay) isRemoved(id int64) bool {
+ if o == nil {
+ return false
+ }
+
+ _, ok := o.removed[id]
+
+ return ok
+}
+
+func (o *deferredSnapshotOverlay) add(snapshot Snapshot) {
+ o.additions = append(o.additions, snapshot)
+}
+
+func (o *deferredSnapshotOverlay) remove(ids map[int64]struct{}) {
+ if o.removed == nil {
+ o.removed = make(map[int64]struct{}, len(ids))
+ }
+ for id := range ids {
+ o.removed[id] = struct{}{}
+ }
+ o.additions = slices.DeleteFunc(o.additions, func(snapshot Snapshot)
bool {
+ _, ok := ids[snapshot.SnapshotID]
+
+ return ok
+ })
+}
+
+func (o *deferredSnapshotOverlay) apply(base []Snapshot) []Snapshot {
+ if o == nil || (len(o.additions) == 0 && len(o.removed) == 0) {
+ return base
+ }
+
+ snapshots := make([]Snapshot, 0, len(base)+len(o.additions))
Review Comment:
Once the overlay has anything in it, every `Snapshots()`, `Equals` and
marshal on the built metadata filters and reallocates the full history again.
That covers any metadata built from a mutated deferred builder, staged tables
inside a transaction for example. Built metadata is immutable, so I'd cache the
applied slice next to the overlay with a `sync.Once`.
##########
table/deferred_snapshots_test.go:
##########
@@ -131,6 +131,210 @@ func
TestDeferredSnapshotsMaterializeForCollectionAndSerialization(t *testing.T)
assert.Equal(t, eagerRebuilt.Snapshots(), rebuilt.Snapshots())
}
+func TestMetadataBuilderFromDeferredBaseRemainsLazyThroughBuild(t *testing.T) {
+ data := metadataWithUnreferencedSnapshot(t)
+ meta, err := ParseMetadataBytesDeferredSnapshots(data)
+ require.NoError(t, err)
+ state := commonMetadataOf(meta).deferredSnapshots
+ require.NotNil(t, state)
+ require.NotEmpty(t, state.raw)
+
+ builder, err := MetadataBuilderFromBase(meta, "")
+ require.NoError(t, err)
+ assert.NotEmpty(t, state.raw, "builder construction must not
materialize deferred history")
+
+ rebuilt, err := builder.Build()
+ require.NoError(t, err)
+ assert.NotEmpty(t, state.raw, "building unchanged metadata must remain
lazy")
+ assert.Equal(t, meta.CurrentSnapshot(), rebuilt.CurrentSnapshot())
+ assert.NotEmpty(t, state.raw, "referenced snapshot access must remain
eager")
+
+ assert.Len(t, rebuilt.Snapshots(), 2)
+ assert.Nil(t, state.raw, "complete collection access must materialize
history")
+}
+
+func TestDeferredBuilderAddsSnapshotWithoutMaterializingHistory(t *testing.T) {
+ data := metadataWithUnreferencedSnapshot(t)
+ meta, err := ParseMetadataBytesDeferredSnapshots(data)
+ require.NoError(t, err)
+ state := commonMetadataOf(meta).deferredSnapshots
+ require.NotNil(t, state)
+
+ builder, err := MetadataBuilderFromBase(meta, "")
+ require.NoError(t, err)
+ current := meta.CurrentSnapshot()
+ require.NotNil(t, current)
+ added := *current
+ added.SnapshotID = 987654321
+ added.ParentSnapshotID = ¤t.SnapshotID
+ added.SequenceNumber = meta.LastSequenceNumber() + 1
+ added.TimestampMs = max(meta.LastUpdatedMillis(), current.TimestampMs)
+ 1
+ require.NoError(t, builder.AddSnapshot(&added))
+ require.NoError(t, builder.SetSnapshotRef(MainBranch, added.SnapshotID,
BranchRef))
+ assert.NotEmpty(t, state.raw, "snapshot addition and ref update must
remain lazy")
+
+ rebuilt, err := builder.Build()
+ require.NoError(t, err)
+ assert.NotEmpty(t, state.raw, "building metadata with an addition must
remain lazy")
+ assert.Equal(t, &added, rebuilt.SnapshotByID(added.SnapshotID))
+ assert.NotEmpty(t, state.raw)
+
+ serialized, err := json.Marshal(rebuilt)
+ require.NoError(t, err)
+ reparsed, err := ParseMetadataBytes(serialized)
+ require.NoError(t, err)
+ assert.Len(t, reparsed.Snapshots(), 3)
+ assert.Equal(t, &added, reparsed.SnapshotByID(added.SnapshotID))
+}
+
+func TestDeferredBuilderRemovesSnapshotWithoutResurrection(t *testing.T) {
+ data := metadataWithUnreferencedSnapshot(t)
+ eager, err := ParseMetadataBytes(data)
+ require.NoError(t, err)
+ require.Len(t, eager.Snapshots(), 2)
+ historicalID := eager.Snapshots()[0].SnapshotID
+
+ meta, err := ParseMetadataBytesDeferredSnapshots(data)
+ require.NoError(t, err)
+ state := commonMetadataOf(meta).deferredSnapshots
+ require.NotNil(t, state)
+ builder, err := MetadataBuilderFromBase(meta, "")
+ require.NoError(t, err)
+ require.NoError(t, builder.RemoveSnapshots([]int64{historicalID},
false))
+ assert.NotEmpty(t, state.raw, "snapshot removal must use the deferred
ID index")
+
+ rebuilt, err := builder.Build()
+ require.NoError(t, err)
+ assert.NotEmpty(t, state.raw, "building metadata with a removal must
remain lazy")
+ assert.Nil(t, rebuilt.SnapshotByID(historicalID))
+ assert.NotEmpty(t, state.raw, "removed lookup must not materialize
history")
+
+ serialized, err := json.Marshal(rebuilt)
+ require.NoError(t, err)
+ reparsed, err := ParseMetadataBytes(serialized)
+ require.NoError(t, err)
+ assert.Len(t, reparsed.Snapshots(), 1)
+ assert.Nil(t, reparsed.SnapshotByID(historicalID))
+}
+
+func TestDeferredBuilderCanReplaceRemovedSnapshotID(t *testing.T) {
+ data := metadataWithUnreferencedSnapshot(t)
+ meta, err := ParseMetadataBytesDeferredSnapshots(data)
+ require.NoError(t, err)
+ historical := meta.SnapshotByID(3051729675574597004)
+ require.NotNil(t, historical)
+ state := commonMetadataOf(meta).deferredSnapshots
+ require.NotNil(t, state)
+
+ builder, err := MetadataBuilderFromBase(meta, "")
+ require.NoError(t, err)
+ require.NoError(t,
builder.RemoveSnapshots([]int64{historical.SnapshotID}, false))
+ replacement := *historical
+ replacement.ManifestList = "replacement.avro"
+ current := meta.CurrentSnapshot()
+ require.NotNil(t, current)
+ replacement.ParentSnapshotID = ¤t.SnapshotID
+ replacement.SequenceNumber = meta.LastSequenceNumber() + 1
+ replacement.TimestampMs = meta.LastUpdatedMillis() + 1
+ require.NoError(t, builder.AddSnapshot(&replacement))
+ assert.NotEmpty(t, state.raw)
+
+ rebuilt, err := builder.Build()
+ require.NoError(t, err)
+ assert.Equal(t, "replacement.avro",
rebuilt.SnapshotByID(historical.SnapshotID).ManifestList)
+ serialized, err := json.Marshal(rebuilt)
+ require.NoError(t, err)
+ reparsed, err := ParseMetadataBytes(serialized)
+ require.NoError(t, err)
+ require.Len(t, reparsed.Snapshots(), 2)
+ assert.Equal(t, "replacement.avro",
reparsed.SnapshotByID(historical.SnapshotID).ManifestList)
+}
+
+func TestDeferredBuilderCloneOwnsSnapshotOverlay(t *testing.T) {
+ data := metadataWithUnreferencedSnapshot(t)
+ eager, err := ParseMetadataBytes(data)
+ require.NoError(t, err)
+ historicalID := eager.Snapshots()[0].SnapshotID
+
+ meta, err := ParseMetadataBytesDeferredSnapshots(data)
+ require.NoError(t, err)
+ builder, err := MetadataBuilderFromBase(meta, "")
+ require.NoError(t, err)
+ cloned := builder.clone()
+ require.NoError(t, cloned.RemoveSnapshots([]int64{historicalID}, false))
+
+ originalBuilt, err := builder.Build()
+ require.NoError(t, err)
+ cloneBuilt, err := cloned.Build()
+ require.NoError(t, err)
+ assert.NotNil(t, originalBuilt.SnapshotByID(historicalID))
+ assert.Nil(t, cloneBuilt.SnapshotByID(historicalID))
+ assert.Len(t, originalBuilt.Snapshots(), 2)
+ assert.Len(t, cloneBuilt.Snapshots(), 1)
+}
+
+func TestDeferredBuiltMetadataOwnsSnapshotOverlay(t *testing.T) {
+ meta, err :=
ParseMetadataBytesDeferredSnapshots(metadataWithUnreferencedSnapshot(t))
+ require.NoError(t, err)
+ builder, err := MetadataBuilderFromBase(meta, "")
+ require.NoError(t, err)
+ current := meta.CurrentSnapshot()
+ require.NotNil(t, current)
+ added := *current
+ added.SnapshotID = 987654321
+ added.ParentSnapshotID = ¤t.SnapshotID
+ added.SequenceNumber = meta.LastSequenceNumber() + 1
+ added.TimestampMs = max(meta.LastUpdatedMillis(), current.TimestampMs)
+ 1
+ require.NoError(t, builder.AddSnapshot(&added))
+
+ built, err := builder.Build()
+ require.NoError(t, err)
+ require.NoError(t, builder.RemoveSnapshots([]int64{added.SnapshotID},
false))
+ assert.NotNil(t, built.SnapshotByID(added.SnapshotID))
+ assert.Len(t, built.Snapshots(), 3)
+ _, err = builder.SnapshotByID(added.SnapshotID)
+ assert.ErrorIs(t, err, ErrSnapshotNotFound)
+}
+
+func TestDeferredBuilderRoundTripAllMetadataVersions(t *testing.T) {
+ for _, tc := range []struct {
+ name string
+ data string
+ }{
+ {name: "v1", data: ExampleTableMetadataV1},
+ {name: "v2", data: ExampleTableMetadataV2},
+ {name: "v3", data: ExampleTableMetadataV3},
+ } {
+ t.Run(tc.name, func(t *testing.T) {
+ var fields map[string]json.RawMessage
+ require.NoError(t, json.Unmarshal([]byte(tc.data),
&fields))
+ fields["refs"] = json.RawMessage(`{}`)
+ data, err := json.Marshal(fields)
+ require.NoError(t, err)
+
+ eager, err := ParseMetadataBytes(data)
+ require.NoError(t, err)
+ eagerBuilder, err := MetadataBuilderFromBase(eager, "")
+ require.NoError(t, err)
+ deferred, err :=
ParseMetadataBytesDeferredSnapshots(data)
+ require.NoError(t, err)
+ state := commonMetadataOf(deferred).deferredSnapshots
+ require.NotNil(t, state)
+ builder, err := MetadataBuilderFromBase(deferred, "")
+ require.NoError(t, err)
+ const builtTimestamp = int64(1800000000000)
+ eagerBuilder.lastUpdatedMS = builtTimestamp
+ builder.lastUpdatedMS = builtTimestamp
+ eagerRebuilt, err := eagerBuilder.Build()
+ require.NoError(t, err)
+ rebuilt, err := builder.Build()
+ require.NoError(t, err)
+ assert.NotEmpty(t, state.raw)
+ assert.True(t, eagerRebuilt.Equals(rebuilt))
Review Comment:
This round-trip only covers the no-op rebuild, which is the one case where
the overlay is empty. I'd add a mutate case. Add a snapshot and remove a
non-eager base ID, `Build`, then `MetadataBuilderFromBase(built)`, mutate again
and `Build`. Then compare `Equals` and the marshaled bytes against the eager
path. That exercises the overlay carrying over through
`common.snapshotOverlay.clone()`, which I don't see covered today.
Two `SetSnapshotRef` cases would fit here too: a removed ID should error,
and an unreferenced base snapshot should succeed while `raw` stays non-empty.
##########
table/deferred_snapshots_test.go:
##########
@@ -131,6 +131,210 @@ func
TestDeferredSnapshotsMaterializeForCollectionAndSerialization(t *testing.T)
assert.Equal(t, eagerRebuilt.Snapshots(), rebuilt.Snapshots())
}
+func TestMetadataBuilderFromDeferredBaseRemainsLazyThroughBuild(t *testing.T) {
+ data := metadataWithUnreferencedSnapshot(t)
+ meta, err := ParseMetadataBytesDeferredSnapshots(data)
+ require.NoError(t, err)
+ state := commonMetadataOf(meta).deferredSnapshots
+ require.NotNil(t, state)
+ require.NotEmpty(t, state.raw)
+
+ builder, err := MetadataBuilderFromBase(meta, "")
+ require.NoError(t, err)
+ assert.NotEmpty(t, state.raw, "builder construction must not
materialize deferred history")
Review Comment:
These assertions read `state.raw` without holding `state.mu`. That's fine
single-threaded, but it will trip `-race` as soon as a test adds concurrent
access. A small `isMaterialized()` helper that takes `RLock` would work for
every assertion in this file.
--
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]
