(iceberg-go) branch main updated: fix(table): guard nil base in snapshot timestamp check (#1799)

[ze...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

zeroshade pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-go.git


The following commit(s) were added to refs/heads/main by this push:
     new 6e678465d fix(table): guard nil base in snapshot timestamp check (#1799)
6e678465d is described below

commit 6e678465d6722a3f7e8783257448a12dbe9238d2
Author: Badal Prasad Singh <ba...@datazip.io>
AuthorDate: Fri Aug 14 21:29:45 2026 +0530

    fix(table): guard nil base in snapshot timestamp check (#1799)
    
    * fix: guard the nil base
    
    Signed-off-by: badalprasadsingh <ba...@datazip.io>
    
    * fix: nil base
    
    Signed-off-by: badalprasadsingh <ba...@datazip.io>
    
    ---------
    
    Signed-off-by: badalprasadsingh <ba...@datazip.io>
---
 table/metadata.go                       |  28 ++++++-
 table/metadata_builder_internal_test.go | 133 ++++++++++++++++++++++++++++++++
 2 files changed, 158 insertions(+), 3 deletions(-)

diff --git a/table/metadata.go b/table/metadata.go
index ea09c3be1..50cf8d8c1 100644
--- a/table/metadata.go
+++ b/table/metadata.go
@@ -42,6 +42,7 @@ const (
 	supportedTableFormatVersion = 3
 	minFormatVersionRowLineage  = 3
 	initialRowID                = int64(0)
+	initialSequenceNumber       = int64(0)
 	oneMinuteInMs               = int64(60_000)
 )
 
@@ -524,6 +525,7 @@ func (b *MetadataBuilder) addSnapshotInternal(snapshot *Snapshot, preserveUpdate
 		return nil
 	}
 
+	lastSeqNum := b.currentLastSequenceNumber()
 	if len(b.schemaList) == 0 {
 		return errors.New("can't add snapshot with no added schemas")
 	} else if len(b.specs) == 0 {
@@ -532,9 +534,9 @@ func (b *MetadataBuilder) addSnapshotInternal(snapshot *Snapshot, preserveUpdate
 		return fmt.Errorf("can't add snapshot with id %d, already exists", snapshot.SnapshotID)
 	} else if b.formatVersion >= 2 &&
 		snapshot.ParentSnapshotID != nil &&
-		snapshot.SequenceNumber <= *b.lastSequenceNumber {
+		snapshot.SequenceNumber <= lastSeqNum {
 		return fmt.Errorf("can't add snapshot with sequence number %d, must be > than last sequence number %d",
-			snapshot.SequenceNumber, *b.lastSequenceNumber)
+			snapshot.SequenceNumber, lastSeqNum)
 	}
 
 	if len(b.snapshotLog) > 0 {
@@ -544,7 +546,15 @@ func (b *MetadataBuilder) addSnapshotInternal(snapshot *Snapshot, preserveUpdate
 				snapshot.TimestampMs, last.TimestampMs)
 		}
 	}
-	maxTS := max(b.lastUpdatedMS, b.base.LastUpdatedMillis())
+
+	// b.base is nil for builders created by NewMetadataBuilder (the create-table path);
+	// only MetadataBuilderFromBase populates it. Treat the missing base as "no previous update"
+	// instead of dereferencing it, mirroring the fallback in currentNextRowID.
+	var baseLastUpdated int64
+	if b.base != nil {
+		baseLastUpdated = b.base.LastUpdatedMillis()
+	}
+	maxTS := max(b.lastUpdatedMS, baseLastUpdated)
 	if snapshot.TimestampMs-maxTS < -oneMinuteInMs {
 		return fmt.Errorf("invalid snapshot timestamp %d: before last updated timestamp %d",
 			snapshot.TimestampMs, maxTS)
@@ -604,6 +614,18 @@ func (b *MetadataBuilder) validateAndUpdateRowLineage(snapshot *Snapshot) error
 	return nil
 }
 
+// currentLastSequenceNumber returns the last sequence number assigned to a
+// snapshot. b.lastSequenceNumber is nil for v1 tables and for builders created
+// by NewMetadataBuilder that have not added a snapshot yet, in which case the
+// spec's initial sequence number is the correct floor.
+func (b *MetadataBuilder) currentLastSequenceNumber() int64 {
+	if b.lastSequenceNumber != nil {
+		return *b.lastSequenceNumber
+	}
+
+	return initialSequenceNumber
+}
+
 func (b *MetadataBuilder) currentNextRowID() int64 {
 	if b.nextRowID != nil {
 		return *b.nextRowID
diff --git a/table/metadata_builder_internal_test.go b/table/metadata_builder_internal_test.go
index 7156ac2ea..c7e81af52 100644
--- a/table/metadata_builder_internal_test.go
+++ b/table/metadata_builder_internal_test.go
@@ -118,6 +118,41 @@ func builderWithoutChanges(formatVersion int) MetadataBuilder {
 	return *builder
 }
 
+func freshMetadataBuilder(t *testing.T, formatVersion int) *MetadataBuilder {
+	t.Helper()
+
+	tableSchema := schema()
+	partitionSpec := partitionSpec()
+	sortOrder := sortOrder()
+
+	builder, err := NewMetadataBuilder(formatVersion)
+	require.NoError(t, err)
+	require.NoError(t, builder.SetLoc("s3://bucket/test/location"))
+	require.NoError(t, builder.AddSchema(&tableSchema))
+	require.NoError(t, builder.SetCurrentSchemaID(-1))
+	require.NoError(t, builder.AddSortOrder(&sortOrder))
+	require.NoError(t, builder.SetDefaultSortOrderID(-1))
+	require.NoError(t, builder.AddPartitionSpec(&partitionSpec, true))
+	require.NoError(t, builder.SetDefaultSpecID(-1))
+	require.Nil(t, builder.base, "builder from NewMetadataBuilder must have no base metadata")
+
+	return builder
+}
+
+func freshBuilderSnapshot(snapshotID int64, parentSnapshotID *int64, sequenceNumber, timestampMs int64) Snapshot {
+	schemaID := 0
+
+	return Snapshot{
+		SnapshotID:       snapshotID,
+		ParentSnapshotID: parentSnapshotID,
+		SequenceNumber:   sequenceNumber,
+		TimestampMs:      timestampMs,
+		ManifestList:     fmt.Sprintf("/snap-%d.avro", snapshotID),
+		Summary:          &Summary{Operation: OpAppend, Properties: map[string]string{}},
+		SchemaID:         &schemaID,
+	}
+}
+
 func TestBuildUnpartitionedUnsorted(t *testing.T) {
 	TestLocation := "file:///tmp/iceberg-test"
 	tableSchema := schema()
@@ -1301,6 +1336,104 @@ func TestAddSnapshotRejectsInvalidTimestamp(t *testing.T) {
 	require.NoError(t, err)
 }
 
+func TestAddSnapshotOnBuilderWithoutBase(t *testing.T) {
+	t.Run("parentless snapshot", func(t *testing.T) {
+		builder := freshMetadataBuilder(t, 2)
+		snapshot := freshBuilderSnapshot(1, nil, 0, time.Now().UnixMilli())
+
+		require.NoError(t, builder.AddSnapshot(&snapshot))
+
+		meta, err := builder.Build()
+		require.NoError(t, err)
+		require.Len(t, meta.Snapshots(), 1)
+		require.Equal(t, snapshot.TimestampMs, meta.LastUpdatedMillis())
+	})
+
+	t.Run("snapshot with parent", func(t *testing.T) {
+		builder := freshMetadataBuilder(t, 2)
+		parentSnapshotID := int64(1)
+		snapshot := freshBuilderSnapshot(2, &parentSnapshotID, 1, time.Now().UnixMilli())
+
+		require.NoError(t, builder.AddSnapshot(&snapshot))
+
+		meta, err := builder.Build()
+		require.NoError(t, err)
+		require.Len(t, meta.Snapshots(), 1)
+		require.Equal(t, int64(1), meta.LastSequenceNumber())
+	})
+
+	t.Run("rejects sequence number at the initial value", func(t *testing.T) {
+		builder := freshMetadataBuilder(t, 2)
+		parentSnapshotID := int64(1)
+		snapshot := freshBuilderSnapshot(2, &parentSnapshotID, 0, time.Now().UnixMilli())
+
+		err := builder.AddSnapshot(&snapshot)
+		require.ErrorContains(t, err, "can't add snapshot with sequence number 0, must be > than last sequence number 0")
+	})
+
+	t.Run("rejects timestamp before last update", func(t *testing.T) {
+		builder := freshMetadataBuilder(t, 2)
+		now := time.Now().UnixMilli()
+		first := freshBuilderSnapshot(1, nil, 0, now)
+		require.NoError(t, builder.AddSnapshot(&first))
+
+		// No ref has been set, so the snapshot log is empty and this exercises the
+		// last-updated bound rather than the last-snapshot bound.
+		require.Empty(t, builder.snapshotLog)
+		second := freshBuilderSnapshot(2, nil, 1, now-(2*oneMinuteInMs))
+
+		err := builder.AddSnapshot(&second)
+		require.ErrorContains(t, err, "before last updated timestamp")
+	})
+}
+
+func TestAddSnapshotOnBuilderWithoutBaseV3(t *testing.T) {
+	// v3 row lineage validation only became reachable for a builder without base
+	// metadata once AddSnapshot stopped panicking before it.
+	builder := freshMetadataBuilder(t, 3)
+	firstRowID, addedRows := int64(0), int64(10)
+	snapshot := freshBuilderSnapshot(1, nil, 0, time.Now().UnixMilli())
+	snapshot.FirstRowID, snapshot.AddedRows = &firstRowID, &addedRows
+
+	require.NoError(t, builder.AddSnapshot(&snapshot))
+	require.Equal(t, addedRows, builder.NextRowID())
+
+	behindFirstRowID := int64(5)
+	parentSnapshotID := int64(1)
+	stale := freshBuilderSnapshot(2, &parentSnapshotID, 1, time.Now().UnixMilli())
+	stale.FirstRowID, stale.AddedRows = &behindFirstRowID, &addedRows
+
+	err := builder.AddSnapshot(&stale)
+	require.ErrorIs(t, err, ErrInvalidRowLineage)
+	require.ErrorContains(t, err, "first-row-id 5 is behind table next-row-id 10")
+
+	meta, err := builder.Build()
+	require.NoError(t, err)
+	require.Len(t, meta.Snapshots(), 1)
+	require.Equal(t, addedRows, meta.NextRowID())
+}
+
+func TestAddSnapshotUpdateOnBuilderWithoutBase(t *testing.T) {
+	builder := freshMetadataBuilder(t, 2)
+	parentSnapshotID := int64(1)
+	snapshot := freshBuilderSnapshot(2, &parentSnapshotID, 1, time.Now().UnixMilli())
+	update := NewAddSnapshotUpdate(&snapshot)
+
+	require.NoError(t, builder.AddSnapshotUpdate(update))
+
+	// AddSnapshotUpdate must store the caller's update verbatim rather than
+	// rebuilding it, so runtime-only fields survive the OCC retry path.
+	require.NotEmpty(t, builder.updates)
+	stored, ok := builder.updates[len(builder.updates)-1].(*addSnapshotUpdate)
+	require.True(t, ok, "expected last update to be *addSnapshotUpdate, got %T", builder.updates[len(builder.updates)-1])
+	require.Same(t, update, stored)
+
+	meta, err := builder.Build()
+	require.NoError(t, err)
+	require.Len(t, meta.Snapshots(), 1)
+	require.Equal(t, int64(1), meta.LastSequenceNumber())
+}
+
 func TestConstructDefaultMainBranch(t *testing.T) {
 	// TODO: Not sure what this test is supposed to do Rust: `test_construct_default_main_branch`
 	meta, err := getTestTableMetadata("TableMetadataV2Valid.json")