feat(table): add RewriteManifests clustering

[fallintoplace]

What changed

• add WithRewriteManifestClusterBy for opt-in clustering
• group live data entries by cluster key + partition spec
• roll each key at the configured manifest target size
• keep the default size-only path unchanged
• clean open output manifests when a callback or writer fails

Why

• The Java RewriteManifests.clusterBy API uses the same idea: keep files with the same key together.
• This is useful when the key matches a partition value used by common filters.
• The clustered layout can let scan planning reject unrelated manifests before opening their entries.
• The option is opt-in, so existing rewrites keep the current behavior.

Benchmark

Rewrite throughput was roughly neutral in the benchmark. The important result is the read-side benchmark. It exercises Scan.filterManifestsWithSchema, the manifest-pruning stage used by local PlanFiles.

Command: go test ./table -run=^$ -bench=^BenchmarkManifestPruningModes$ -benchmem -benchtime=1s -count=3

Workload: 512 one-entry input manifests, 32 partition values, and an equality filter on id = 7.

• size-only: 18.6–18.8 µs/op, 17,728 B/op, 580 allocs/op, 64/64 manifests selected
• cluster-by: 10.8–13.5 µs/op, 11,456 B/op, 324 allocs/op, 1/32 manifests selected

That is about 40% lower pruning time, 35% fewer bytes, and 44% fewer allocations for this partition-aligned workload.

Java reference: https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/RewriteManifests.java

Tests

• go test ./table/...
• go vet ./table/...
• go test -race ./table -run 'TestRewriteManifests(ClusterBy|CleansOrphansOnInvalidClusterKey)' -count=1

[zeroshade]

The clustering and cleanup paths otherwise look good, and the focused race tests, full table subtree, vet, benchmarks, CI, and synthetic merge all passed. One public-API panic remains in cluster-key validation.

[zeroshade]

[P1] Reject cluster keys that cannot safely round-trip through the writer map

reflect.Type.Comparable() only checks the static type. For example, struct{ Value any }{Value: []int{1}} passes this check and then writers[key] panics with hash of unhashable type: []int. A math.NaN() key also passes, but because it is not equal to itself the inserted writer cannot be retrieved during finalization, causing a nil-pointer panic at line 205. I reproduced both through the exported Transaction.RewriteManifests API; NaN is a realistic value when clustering by an identity float partition. Please validate the value itself (for example, reflect.Value.Comparable() plus a reflexivity check) and add public-API regressions for both cases.

[laskoviymishka]

This is in good shape. The clustering path reads cleanly: one rolling writer per (spec, key) with the writers held open across input manifests so interleaved same-key entries still land together, the deferred abort and orphan cleanup on the failure path, and live files written as EXISTING so the original snapshot and sequence numbers are preserved. The Schema.MarshalJSON fix is a real one too. Copying only the two JSON-tagged fields instead of the whole struct drops the atomic.Pointer copy that was racing the lazy-lookup caches, and the new concurrency test covers it well.

The cluster-key validation thread zeroshade opened looks addressed to me. validateManifestClusterKey now checks Comparable() before the map lookup and adds the reflexivity check for NaN, so I couldn't find a way to reach the panic at head.

I'm not going to hold this hard. The one thing I'd like to settle before merge is the OCC-retry behavior: every retry reruns the full clustering pass from scratch, where Java's requiresRewrite reuses the already-written output when nothing was displaced. The committed result is correct either way, so this is more "decide and document" than a bug, but if clusterBy is non-deterministic we'd produce a different layout each retry where Java wouldn't. I'd want us to either mirror requiresRewrite or state that clusterBy must be deterministic.

The rest is smaller. The per-entry reflect validation does K keys' worth of work N entries' worth of times, the pruning payoff is only asserted inside a benchmark CI never runs, and there are a couple of doc notes worth adding (concurrent-writer cardinality, and that clustering bypasses minCountToMerge). Details inline.

Settle the OCC-retry question (even if the answer is just "clusterBy must be deterministic") and pull the pruning check into a real test, and I'm happy to take another pass and approve.

[laskoviymishka]

Not a blocker, the committed output is correct either way. But every OCC retry reruns the whole clustering pass from scratch (processManifests, mergeManifests, then here), so under contention we write a full new set of clustered manifests on each attempt. Java's BaseRewriteManifests avoids that with requiresRewrite: if the manifests it already processed are all still present in the fresh parent, it reuses the previously written output instead of re-clustering.

