laskoviymishka commented on code in PR #1601:
URL: https://github.com/apache/iceberg-go/pull/1601#discussion_r3677676891
##########
table/metadata.go:
##########
@@ -908,6 +908,23 @@ func (b *MetadataBuilder) SetSnapshotRef(
return fmt.Errorf("%w: invalid snapshot ref option:
%w", iceberg.ErrInvalidArgument, err)
}
}
+
+ // preserve retention properties from an existing ref when not
explicitly overridden by caller
+ if existingRef, ok := b.refs[name]; ok && existingRef.SnapshotRefType
== refType {
Review Comment:
I'd think about moving this inheritance up to the call sites rather than
baking it into `SetSnapshotRef`.
Java keeps `setRef` a pure replace — it stores the `SnapshotRef` as-is — and
does the retention carry-over one level up in `setBranchSnapshotInternal` via
`SnapshotRef.builderFrom(ref, newSnapshotId)`. Doing it inside `SetSnapshotRef`
also changes `setSnapshotRefUpdate.Apply()`: a set-snapshot-ref update that
arrives with absent retention (which Java treats as "clear") now silently
inherits whatever the current ref holds. For the embedded/Hadoop catalog replay
path that's a real semantic divergence, and it's the same root reason there's
now no way to clear a retention value once set — no options means inherit, and
the `WithXxx` validators reject zero.
No current Go caller passes non-zero retention on an update-in-place, so
this is latent design debt rather than a live bug. But if
`snapshot_producers.go` and `transaction.go` read the existing ref's retention
and pass it explicitly into `NewSetSnapshotRefUpdate`, the emitted update is
fully specified, replay stays a pure replace, and clearing becomes
representable again. wdyt?
##########
table/metadata_builder_internal_test.go:
##########
@@ -482,6 +482,120 @@ func TestSetRef(t *testing.T) {
require.Len(t, builder.snapshotLog, 1)
}
+func TestSetSnapshotRefPreservesRetentionOnUpdate(t *testing.T) {
+ builder := builderWithoutChanges(2)
+ schemaID := 0
+ snapshot1 := Snapshot{
+ SnapshotID: 1,
+ ParentSnapshotID: nil,
+ SequenceNumber: 0,
+ TimestampMs: builder.base.LastUpdatedMillis() + 1,
+ ManifestList: "/snap-1.avro",
+ Summary: &Summary{Operation: OpAppend},
+ SchemaID: &schemaID,
+ }
+ parentID := int64(1)
+ snapshot2 := Snapshot{
+ SnapshotID: 2,
+ ParentSnapshotID: &parentID,
+ SequenceNumber: 1,
+ TimestampMs: builder.base.LastUpdatedMillis() + 2,
+ ManifestList: "/snap-2.avro",
+ Summary: &Summary{Operation: OpAppend},
+ SchemaID: &schemaID,
+ }
+
+ const (
+ minKeep = 5
+ maxSnapAgeMs = int64(172800000) // 2 days
+ maxRefAgeMsIn = int64(604800000) // 7 days
+ )
+
+ // Configure retention policy on the branch when it is first created.
+ require.NoError(t, builder.AddSnapshot(&snapshot1))
+ require.NoError(t, builder.SetSnapshotRef(
+ MainBranch, 1, BranchRef,
+ WithMinSnapshotsToKeep(minKeep),
+ WithMaxSnapshotAgeMs(maxSnapAgeMs),
+ WithMaxRefAgeMs(maxRefAgeMsIn),
+ ))
+
+ ref := builder.refs[MainBranch]
+ require.NotNil(t, ref.MinSnapshotsToKeep)
+ require.Equal(t, minKeep, *ref.MinSnapshotsToKeep)
+ require.NotNil(t, ref.MaxSnapshotAgeMs)
+ require.Equal(t, maxSnapAgeMs, *ref.MaxSnapshotAgeMs)
+ require.NotNil(t, ref.MaxRefAgeMs)
+ require.Equal(t, maxRefAgeMsIn, *ref.MaxRefAgeMs)
+
+ // Simulate a normal commit/rollback: only the snapshot pointer is
updated,
+ // with no retention options. Retention must be preserved.
+ require.NoError(t, builder.AddSnapshot(&snapshot2))
+ require.NoError(t, builder.SetSnapshotRef(MainBranch, 2, BranchRef))
+
+ ref = builder.refs[MainBranch]
+ require.Equal(t, int64(2), ref.SnapshotID, "snapshot pointer should
advance")
+ require.NotNil(t, ref.MinSnapshotsToKeep, "min-snapshots-to-keep must
not be wiped out on commit")
+ require.Equal(t, minKeep, *ref.MinSnapshotsToKeep)
+ require.NotNil(t, ref.MaxSnapshotAgeMs, "max-snapshot-age-ms must not
be wiped out on commit")
+ require.Equal(t, maxSnapAgeMs, *ref.MaxSnapshotAgeMs)
+ require.NotNil(t, ref.MaxRefAgeMs, "max-ref-age-ms must not be wiped
out on commit")
+ require.Equal(t, maxRefAgeMsIn, *ref.MaxRefAgeMs)
+
+ // The emitted metadata update must also carry the preserved retention
so
+ // REST catalogs receive the correct values.
+ last := builder.updates[len(builder.updates)-1].(*setSnapshotRefUpdate)
+ require.Equal(t, minKeep, last.MinSnapshotsToKeep)
+ require.Equal(t, maxSnapAgeMs, last.MaxSnapshotAgeMs)
+ require.Equal(t, maxRefAgeMsIn, last.MaxRefAgeMs)
+}
+
+func TestSetSnapshotRefOverridesRetentionWhenProvided(t *testing.T) {
+ builder := builderWithoutChanges(2)
+ schemaID := 0
+ snapshot := Snapshot{
+ SnapshotID: 1,
+ ParentSnapshotID: nil,
+ SequenceNumber: 0,
+ TimestampMs: builder.base.LastUpdatedMillis() + 1,
+ ManifestList: "/snap-1.avro",
+ Summary: &Summary{Operation: OpAppend},
+ SchemaID: &schemaID,
+ }
+ require.NoError(t, builder.AddSnapshot(&snapshot))
+ require.NoError(t, builder.SetSnapshotRef(MainBranch, 1, BranchRef,
WithMinSnapshotsToKeep(5)))
+
+ // An explicitly provided option overrides the inherited value.
+ require.NoError(t, builder.SetSnapshotRef(MainBranch, 1, BranchRef,
WithMinSnapshotsToKeep(10)))
Review Comment:
This only proves that overriding the one field that was set works — it
doesn't prove the invariant that matters most.
I'd set all three retention fields in the first call, override just
`MinSnapshotsToKeep` in the second, and assert `MaxSnapshotAgeMs` and
`MaxRefAgeMs` are still carried over. That's the "only fill when nil" contract;
right now nothing fails if a future change accidentally drops the un-overridden
fields.
##########
table/metadata_builder_internal_test.go:
##########
@@ -482,6 +482,120 @@ func TestSetRef(t *testing.T) {
require.Len(t, builder.snapshotLog, 1)
}
+func TestSetSnapshotRefPreservesRetentionOnUpdate(t *testing.T) {
+ builder := builderWithoutChanges(2)
+ schemaID := 0
+ snapshot1 := Snapshot{
+ SnapshotID: 1,
+ ParentSnapshotID: nil,
+ SequenceNumber: 0,
+ TimestampMs: builder.base.LastUpdatedMillis() + 1,
+ ManifestList: "/snap-1.avro",
+ Summary: &Summary{Operation: OpAppend},
+ SchemaID: &schemaID,
+ }
+ parentID := int64(1)
+ snapshot2 := Snapshot{
+ SnapshotID: 2,
+ ParentSnapshotID: &parentID,
+ SequenceNumber: 1,
+ TimestampMs: builder.base.LastUpdatedMillis() + 2,
+ ManifestList: "/snap-2.avro",
+ Summary: &Summary{Operation: OpAppend},
+ SchemaID: &schemaID,
+ }
+
+ const (
+ minKeep = 5
+ maxSnapAgeMs = int64(172800000) // 2 days
+ maxRefAgeMsIn = int64(604800000) // 7 days
+ )
+
+ // Configure retention policy on the branch when it is first created.
+ require.NoError(t, builder.AddSnapshot(&snapshot1))
+ require.NoError(t, builder.SetSnapshotRef(
+ MainBranch, 1, BranchRef,
+ WithMinSnapshotsToKeep(minKeep),
+ WithMaxSnapshotAgeMs(maxSnapAgeMs),
+ WithMaxRefAgeMs(maxRefAgeMsIn),
+ ))
+
+ ref := builder.refs[MainBranch]
+ require.NotNil(t, ref.MinSnapshotsToKeep)
+ require.Equal(t, minKeep, *ref.MinSnapshotsToKeep)
+ require.NotNil(t, ref.MaxSnapshotAgeMs)
+ require.Equal(t, maxSnapAgeMs, *ref.MaxSnapshotAgeMs)
+ require.NotNil(t, ref.MaxRefAgeMs)
+ require.Equal(t, maxRefAgeMsIn, *ref.MaxRefAgeMs)
+
+ // Simulate a normal commit/rollback: only the snapshot pointer is
updated,
+ // with no retention options. Retention must be preserved.
+ require.NoError(t, builder.AddSnapshot(&snapshot2))
+ require.NoError(t, builder.SetSnapshotRef(MainBranch, 2, BranchRef))
Review Comment:
This test never exercises the path the bug actually lives on.
The regression happens because `snapshot_producers.go` and `transaction.go`
build `NewSetSnapshotRefUpdate(branch, snap, BranchRef, 0, 0, 0)` and let
`.Apply()` call `SetSnapshotRef` with no options — calling
`builder.SetSnapshotRef` directly skips that translation entirely. If someone
later changes how `Apply()` maps those zeros, this test stays green while the
bug comes back.
I'd drive it through the real path: construct a
`setSnapshotRefUpdate{MaxRefAgeMs: 0, ...}` on a builder whose `refs["main"]`
already has retention, call `.Apply(builder)`, and assert it's preserved — plus
a `RollbackToSnapshot` case for the rollback caller. That's what actually locks
the contract.
##########
table/metadata_builder_internal_test.go:
##########
@@ -482,6 +482,120 @@ func TestSetRef(t *testing.T) {
require.Len(t, builder.snapshotLog, 1)
}
+func TestSetSnapshotRefPreservesRetentionOnUpdate(t *testing.T) {
+ builder := builderWithoutChanges(2)
+ schemaID := 0
+ snapshot1 := Snapshot{
+ SnapshotID: 1,
+ ParentSnapshotID: nil,
+ SequenceNumber: 0,
+ TimestampMs: builder.base.LastUpdatedMillis() + 1,
+ ManifestList: "/snap-1.avro",
+ Summary: &Summary{Operation: OpAppend},
+ SchemaID: &schemaID,
+ }
+ parentID := int64(1)
+ snapshot2 := Snapshot{
+ SnapshotID: 2,
+ ParentSnapshotID: &parentID,
+ SequenceNumber: 1,
+ TimestampMs: builder.base.LastUpdatedMillis() + 2,
+ ManifestList: "/snap-2.avro",
+ Summary: &Summary{Operation: OpAppend},
+ SchemaID: &schemaID,
+ }
+
+ const (
+ minKeep = 5
+ maxSnapAgeMs = int64(172800000) // 2 days
+ maxRefAgeMsIn = int64(604800000) // 7 days
+ )
+
+ // Configure retention policy on the branch when it is first created.
+ require.NoError(t, builder.AddSnapshot(&snapshot1))
+ require.NoError(t, builder.SetSnapshotRef(
+ MainBranch, 1, BranchRef,
+ WithMinSnapshotsToKeep(minKeep),
+ WithMaxSnapshotAgeMs(maxSnapAgeMs),
+ WithMaxRefAgeMs(maxRefAgeMsIn),
+ ))
+
+ ref := builder.refs[MainBranch]
+ require.NotNil(t, ref.MinSnapshotsToKeep)
+ require.Equal(t, minKeep, *ref.MinSnapshotsToKeep)
+ require.NotNil(t, ref.MaxSnapshotAgeMs)
+ require.Equal(t, maxSnapAgeMs, *ref.MaxSnapshotAgeMs)
+ require.NotNil(t, ref.MaxRefAgeMs)
+ require.Equal(t, maxRefAgeMsIn, *ref.MaxRefAgeMs)
+
+ // Simulate a normal commit/rollback: only the snapshot pointer is
updated,
+ // with no retention options. Retention must be preserved.
+ require.NoError(t, builder.AddSnapshot(&snapshot2))
+ require.NoError(t, builder.SetSnapshotRef(MainBranch, 2, BranchRef))
+
+ ref = builder.refs[MainBranch]
+ require.Equal(t, int64(2), ref.SnapshotID, "snapshot pointer should
advance")
+ require.NotNil(t, ref.MinSnapshotsToKeep, "min-snapshots-to-keep must
not be wiped out on commit")
+ require.Equal(t, minKeep, *ref.MinSnapshotsToKeep)
+ require.NotNil(t, ref.MaxSnapshotAgeMs, "max-snapshot-age-ms must not
be wiped out on commit")
+ require.Equal(t, maxSnapAgeMs, *ref.MaxSnapshotAgeMs)
+ require.NotNil(t, ref.MaxRefAgeMs, "max-ref-age-ms must not be wiped
out on commit")
+ require.Equal(t, maxRefAgeMsIn, *ref.MaxRefAgeMs)
+
+ // The emitted metadata update must also carry the preserved retention
so
+ // REST catalogs receive the correct values.
+ last := builder.updates[len(builder.updates)-1].(*setSnapshotRefUpdate)
+ require.Equal(t, minKeep, last.MinSnapshotsToKeep)
+ require.Equal(t, maxSnapAgeMs, last.MaxSnapshotAgeMs)
+ require.Equal(t, maxRefAgeMsIn, last.MaxRefAgeMs)
+}
+
+func TestSetSnapshotRefOverridesRetentionWhenProvided(t *testing.T) {
+ builder := builderWithoutChanges(2)
+ schemaID := 0
+ snapshot := Snapshot{
+ SnapshotID: 1,
+ ParentSnapshotID: nil,
+ SequenceNumber: 0,
+ TimestampMs: builder.base.LastUpdatedMillis() + 1,
+ ManifestList: "/snap-1.avro",
+ Summary: &Summary{Operation: OpAppend},
+ SchemaID: &schemaID,
+ }
+ require.NoError(t, builder.AddSnapshot(&snapshot))
+ require.NoError(t, builder.SetSnapshotRef(MainBranch, 1, BranchRef,
WithMinSnapshotsToKeep(5)))
+
+ // An explicitly provided option overrides the inherited value.
+ require.NoError(t, builder.SetSnapshotRef(MainBranch, 1, BranchRef,
WithMinSnapshotsToKeep(10)))
+ ref := builder.refs[MainBranch]
+ require.NotNil(t, ref.MinSnapshotsToKeep)
+ require.Equal(t, 10, *ref.MinSnapshotsToKeep)
+}
+
+func TestSetSnapshotRefRetentionNotInheritedAcrossRefTypes(t *testing.T) {
+ builder := builderWithoutChanges(2)
+ schemaID := 0
+ snapshot := Snapshot{
+ SnapshotID: 1,
+ ParentSnapshotID: nil,
+ SequenceNumber: 0,
+ TimestampMs: builder.base.LastUpdatedMillis() + 1,
+ ManifestList: "/snap-1.avro",
+ Summary: &Summary{Operation: OpAppend},
+ SchemaID: &schemaID,
+ }
+ require.NoError(t, builder.AddSnapshot(&snapshot))
+ require.NoError(t, builder.SetSnapshotRef(MainBranch, 1, BranchRef,
WithMinSnapshotsToKeep(5)))
+
+ // A tag with the same name must not inherit branch-only retention such
as
+ // min-snapshots-to-keep, which tags do not support.
+ require.NoError(t, builder.SetSnapshotRef(MainBranch, 1, TagRef))
+ ref := builder.refs[MainBranch]
+ require.Equal(t, TagRef, ref.SnapshotRefType)
+ require.Nil(t, ref.MinSnapshotsToKeep)
+ require.Nil(t, ref.MaxSnapshotAgeMs)
Review Comment:
Two gaps on the tag side here.
This asserts `MinSnapshotsToKeep` and `MaxSnapshotAgeMs` are nil after the
cross-type switch but not `MaxRefAgeMs` — and `MaxRefAgeMs` is the one
retention field valid on both branches and tags, so it's exactly the one a
relaxed condition could leak. I'd add `require.Nil(t, ref.MaxRefAgeMs)` here.
There's also no positive tag→tag case: set `MaxRefAgeMs` on a tag, re-point
it with `TagRef` and no options, assert it's preserved. Tags support that
field, so it should inherit just like branches do.
##########
table/metadata.go:
##########
@@ -908,6 +908,23 @@ func (b *MetadataBuilder) SetSnapshotRef(
return fmt.Errorf("%w: invalid snapshot ref option:
%w", iceberg.ErrInvalidArgument, err)
}
}
+
+ // preserve retention properties from an existing ref when not
explicitly overridden by caller
+ if existingRef, ok := b.refs[name]; ok && existingRef.SnapshotRefType
== refType {
+ if ref.MinSnapshotsToKeep == nil &&
existingRef.MinSnapshotsToKeep != nil {
Review Comment:
While we're here — `metadata.go` already has `clonePtr[T]` a few hundred
lines up that does exactly this nil-check-then-copy. Since `clonePtr(nil)`
returns nil, you can drop the outer `!= nil` guards too:
```go
if ref.MinSnapshotsToKeep == nil {
ref.MinSnapshotsToKeep = clonePtr(existingRef.MinSnapshotsToKeep)
}
```
--
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]
