fix(table): skip empty historical-delete manifest merges by 1fanwang · Pull Request #2057 · apache/iceberg-go

1fanwang · 2026-09-25T06:12:39Z

Manifest merging can fail a valid commit when a merge bin contains only historical DELETED entries. Those entries are intentionally filtered out, but the merge path opened a new manifest writer first, then closed it empty and returned empty manifest file has been written.

After this change, merge bins that retain no entries produce no replacement manifest. Current-snapshot deletes are still copied, non-empty bins still merge normally, and empty ManifestWriter calls still fail for direct callers.

Closes #2037

Testing

Raw logs

Before the fix:

$ go test ./table -run 'TestManifestMerge(SkipsHistoricalDeletedOnlyManifest|KeepsCurrentSnapshotDeletedEntries|GroupDropsEmptyMergedBin)'
--- FAIL: TestManifestMergeSkipsHistoricalDeletedOnlyManifest (0.00s)
    snapshot_producers_test.go:601:
        Error:       Received unexpected error:
                     empty manifest file has been written
--- FAIL: TestManifestMergeGroupDropsEmptyMergedBin (0.00s)
    snapshot_producers_test.go:664:
        Error:       Received unexpected error:
                     empty manifest file has been written
FAIL
FAIL    github.com/apache/iceberg-go/table    0.498s
FAIL

After the fix:

$ go test ./table -run 'TestManifestMerge(SkipsHistoricalDeletedOnlyManifest|KeepsCurrentSnapshotDeletedEntries|GroupDropsEmptyMergedBin)'
ok      github.com/apache/iceberg-go/table    0.543s

$ go test ./table/...
ok      github.com/apache/iceberg-go/table              8.299s
ok      github.com/apache/iceberg-go/table/compaction   2.538s
ok      github.com/apache/iceberg-go/table/dv           (cached)
ok      github.com/apache/iceberg-go/table/internal     (cached)
ok      github.com/apache/iceberg-go/table/substrait    (cached)

$ go vet ./table/...

$ golangci-lint run --timeout=10m ./table/...
0 issues.

Signed-off-by: 1fanwang <1fannnw@gmail.com>

laskoviymishka

The lazy-writer approach here is the right call. Filtering the bin before opening a writer sidesteps the ErrEmptyManifest guard without touching ManifestWriter's own validation, and I traced the classification switch: nothing live or current-snapshot can get dropped, so the reported commit crash is genuinely fixed.

What I'd resolve before this merges is the cleanup order on the error path. The two defers now close the underlying file before the ManifestWriter, so a mid-loop write failure flushes wr into an already-closed file. That's the same "write after close" hazard TestCommitManifestsCloseFailureReturnsNoUpdates was written to catch, reintroduced quietly because none of the new tests exercise that ordering. Collapsing both cleanups into one closure with an explicit order (rather than leaning on LIFO across two statements) fixes it, and a regression test that fails a later entry write after the writer's open would lock it down.

The rest is minor: the ensureWriter closure writes to the named-return err as a dead side effect, the zero-count-manifest omission is a permanent divergence from Java/PyIceberg worth a comment so it doesn't get "fixed" later, and the commitManifests/formatting changes are unrelated drive-bys I'd split out. Also worth noting #2037 isn't fully closed by this: a fully-tombstoned manifest that lands alone in a bin still bypasses filtering via the len(bin)==1 short-circuit.

laskoviymishka · 2026-09-25T09:08:30Z

 			internal.CheckedClose(wr, &err)
 		}
 	}()
+	defer func() {


The two cleanup defers run in the wrong order on the error path: LIFO now closes fileCloser before wr, so if an entry write fails mid-loop after the writer's already open, wr.Close() flushes the ManifestWriter into an already-closed file. That's the "write after close" hazard TestCommitManifestsCloseFailureReturnsNoUpdates was written to guard, and every other writer-cleanup site in this file registers the fileCloser defer first so wr flushes first.

I'd collapse both into one closure with an explicit order rather than relying on LIFO across two statements:

defer func() { if wr != nil && !writerClosed { internal.CheckedClose(wr, &err) } if fileCloser != nil { internal.CheckedClose(fileCloser, &err) } }()

And add a regression test that opens the writer then fails a later entry write (the trackingIO/failWriteAt idiom), asserting NotContains "write after close". None of the new tests hit that ordering today, which is why this slips through.

laskoviymishka · 2026-09-25T09:08:30Z

+			return nil
+		}
+
+		wr, path, counter, fileCloser, err = m.snap.newManifestWriter(spec)