The extra I/O aside, there's a subtler divergence. If clusterBy is non-deterministic, Java locks in the first attempt's layout when nothing was displaced, but we'd produce a fresh clustering each retry. I'd at least decide explicitly whether to mirror requiresRewrite here or document that clusterBy must be deterministic. wdyt?

[laskoviymishka]

validateManifestClusterKey runs the full reflect pass (TypeOf, ValueOf, Comparable, the kind switch, and the reflexivity Equal) on every entry, but once a key is in writers we've already proven it's a safe map key. On a table with millions of entries across a handful of clusters that's millions of reflect round-trips where a dozen would do.

We still need the comparability guard before writers[key] to avoid the panic, but that can be the cheap half: reflect.ValueOf(clusterValue).Comparable() per entry, with the full validateManifestClusterKey (including the NaN reflexivity check) deferred to the !ok new-key branch. wdyt?

[laskoviymishka]

This gate is the only place we assert that clustering actually lets the evaluator prune manifests, which is the read-side payoff the whole feature is for, and it only runs under go test -bench. Regular go test ./... skips benchmark bodies, so CI never checks it. The unit tests confirm entries share a key, but not that pruning follows.

I'd lift the clustered-vs-size-only filterManifestsWithSchema comparison into a normal Test on a smaller dataset asserting len(selected) < len(total) for the clustered layout, so the guarantee is enforced in CI.

[laskoviymishka]

counter.Count only advances when the Avro writer flushes a block, not per entry (the createManifest comment at snapshot_producers.go:422 calls this out), so this fires at block granularity and a manifest can overshoot targetSizeBytes by close to a full block before it rolls. Small for a 128MB target, but worth a line in the godoc.

Related: TestRewriteManifestsClusterByRollsAtTargetSize passes with targetSizeBytes=1 only because the Avro header already clears 1 byte before any entry, so it isn't really exercising a mid-stream roll. A test with a target that trips on the block boundary would be a more honest signal.

[laskoviymishka]

Worth documenting here: one writer stays open per distinct (specID, clusterKey) for the entire pass, so a high-cardinality clusterBy (say, keying on file path) keeps that many files open against the object store at once and can hit fd or handle limits. Java's clusterBy has the same one-in-flight-file-per-key shape, so I'd just note the cardinality expectation in the godoc rather than change behavior.

[laskoviymishka]

This bails before groupBySpec and minCountToMerge. Correct for RewriteManifests since it pins minCountToMerge=1, but manifestMergeManager is a shared struct, so a future caller that sets both clusterBy and minCountToMerge would have the latter silently dropped. A short comment that clustering replaces bin-packing (minCountToMerge included) rather than composing with it would save someone that surprise.

[laskoviymishka]

Two combos I'd add alongside this while we're here. A conflict+retry in clustering mode: clustering rewrites everything as new files each attempt, so the superseded-generation cleanup is leaned on harder here than in bin-packing, and none of the existing OCC-retry tests pass WithRewriteManifestClusterBy (this ties into the re-cluster note on clusterManifests). And clusterBy combined with a predicate or specID: both narrow r.eligible before clustering and are publicly supported, but the kept-vs-rewritten split against the file-count guard is currently untested.

[laskoviymishka]

Small one: entryErr is the range iterator's error variable, and reusing it for the writer-creation result conflates "iterator failed" with "writer creation failed". Harmless today since the range rebinds it each iteration, but a future edit between here and the next entryErr check could end up testing the wrong error. I'd give this its own var.

[laskoviymishka]

len(order) is a safe lower-bound capacity here since each key contributes at least one manifest, so make([]iceberg.ManifestFile, 0, len(order)) skips the early regrows.

[laskoviymishka]

benchmarkManifestPruningMode opens with b.Helper() but this identically-shaped sibling doesn't, so a b.Fatal here points inside the helper instead of at the b.Run callsite. Adding b.Helper() as the first statement lines the two up.

[zeroshade]

The clustering implementation preserves live data-file identity and entry sequence metadata, and the OCC reuse and pruning coverage addresses the earlier concerns. Approving, with one minor error-path cleanup to pick up whenever convenient.

What I verified

The invariant that matters for RewriteManifests is that it reorganises metadata without changing table contents, so:

• ManifestWriter.Existing (manifest.go:1995-1999) passes through entry.SnapshotID(), the entry sequence number, entry.FileSequenceNum(), and the DataFile unchanged — so snapshot ID and both sequence numbers survive the rewrite.
• clusterManifests (rewrite_manifests_cluster.go:173) walks every live entry exactly once and routes it by (specID, key), rolling writers only after the flush threshold. The live file set is preserved.
• Emitting retained entries as EXISTING and dropping DELETED ones is correct and matches both Java's BaseRewriteManifests and this repo's existing behaviour — ADDED means "added in this snapshot", which isn't true of a relocated file. Noting it explicitly because it looks like a status change if you only read the diff.
• Cluster-key validation happens before the map lookup (rewrite_manifests_cluster.go:178-180), with first-use reflexivity validation rejecting non-reflexive values — that closes my earlier finding, and the nested-uncomparable and NaN regression tests cover it.
• OCC retry reuse is implemented and tested: rewrite_manifests_test.go:1013+ confirms all three original stale files survive two conflicts and the callback is not re-run.

Minor — output path orphaned on writer-constructor error

rewrite_manifests_cluster.go:83 calls snapshotProducer.newManifestWriter. That factory creates the object-store path first, and if the NewManifestWriter constructor then errors it closes the handle without removing the path (snapshot_producers.go:725-737). The cluster cleanup defer only records paths after successful writer creation (rewrite_manifests_cluster.go:194, :214), so a constructor failure — malformed spec or schema, say — leaves an unreferenced empty manifest object behind.

Successful rewrites are unaffected, so this is cleanup rather than correctness. Removing the just-created path on constructor failure, plus a focused failure test, would close it.

Out of scope, noted for later

The pre-existing manifestActiveFiles guard compares active file counts rather than file-path sets, so a same-count swap would pass it. Not this PR's problem — it's untouched here apart from a doc comment, and the count fallback exists deliberately because V1 manifests don't populate the count fields. Worth hardening separately if you think it's worth the read cost.

CI green (15/15).

---

This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg Go maintainer, who has read the findings and signed off. If something feels off, please reply on the PR and a maintainer will follow up.

More on how to contribute to Apache Iceberg Go: CONTRIBUTING.md

[zeroshade]

My approval stands on the production diff. I went after the post-approval commit specifically, since a late resource-cleanup fix is exactly the kind of change that introduces a double-close or a missed path, and it holds up. Three things I'd like to land before merge, none of them production defects.

The post-approval delta

I approved at 397f86b1d. Exactly one commit landed after: 25880809. Its production change is one line:

table/snapshot_producers.go:735
-  return nil, "", nil, nil, errors.Join(err, out.Close())
+  return nil, "", nil, nil, errors.Join(err, out.Close(), sp.io.Remove(path))

Everything else is test-side: the new orphan test, plus mechanical signature adaptation for filterManifestsWithSchema's new partitionFilters parameter. I confirmed partitionFiltersForSchema is upstream (scanner.go:806, used at :936, :1068, :1237), so those hunks are rebase adaptation, not behaviour change.

It doesn't touch anything I verified: my approval cited manifest.go:1995-1999, rewrite_manifests_cluster.go:173 and :178-180, and rewrite_manifests_test.go:1013+. None changed. And the fix itself is right — argument evaluation is left-to-right so out.Close() runs before sp.io.Remove(path), which is the correct order for filesystems that refuse to unlink an open handle; err is first in the errors.Join so it stays the cause; and no caller can double-remove, because the error return is nil, "", nil, nil, err and no caller ever learns path.

Not inert: reverting only the sp.io.Remove(path) term fails the new test with the orphan visible in the actual set.

What I verified on the cleanup path

24-combination injected-failure matrix — a FileWriter stub with per-instance Write/Close accounting, across writer index ∈ {0,1,2} × failing write # ∈ {1 (ocf header, inside the constructor), 2 (block flush at Close)} × targetSizeBytes ∈ {8 MiB, 1} × Close-always-fails ∈ {false, true}, over 6 inputs / 3 interleaved cluster keys. In every combination:

• Every writer created was closed exactly once. Close vectors observed: [1], [1 1], [1 1 1], and on the roll path [1 1 1 1], [1 1 1 1 1]. Never 0, never 2.
• No error shadowing. The write failure always led the message with the cleanup close error appended ("probe: write failed\nprobe: close failed"), never the reverse.
• No object left behind.

Plus: a failing Close on the success path surfaces the error and returns nil manifests, so a manifest whose file failed to close is never handed back for registration — close() computes ToManifestFile before the deferred fileCloser.Close(), and closeWriter discards the manifest whenever closeErr != nil. A clusterBy callback that panics mid-pass still runs the deferred cleanup: every writer closed once, no orphans, panic propagates.

I mutation-tested my own probe so that isn't vacuous — removing the fileCloser nil-ing (double close), deleting the deferred abort() loop, and deleting the deferred path-removal loop each produce 12–20 subtest failures.

Clustering preserves the entry multiset. 12 inputs → 3 clustered outputs, and 9 inputs at targetSizeBytes=1 → 9 rolled outputs. Full projection (path, snapshot ID, both sequence numbers, record count, partition) byte-identical before/after, only Status ADDED→EXISTING as designed. Per output manifest, header existing_files equals the real entry count, added/deleted are 0, and min_sequence_number is exactly the min over that manifest's entries.

Reader compatibility on V3. Six single-row appends, clustered rewrite, committed: every live data file kept its first_row_id (0–5 unchanged), snapshot ID, both sequence numbers and partition. The two new manifests got manifest-list first_row_id 6 and 9 with rows=3 each — disjoint and ordered. Safe because ManifestReader materialises df.FirstRowIDField on read and ManifestListWriter only assigns when FirstRowIDValue == nil, so lineage survives regrouping.

golangci-lint 0 issues; -race -count=3 clean; CI 15/15.

Major — please land these

1. table/rewrite_manifests.go:220-222 — the requiresRewrite safety guard has zero test coverage.

if len(required) > 0 {
	return nil, false, nil
}

This is the branch that prevents reusing attempt-0's clustered output when a concurrent writer displaced one of the manifests this rewrite replaced. I deleted those three lines and ran the whole package: ok github.com/apache/iceberg-go/table 11.668s. Not one test noticed.

With the guard gone, reuse emits r.added plus every fresh-parent manifest not in the rewritten set. If a peer merged M0,M1,M2 into N, the result is [X, N] and every original data file appears twice — duplicated rows, silently committed. checkRemovedFiles doesn't help, since rewriteManifests registers no deletedFiles. The existing TestRewriteManifestsClusterByReusesOutputOnOCCRetry only covers the safe direction (it is real — disabling reuse fails it with clusterCalls 12 != 3), but the unsafe direction is untested.

A regression test I wrote and validated (passes at head, fails on the mutation): have a peer RewriteManifests merge the three manifests our rewrite replaces so none survives in the fresh parent, then commit the clustered rewrite from the stale head and assert each of stale-0/1/2 appears exactly once across the committed manifests.

Secondary note on the same block: :226 and :228 are only non-negative because of that guard — the mutation didn't corrupt data, it panicked in makeslice: cap out of range. Correct today, but the arithmetic silently depends on an invariant three lines above; max(0, …) or a comment would make that explicit.

2. table/rewrite_manifests_test.go:584 and :665 — two existing tests were repurposed, dropping the only coverage of the default size-only paths.

-	res, err := txn.RewriteManifests(ctx, table.WithRewriteManifestPredicate(pred))
+	res, err := txn.RewriteManifests(ctx,
+		table.WithRewriteManifestPredicate(pred),
+		table.WithRewriteManifestClusterBy(func(iceberg.DataFile) any { return "selected" }),
+	)

(same shape for WithRewriteSpecID(0)). TestRewriteManifestsPredicate and TestRewriteManifestsSpecIDFilter no longer test what their names say. Grep confirms they were the only coverage: WithRewriteManifestPredicate appears in exactly one test, and WithRewriteSpecID in two, of which :540 is the invalid-id error case. So at head there is no test for predicate + size-only or specID + size-only — and cmd/iceberg/rewrite_manifests.go:36 passes WithRewriteSpecID with no clustering, so the CLI's exact configuration is the untested one.

The conversion wasn't necessary — I removed the added clusterBy from both and they pass unchanged. @laskoviymishka asked to add these combos "alongside this"; please make them subtests (t.Run("SizeOnly"/"Clustered")) or sibling tests rather than converting in place.

3. The benchmark table is stale. Ran your exact command, -count=6, benchstat:

metric	claimed	measured at head
cluster sec/op	−40%	−54.07% (p=0.002)
size-only B/op	17,728	9,568
cluster B/op	11,456	6,624
B/op improvement	−35%	−30.77%
size-only allocs/op	580	176
cluster allocs/op	324	112
allocs improvement	−44%	−36.36%
manifests selected	64/64 vs 1/32	64/64 vs 1/32 ✅

I hoisted partitionFiltersForSchema back inside the loop to emulate the pre-head-commit benchmark and got 11,072 B / 196 allocs — still nowhere near 17,728/580. So the table predates not just the head commit but the whole rebase onto main (#1965, #1976, #1964 all cut allocations in this path). sec/op variance is high here (±42%, sibling agents loading the box); the alloc/byte figures are deterministic and those are the ones that are wrong.

"Rewrite throughput was roughly neutral" is accurate — 26.2 vs 25.6 ms/op, 192,869 vs 192,832 allocs.

Minor

• rewrite_manifests_cluster.go:226 — if writer.writer == nil || !writer.hasEntries { continue } is unreachable today, but if it ever became reachable it would be fail-unsafe: completed = true runs right after, suppressing the deferred cleanup, so you'd leak an open handle and an orphan object. writer.abort() in that branch makes it harmless. (You can't call closeWriter there — ManifestWriter.Close returns ErrEmptyManifest, which is why the guard exists.)
• snapshot_producers.go:513 / rewrite_manifests_cluster.go:173,217 (latent) — clusterManifests reads with discardDeleted=true and writes everything as Existing(), whereas createManifest has a three-way switch emitting Add() for files added by this snapshot and Delete() for its tombstones. Correct for RewriteManifests (it adds and deletes nothing — and I blessed this as matching Java's BaseRewriteManifests). But manifestMergeManager is shared with mergeAppendFiles, and if anything ever set clusterBy there, newly-added files would be stamped EXISTING and this-snapshot tombstones silently dropped. One sentence on clusterManifests stating it assumes a producer that neither adds nor deletes files would prevent a nasty surprise.
• Three of @laskoviymishka's nits are still open: entryErr reused for writer construction (:188), make([]iceberg.ManifestFile, 0) with no capacity hint (:234), and the missing b.Helper() on benchmarkManifestMergeMode (bench_test.go:47).

Prior items

Mine (CHANGES_REQUESTED 2026-08-28): P1 cluster keys that can't round-trip → Fixed, and mutation-verified both ways: reverting to Type.Comparable() reproduces panic: hash of unhashable type: []int, and deleting the reflexivity check reproduces the NaN nil-deref. Both driven through the exported API.

Mine (APPROVED 2026-09-01): orphaned output path on writer-constructor error → Fixed by the post-approval commit. manifestActiveFiles comparing counts not path sets → N.A., still deferred.

@laskoviymishka (APPROVED 2026-08-31): OCC retry re-clusters from scratch → Fixed, both the requiresRewrite mirror and the determinism note, mutation-verified — but see Major 1. Per-entry reflect validation → Fixed exactly as requested (cheap check at :179, full at :185). Pruning payoff only in a benchmark → Fixed, lifted into a real test, mutation-verified twice. Block-granularity counter.Count overshoot + a roll test that trips a real block boundary → Fixed, and the new TestManifestMergeClusterRollsAfterAvroBlockFlush is mutation-verified. Writer-cardinality doc → Fixed. Clustering replaces bin-packing including minCountToMerge → Partially fixed (statement lives on the public option rather than the dispatch site; fine by me). Conflict+retry test in clustering mode → Fixed. clusterBy + predicate/specID combos → Partially fixed, and it caused Major 2.

Description

• The benchmark table is stale (Major 3).
• Files omitted: table/snapshot_producers.go isn't mentioned, and it gains the retryManifestRebuilder interface (:80-87) plus a behaviour change to the shared newManifestWriter (:735) that now removes the created path on constructor failure for all four call sites, not just clustering.
• OCC-retry output reuse (rebuildManifests/reuseManifests) is a real semantic change to retry behaviour and appears nowhere in "What changed" — only in the commit log.
• The Tests section doesn't mention that two pre-existing tests were converted.

---

This review was drafted by an AI-assisted tool and confirmed by an Iceberg Go maintainer. The findings cite the project's review criteria; if you think one is mis-applied, please reply and a maintainer will weigh in.

[zeroshade]

Every one of the 11 prior review threads is genuinely fixed at 20fdd35 and pinned by a test — 10 targeted mutations each turned a test red, and probes confirm the cluster-key validator rejects all 13 nested non-reflexive/uncomparable shapes and that clustering preserves V3 first_row_id.

Re-review verification: 10 of 11 prior findings confirmed fixed at 20fdd35 (each verified by mutating the fix and observing the suite go red, not by taking the claim on trust). Still open:

• partially fixed — [laskoviymishka] entryErr is the range iterator's error variable; reusing it for the writer-creation result conflates two failures — give it its own var

Verification performed

go build ./table/... (ok); go vet ./table/ (clean); go test ./table/ -run 'TestRewriteManifests|TestManifestMergeCluster|TestManifestActiveFiles|TestCommitManifests|TestRebuildManifestList|TestParentDependentManifests' -count=1 (ok, 1.898s); go test -race ./table/ -run 'TestRewriteManifests|TestManifestMerge|TestCommitManifests|TestRebuildManifestList|TestParentDependentManifests|TestSnapshotProducer|TestCreateManifest' -count=1 (ok, 4.279s, no races); go test ./table -bench=^BenchmarkManifestMergeModes$ -benchtime=20x -count=2 (BySize 14.7-20.0ms/op vs ByClusterKey 14.8-15.0ms/op — merge throughput neutral, confirming the PR's claim); 10 mutation runs, all red as tabulated; 3 throwaway probes in table/pr1940_probe_test.go (V3 row-ID preservation PASS, 13 nested bad-key shapes all rejected, fresh-pointer key accepted-and-committed) — probe deleted, `git status --short` and `git diff --stat` both empty at 20fdd35. Separately verified the pi-lens 'catalog/glue setup failed' report is spurious: `go vet ./catalog/glue/` clean and `go test ./catalog/glue/ -run XXX` returns ok in this worktree, and the PR touches only table/.

---

This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg Go maintainer. The maintainer approving this PR has read the findings and signed off. If something feels off, please reply on the PR and a maintainer will follow up.

More on how Apache Iceberg Go handles maintainer review: CONTRIBUTING.md.

[zeroshade]

nit — Unreachable defensive branch in the finalization loop can silently orphan a file if it ever becomes reachable

if writer.writer == nil || !writer.hasEntries { writer.abort(); continue } cannot fire: every writer placed in order is opened immediately before an Existing(entry) call (:220-223), and the roll path at :207-218 re-opens and then writes before the loop advances, so at finalization every writer has a live *ManifestWriter and hasEntries==true. Any error path returns early instead of reaching here. The branch is harmless today, but it is the one place that abandons a writer while completed is later set to true at :244 — which means the file at writer.path, already appended to paths at :197/:217, would be skipped by the cleanup defer at :154-158 and left as a silent orphan. Either drop the branch, or if it is kept as a guard, remove its path from paths (or delete the file) so the invariant 'a manifest we abort is never left behind' holds unconditionally.

[zeroshade]

nit — Residual entryErr shadow at the roll site

Thread 8 asked for the writer-result error to stop borrowing the range iterator's entryErr name. Both writer-creation sites were changed to openErr (:190, :208) but the roll site still reads if entryErr := closeWriter(writer); entryErr != nil. It is a distinct shadowing declaration so the code is correct, but it re-introduces exactly the naming the thread flagged. Suggest closeErr for symmetry with openErr.

[zeroshade]

Both prior nits are correctly fixed; mutation testing proves the old continue caused silent data loss plus orphaned files while the new error return fails loudly and cleans up every path.

Re-review verification: 2 of 2 prior findings confirmed fixed at 0ab5337 (each verified by mutating the fix and observing the suite go red, not by taking the claim on trust).

Verification performed

go build ./table/... (OK); go vet ./table/... (OK); go test ./table/... -run 'Cluster|RewriteManifests|Manifest' -race -count=1 (ok table 8.928s, table/internal 3.470s); go test ./table/ -count=1 full package (ok 7.482s) under an injected panic() in the finalization branch to prove unreachability; throwaway table/pr1940_probe_test.go run against baseline, forced-branch-with-new-code, and forced-branch-with-old-code mutations. Worktree restored: git status --porcelain empty, build+vet green.

---

This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg Go maintainer. The maintainer approving this PR has read the findings and signed off. If something feels off, please reply on the PR and a maintainer will follow up.

More on how Apache Iceberg Go handles maintainer review: CONTRIBUTING.md.

[zeroshade]

nit — writer.abort() before the error return is redundant with the deferred cleanup

The deferred cleanup at lines 145-157 iterates for _, writer := range writers { writer.abort() }, and this writer is a member of writers. abort() is idempotent (it nils both w.writer and w.fileCloser under nil guards), so the explicit call is a harmless no-op duplicate. Verified the defer alone is sufficient: the forced-branch probe reported orphans=[] with all removal performed by the deferred path loop. Not worth a round-trip on its own.