This assigns into the named-return err, but every call site checks the closure's return value inside a for entry, err := range ... where the := shadows the outer err, so the explicit return nil, err is what actually propagates and this write to the outer err is dead. It reads like the named return is tracked automatically, which invites a future edit to drop the explicit check on the false assumption err is already set. I'd give the closure its own local:

ensureWriter := func() error { if wr != nil { return nil } var werr error wr, path, counter, fileCloser, werr = m.snap.newManifestWriter(spec) return werr }

laskoviymishka · 2026-09-25T09:08:30Z

 		}
 	}

+	if wr == nil {


Java's ManifestMergeManager and PyIceberg both keep a zero-count manifest here rather than dropping it, so for identical merge history Go emits fewer manifest files than the other clients. That's the intended tradeoff from #2037 and it's read-safe, but it's a permanent divergence worth a short comment right here (and a line in the PR description) so nobody diffing manifest counts across clients treats it as a bug and "fixes" it back to parity later.

laskoviymishka · 2026-09-25T09:08:30Z

 				return nil, err
 			}
-			output = append(output, created)
+			if created != nil {


Separate from this fix but related: the len(bin)==1 short-circuit in mergeGroup passes a lone manifest through unfiltered, so a manifest that's 100% historical-DELETED and lands alone in its bin (already at target size, or gated by minCountToMerge) never reaches createManifest and carries its dead tombstones forward indefinitely. This PR fixes the commit crash, not that; fine as a follow-up, but worth noting so #2037 isn't assumed fully closed.

laskoviymishka · 2026-09-25T09:08:30Z

+	require.Equal(t, df.FilePath(), entries[0].DataFile().FilePath())
+}
+
+func TestManifestMergeGroupDropsEmptyMergedBin(t *testing.T) {


The three new tests cover the happy paths well, but #2037's validation list explicitly calls for "a read error occurring before an output writer is created," and none of these hit it. I'd add a two-manifest bin where the first is all-historical-delete (writer stays nil) and the second fails to read, and assert createManifest returns the read error, not (nil, nil), and that no writer was opened. That's exactly the ensureWriter-gating seam most likely to regress if the error check is ever reordered.

laskoviymishka · 2026-09-25T09:08:30Z

+	// lacks stay 0 and are dropped by the `omitempty` tags); the catalog
+	// applies a set-snapshot-ref as a pure replace, so this fully
+	// determines the resulting ref rather than merging with the old one.
+	retainingSnapshotRef := sp.txn.meta.NewRetainingSnapshotRefUpdate(branch, sp.snapshotID, BranchRef)


This commitManifests refactor (and the NewDataFileBuilder / mergeConcurrency reformats) are behavior-neutral and unrelated to the empty-manifest fix. I'd pull them out so blame and bisect stay clean on the actual change.

Collapse the two deferred closes into a single closure with explicit order so a mid-loop write failure flushes into an open file, not a closed one. Add a regression test that fails a later entry write after the writer is opened. Document the intentional zero-count-manifest divergence from Java/PyIceberg. Signed-off-by: 1fanwang <1fannnw@gmail.com>

Signed-off-by: 1fanwang <1fannnw@gmail.com>

fix(table): skip empty historical-delete manifest merges

c397590

Signed-off-by: 1fanwang <1fannnw@gmail.com>

1fanwang requested a review from zeroshade as a code owner September 25, 2026 06:12

laskoviymishka requested changes Sep 25, 2026

View reviewed changes

1fanwang added 2 commits September 28, 2026 14:24

fix(table): gofmt and intrange lint fixes

84232e4

Signed-off-by: 1fanwang <1fannnw@gmail.com>

Navigation Menu

fix(table): skip empty historical-delete manifest merges - #2057

fix(table): skip empty historical-delete manifest merges#2057
1fanwang wants to merge 3 commits into
apache:mainfrom
1fanwang:1fannnw/skip-empty-manifest-merge

1fanwang commented Sep 25, 2026

Uh oh!

laskoviymishka left a comment

Uh oh!

laskoviymishka Sep 25, 2026

Uh oh!

laskoviymishka Sep 25, 2026

Uh oh!

laskoviymishka Sep 25, 2026

Uh oh!

laskoviymishka Sep 25, 2026

Uh oh!

laskoviymishka Sep 25, 2026

Uh oh!

laskoviymishka Sep 25, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

1fanwang commented Sep 25, 2026

Testing

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

laskoviymishka Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

laskoviymishka Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

laskoviymishka Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

laskoviymishka Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

laskoviymishka Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

laskoviymishka Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants