perf(table): index partition specs by ID

[fallintoplace]

Summary

• Adds an ID to position index for partition specs.
• Uses it for metadata PartitionSpecByID, default PartitionSpec, and builder GetSpecByID.
• Keeps the existing defensive-copy behavior.
• Rejects duplicate partition-spec IDs during metadata validation.
• Updates the index on add/remove and uses copy-on-write for builder clones.
• Includes fallback coverage for stale in-package slices and read-only concurrent lookups.

Benchmark

Command:

go test ./table -run '^$' -bench '^(BenchmarkPartitionSpecByID|BenchmarkMetadataBuilderGetSpecByID)$' -benchmem -benchtime=200ms -count=5

Median of 5 runs on Apple M1 Pro:

Workload	Before	After
Metadata, 2,048 specs, last hit	2,528 ns/op	74 ns/op
Metadata, 2,048 specs, miss	2,443 ns/op	1,390 ns/op
Builder, 2,048 specs, last hit	54.9 us, 98,304 B/op, 2,048 allocs/op	33.3 ns/op, 48 B/op, 1 alloc/op
Builder, 2,048 specs, miss	56.5 us, 98,403 B/op, 2,051 allocs/op	1.47 us, 72 B/op, 3 allocs/op

The metadata path still clones the returned spec. Builder misses still format the existing error. Indexed hits are O(1). Misses intentionally scan the slice to preserve fallback behavior for in-package element mutations, so miss lookups remain O(n). Small slices use the existing linear-scan path.

Checks

• go test ./table -count=1 -timeout=5m
• go test ./table -race -run '^(Test.*PartitionSpecIndex|Test.*PartitionSpecLookupsConcurrent|TestMetadataBuilderRemoveUnknownPartitionSpecKeepsIndex|TestRejectsDuplicatePartitionSpecIDs)$' -count=1
• go test ./... -run '^$' -count=1
• go vet ./table

[laskoviymishka]

Nice work here. The O(n)→O(1) indexing lines up cleanly with the existing snapshotIndex pattern, and the copy-on-write handling across clone/Build/AddPartitionSpec is careful. The benchmark coverage across spec counts is a good touch too.

I'd hold this before merging though.

The main thing is the linear-scan fallback in partitionSpecIndexPosition. The comment says it covers in-package fixtures that "replaced the slice", but it only detects whole-slice replacement. An element-wise mutation on the same backing array (specs[1] = X) misses the map, partitionSpecIndexNeedsRebuild reports the index is current, and we return "not found" for a spec that's actually there. No production path hits that today since builders always mutate through AddPartitionSpec/RemovePartitionSpecs, but the comment promises coverage the code doesn't deliver, and a silent false miss in the fallback is the kind of thing that bites later. I'd either tighten the comment or fall through to the scan on a miss (details inline).

Second, the two concurrent tests don't exercise the copy-on-write path they look like they're guarding. Every goroutine only reads an index that's built once and never mutated, so -race would pass even with the shared flag removed. The scenario worth covering is a reader on already-built metadata racing a builder that mutates and triggers the clone.

Things I'd like to settle before merge:

• Make the fallback comment match what the code detects, or cover element-wise mutation (and add a test that pins it either way)
• Rework one concurrent test to actually trigger copy-on-write under -race, or document that it only checks post-init read safety
• Confirm the index still wins at realistic (single-digit) spec counts, or gate it behind a length threshold
• Clean up the dead nil-guard in buildCommonMetadata and the duplicated guard block in clone()

Once those are settled, happy to take another pass and approve.

[laskoviymishka]

Small thing while we're here: for i, spec := range specs copies each PartitionSpec by value and then calls spec.ID() twice, while the fallback scan below uses for i := range specs + specs[i].ID(). Worth matching that here: id := specs[i].ID() once, index off i.

[laskoviymishka]

The comment above says the linear-scan fallback covers in-package fixtures that "replaced the slice", but it only actually covers whole-slice replacement. An element-wise mutation on the same backing array slips through and returns a silent false miss.

Start from specs [A(0), B(1)], build the index, then do specs[1] = C(5) in place: a lookup for id 5 misses the map, partitionSpecIndexNeedsRebuild returns false (both len and &specs[0] are unchanged), and we return (0, false), claiming C isn't there when it's sitting at position 1.

No production path mutates a spec slice element-wise like that today since builders always go through AddPartitionSpec/RemovePartitionSpecs, so this isn't live. But the comment promises more than the code delivers. I'd either tighten the comment to say we only detect whole-slice replacement, or fall through to the linear scan on a miss when the index looks current, though that second option turns every genuine miss into an O(n) scan, which partly defeats the miss fast-path the benchmark is measuring. A test doing specs[1] = X would pin whichever behavior we pick. wdyt?

[laskoviymishka]

These are two separate if b.partitionSpecIndex != nil blocks with nothing mutating the field between them, so the second guard is dead: the split just makes a reader double-check that no reassignment sneaks in. I'd fold them into one block (build the clone and set b.partitionSpecIndex.shared = true together). The snapshotIndex clone right below has the same shape, so worth aligning both while we're here.

[laskoviymishka]

ensurePartitionSpecIndex() calls buildPartitionSpecIndex, which always returns a non-nil &partitionSpecIndexData{}, so b.partitionSpecIndex is guaranteed non-nil right after it returns and this guard can't fail. I'd drop the check and set shared = true unconditionally, since a nil-guard that never fires makes the real nil invariants harder to trust.

[laskoviymishka]

When the index is stale this builds a fresh one into a local and throws it away, so N lookups against a stale builder each pay the full O(n) rebuild instead of paying it once. The same discard-the-rebuild pattern is in PartitionSpec and PartitionSpecByID.

It's deliberate: TestMetadataBuilderPartitionSpecIndexFallsBackAfterSliceReplacement asserts assert.Same on the original index, so persisting would break that invariant. But GetSpecByID sits on hot paths (SetDefaultSpecID, the AddPartitionSpec dup check), so I'd either call ensurePartitionSpecIndex() to persist and relax that test, or drop a comment saying we intentionally don't persist on the fixture-replaced path. wdyt?

[laskoviymishka]

The benchmark bottoms out at 4 specs and tops out at 2,048, but real tables rarely get past single-digit spec counts: the spec list only grows on intentional partition evolution, not per write. At N of 1-4 a map lookup with its hash and allocation can lose to a plain slice scan, so the interesting crossover is exactly the regime this skips.

I'd add N=1 and N=4 cases and report whether the index actually wins there. If it doesn't, it'd be worth gating the index behind a len(specs) threshold so small tables keep the cheaper scan, since the machinery here (pointer-identity check, COW flag, rebuild fallback) is a fair bit of surface to carry if the payoff only shows up at spec counts tables don't reach.

[laskoviymishka]

These two concurrent tests don't actually exercise the copy-on-write path they look like they're guarding.

Every goroutine only reads from an index that's built once and never mutated: the positions map is written before any goroutine starts and shared/firstSpec are never touched, so the Go memory model already makes these reads safe. A -race run here would pass even if we deleted the shared flag and the whole clone-on-write dance.

The race that matters is a builder marking its index shared in Build(), then AddPartitionSpec triggering ensurePartitionSpecIndexMutable (which clones) while another goroutine reads the metadata built before the mutation. I'd rework one of these to spin up a reader on the built commonMetadata and concurrently call builder.AddPartitionSpec on the builder that produced it, which is what proves the isolation holds under -race. If the intent is only "concurrent reads after init are safe," a comment saying so would keep the next reader from trusting it for more than it tests.

[zeroshade]

Thanks for addressing the earlier review points — the stale-slice fallback, copy-on-write race coverage, realistic threshold benchmarks, and cleanup all look resolved now. I found one new concurrency issue that still needs fixing before merge.

Duplicate partition-spec IDs make read-only metadata lookups race. In partitionSpecIndexNeedsRebuild, len(index.positions) counts unique IDs, while len(specs) counts every spec. Since checkPartitionSpecs currently accepts duplicate IDs, such metadata permanently looks stale. Every PartitionSpecByID/PartitionSpec call then rebuilds and assigns c.partitionSpecIndex in ensurePartitionSpecIndex (around lines 190-195 and 2184-2187). Concurrent readers therefore race even though metadata lookups are otherwise read-only.

I reproduced this with 64 specs sharing ID 7 and eight goroutines repeatedly calling PartitionSpecByID(7) under -race: this head fails with writes at metadata.go:2186 racing reads at metadata.go:191; the identical probe passes on the merge base.

Please either reject duplicate partition-spec IDs during validation, or track the indexed source count independently of the unique-ID map cardinality so the index becomes stable. A concurrent duplicate-ID regression test would pin the fix.

[laskoviymishka]

Nice, this is close. Everything from my last pass landed, and the duplicate-ID race @zeroshade caught is genuinely closed now (the sourceCount field plus TestCommonMetadataPartitionSpecIndexDuplicateIDsConcurrent, which fails on the old code under -race and deterministically via assert.Same).

One open item before I approve, and it's really one design question. The race got fixed by tolerating duplicate spec IDs, but we reject those everywhere else: checkSchemas and checkSnapshots both bail on duplicate IDs, and Java's PartitionUtil.indexSpecs throws on them at load. So after this we'd silently read (first-one-wins) metadata that Java refuses to open, which is the kind of cross-client divergence that's painful to debug. I'd lean toward rejecting duplicate IDs in checkPartitionSpecs, matching the sibling checkers, matching Java, matching the spec's unique-spec-ID rule, instead of tolerating via sourceCount. It also makes the read-path rebuild in ensurePartitionSpecIndex unreachable, which folds in the one structural nit I left inline (we write c.partitionSpecIndex back on a read, where SnapshotByID rebuilds into a local). wdyt?

Everything I asked for last round is in:

• element-mutation fallback in partitionSpecIndexPosition, pinned by TestCommonMetadataPartitionSpecIndexFallsBackAfterElementMutation
• copy-on-write exercised under concurrency
• benchmarks across realistic single-digit spec counts
• the dead nil-guard and duplicated clone() block cleaned up

Settle the reject-vs-tolerate direction and I'm happy to approve.

[laskoviymishka]

The sourceCount fix does close the duplicate-ID race, but it leaves us tolerating something Java refuses. PartitionUtil.indexSpecs builds an ImmutableMap whose .build() throws on a duplicate spec ID, so Java won't even load metadata with duplicate IDs, whereas after this we'll happily read it and silently expose only the first spec for the dup'd id. That also diverges from our own checkSchemas/checkSnapshots, which both reject duplicate IDs, and from the spec's unique-spec-ID rule.

So rather than tolerating duplicates here via sourceCount, I'd lean toward rejecting them in checkPartitionSpecs: a seen set returning ErrInvalidMetadata, matching the two sibling checkers and matching Java. As a bonus it makes sourceCount != len(specs) unreachable for real metadata, which also takes the read-path rebuild off the table (see the ensurePartitionSpecIndex thread). sourceCount can stay for in-package fixture safety.

wdyt? Happy either way if you'd rather keep tolerating, but then I think we owe a comment here on why we accept what the sibling checkers and Java reject.

[laskoviymishka]

Not a change request, just noticing this stays O(n) on a clean miss, unlike snapshotIndexPosition which returns early. That's actually correct here: the scan is what makes the element-mutation fallback work, since a clean miss is indistinguishable from an in-place mutation that introduced a new id. Worth a one-liner saying so, so nobody "optimizes" the early return back in and reintroduces a false miss.

[laskoviymishka]

This is the one structural thing still bugging me, and it's the same root cause as the duplicate-ID race: we write c.partitionSpecIndex back from a read path. SnapshotByID deliberately doesn't; it rebuilds into a local and never touches the struct field, so two concurrent readers can't race on it.

sourceCount closed the duplicate-ID trigger, but the write-back is still reachable by any in-package fixture that swaps c.Specs and then reads concurrently (we already have ...FallsBackAfterSliceReplacement doing the swap). Mirroring SnapshotByID here (rebuild into a local, feed it to partitionSpecIndexPosition, don't assign the field) closes the class for good and makes this actually mirror snapshotIndex like the comment claims.

If we also reject duplicate IDs above, these two together make the rebuild branch unreachable in production entirely.

[laskoviymishka]

Small one tied to the sourceCount change: this asserts len(positions) == len(Specs), but sourceCount exists precisely to decouple those. For duplicate-ID metadata len(positions) < len(Specs) and this would fail even though the index is correct. I'd assert sourceCount instead: require.Equal(t, len(common.Specs), common.partitionSpecIndex.sourceCount).

[laskoviymishka]

Following up on the newly-rebased-in schema commit (fix(schema): avoid copying lazy caches during JSON encoding) since my last review.

The fix is correct and a nice catch. Dropping the *(*Alias)(s) copy gets rid of a real atomic.Pointer copy-after-use hazard, and the JSON stays byte-identical (the nil-to-[]int{} coercion is preserved), so I'm happy with it on its own merits. Two small, non-blocking test/robustness notes inline.

One aside: that commit is really independent of the spec-index work (different subsystem, separate bisect range), so ideally it'd land as its own PR. Not a blocker, it's clean enough to go on its own; it just keeps each change easy to revert in isolation.

Otherwise my open item from the last review stands: the reject-vs-tolerate call on duplicate spec IDs. Settle that and I'm happy to approve; this is close.

[laskoviymishka]

This is the right fix. The old *(*Alias)(s) copied the five atomic.Pointer cache fields by value, which is the copy-after-use case go vet flags, and the selective literal keeps the JSON byte-identical (only schema-id and identifier-field-ids are exported off Alias, and the nil-to-[]int{} coercion is preserved).

The one thing I'd guard: the named literal is point-in-time. If a new exported JSON-tagged field gets added to Schema later, marshaling silently zeroes it, with no compiler error and no failing test. I'd drop a one-line comment above it noting only ID and IdentifierFieldIDs are exported here, so anyone adding a field knows to list it. Non-blocking.

[laskoviymishka]

The test always builds with nil identifier IDs, so the non-nil ids branch in MarshalJSON never actually gets raced. I'd add a parallel case with something like []int{1} and assert the encoded JSON carries "identifier-field-ids":[1].

Related: the closing assert.Nil(t, schema.IdentifierFieldIDs) passes on both old and new code (the old copy mutated aliasCopy, never *s), so it doesn't actually guard this fix; -race is what catches the regression. I'd either drop it or leave a one-line note that the real invariant is that marshaling mustn't mutate the receiver. wdyt?

[zeroshade]

The duplicate-ID read race I raised on 2026-08-28 is properly resolved in this revision:

• sourceCount tracks the source slice length independently of unique map cardinality, so a duplicate-ID slice no longer produces a len(map) mismatch.
• partitionSpecIndexForLookup (table/metadata.go:2286) rebuilds a stale index into a local and never writes back to commonMetadata, which removes the concurrent-write path on read-only metadata.
• checkPartitionSpecs rejects duplicate IDs outright at table/metadata.go:2714.

Both paths are pinned by tests: TestCommonMetadataPartitionSpecIndexDuplicateIDsConcurrent (table/partition_spec_index_test.go:331), which fails on the old code under -race, and TestRejectsDuplicatePartitionSpecIDs (:144) for persisted-metadata validation.

@laskoviymishka's standing review asked whether duplicate spec IDs should be rejected or tolerated. This revision answers that by rejecting them with ErrInvalidMetadata at table/metadata.go:2714, with the latter test proving the behaviour — that resolves the open design question, so I'm approving rather than holding this further. @laskoviymishka, please clear your review when you get a chance; flag it if you disagree with reject-over-tolerate and I'll reopen the discussion.

On the rest of the change:

• The exported Metadata and MetadataBuilder API is unchanged. The index is private derived state and is excluded from JSON.
• Metadata getters retain their defensive-copy behaviour, so public metadata stays effectively immutable; copy-on-write applies only to internal builder-to-built sharing.
• The index is derived from the authoritative Specs list at deserialization and builder construction, and updated on spec add/remove.
• Unknown spec IDs still return ErrPartitionSpecNotFound rather than a zero value.

Benchmark evidence is included (metadata 2,048-spec last hit 2,528 → 74 ns/op; builder 54.9 µs → 33.3 ns/op), and small slices keep linear scans, which avoids pessimising the common small-spec case. CI is green.

---

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]

The duplicate-ID read race is genuinely fixed — by two independent mechanisms, both of which I verified are live by breaking them. But the central fast path this PR exists for has zero test coverage, and the measured realistic-range effect is a net allocation regression.

Blocking — table/metadata.go:281: the map fast path and its fallback both survive deletion

partitionSpecIndexPosition only consults index.positions when len(specs) >= partitionSpecIndexMinSize (32). Every fixture in partition_spec_index_test.go uses 1–3 specs, except TestMetadataBuilderPartitionSpecIndexCopyOnWriteConcurrent (1,024, but it reads one ID and asserts only pointer isolation). Two mutations:

• Set partitionSpecIndexMinSize = 1<<40 — the index is never read anywhere, ever: ok github.com/apache/iceberg-go/table 6.389s. The entire optimization can be disabled without a single test failing.
• Delete the hit verification (i >= 0 && i < len(specs) && specs[i].ID() == id) and the miss-fallback scan: ok ... 7.341s.

That second one is exactly the change @laskoviymishka asked for in round 1 ("fall through to the scan on a miss"), and TestCommonMetadataPartitionSpecIndexFallsBackAfterElementMutation / ...FallsBackAfterSliceReplacement are the tests added to pin it. They pass under the mutation because at 2 specs they never reach the map.

A 40-spec probe does catch it:

--- FAIL: TestProbeElementMutationAboveThreshold
    Expected value not to be nil.
    // specs[7] = NewPartitionSpecID(999) in place; PartitionSpecByID(999) returned nil

On unmutated head that probe passes, so the code is right — only the tests are inert. Fix: lift at least the element-mutation test and one hit/miss lookup test above the threshold, ideally sizing the fixture from the constant (partitionSpecIndexMinSize + 8) so coverage can't silently drop below the gate again.

Major

1. table/metadata.go:241 — the index is built unconditionally but read only at ≥32 specs, so the realistic 1-spec table pays for it and gets nothing.

buildPartitionSpecIndex is called with no size gate from preValidate (:2800), MetadataBuilderFromBase (:567), NewMetadataBuilder (:453), AddPartitionSpec (:849), RemovePartitionSpecs (:1812), and buildCommonMetadata→ensurePartitionSpecIndex (:1464). ExampleTableMetadataV2 has exactly 1 partition spec — the realistic shape. Interleaved base-vs-head, allocation counts deterministic (±0%, p=0.002):

path	allocs/op base→head	B/op
ParseMetadataBytes (1 spec)	491 → 494 (+0.61%)	28.46 → 28.67 KiB
MetadataBuilderFromBase	58 → 61 (+5.17%)	5.031 → 5.242 KiB
builder.clone() (txn staging)	16 → 17 (+6.25%)	1.805 → 1.828 KiB
FromBase+Build round trip	86 → 89 (+3.49%)	8.617 → 8.829 KiB
PartitionSpecByID on parsed md	8 → 8 (~)	640 → 640 (identical)

Every parse, every builder construction and every transaction clone gets more expensive, while the lookup the index exists to accelerate is byte-for-byte identical at 1 spec. Gate construction on the same threshold (and make partitionSpecIndexNeedsRebuild threshold-aware), or drop the threshold entirely — correctness doesn't depend on it (setting it to 1 leaves the suite green).

2. table/metadata.go:289 — ~90% of the headline 2,048-spec win comes from the loop rewrite, not the index.

Three-way interleaved (base / head-with-map-disabled / head), 8 rounds, ABLookup/n=2048/last:

base   17.758µs ± 46%
nomap   1.683µs ± 60%   -90.53% vs base   (p=0.000)
head    0.190µs ± 59%   -88.72% vs nomap  (p=0.000)

base→nomap is only for _, s := range specs { s.ID() } → for i := range specs { specs[i].ID() } — no longer copying a 5-word iceberg.PartitionSpec per element. At realistic sizes that loop change is the whole measurable win:

• base→nomap: n=4 miss −69.6% (p=0.000); n=8 last −34.2% (p=0.021), miss −81.5% (p=0.000); n=16 last −52.1% (p=0.001), miss −86.3% (p=0.000)
• nomap→head at n=1/4/8/16: no significant difference in any of 12 cases (p ≥ 0.083); one case is slower with the map (n=4 first +63.5%, p=0.015)

Not a correctness objection — but a one-line loop change with none of partitionSpecIndexData, COW, rebuild fallback or threshold (~180 fewer lines) delivers all of the realistic-range benefit and 10.6× of the 93× at 2,048. Worth saying so in the description, and worth asking whether the index carries its weight.

3. table/metadata.go:231 — partitionSpecIndexMinSize = 32 isn't justified by any benchmark here. The grid {1,4,8,16,32,256,2048} measures head only, so it can't show a map-on/map-off crossover. Measured (nomap vs head, 8 rounds): n=32 last −15.0% (p=0.038), n=64 last −24.7% (p=0.010), n=256 last −48.2% (p=0.050).

Noise calibration matters here: the n=16 pair runs identical code in both binaries and still reported miss −32.97% (p=0.001), so this machine's false-significance floor is ≈33% — which puts the n=32 −15% inside the noise envelope. The first size at which I can demonstrate a real win is 64. Either measure the crossover or say in the comment that 32 is a conservative guess.

What checks out

Seven mutations run; the ones that should bite, do:

mutation	result
delete the duplicate-ID branch in checkPartitionSpecs	FAIL: TestRejectsDuplicatePartitionSpecIDs ✓
drop clone-when-shared in ensurePartitionSpecIndexMutable	WARNING: DATA RACE + 3 failures under -race -count=3 ✓
reintroduce read-path write-back in partitionSpecIndexForLookup	DATA RACE ×4 + FAIL: ...FallsBackAfterSliceReplacementConcurrent ✓

Concurrency probes (64 specs so the map path is live, 16 goroutines × 200 iterations × 3 lookups, plus a variant with a deliberately stale index forcing the recovery rebuild on every read): -race -count=5 clean, index pointer unchanged in both.

Answering the questions I raised explicitly: negative spec IDs never reach the new check — they're rejected during spec decode (spec ID must be non-negative: -5). Zero and duplicate-zero behave correctly. PartitionSpecByID is not confusable between missing and unpartitioned — nil for missing, verified alongside a legitimately zero-field spec. (PartitionSpec() still falls back to clonePartitionSpec(*iceberg.UnpartitionedSpec) whose ID is 0, so that accessor can't distinguish — pre-existing, and now unreachable for validated metadata since checkPartitionSpecs requires the default to be present.)

Build/gofmt clean; golangci-lint 0 issues; go test ./table -count=1 ok; CI 15/15.

Minor

• :267 — partitionSpecIndexNeedsRebuild uses len(index.positions) != len(specs) while sibling schemaIndexNeedsRebuild (:194) uses index.sourceCount != len(schemas). Dropping sourceCount is correct now that duplicates are rejected, but one line saying why (dups rejected ⇒ cardinality == length) stops someone "aligning" them back.
• :2834 — new checkPartitionSpecs uses for _, spec := range c.Specs, copying a PartitionSpec per element, while buildPartitionSpecIndex uses for i := range specs specifically to avoid that. @laskoviymishka's round-1 inline asked for the latter; match it here.
• :2832 — seen map allocated on every validation even for a 1-spec table. checkSchemas does the same so it's consistent, but it contributes to the +3 allocs/parse above.
• Error precedence changed: metadata with both duplicate spec IDs and an unresolvable default-spec-id now reports duplicate partition spec ID 1 where base reported default-spec-id 77 can't be found. Same sentinel, no API impact.

Prior items

Mine (CHANGES_REQUESTED 2026-08-28) — duplicate-ID read race: Fixed, by two independent mechanisms, both verified live (above).

One caveat worth flagging: the head commit deleted TestCommonMetadataPartitionSpecIndexDuplicateIDsConcurrent, the regression test I asked for. That's defensible — its trigger can no longer reach commonMetadata from either entry point, and the residual class (stale index + concurrent readers) is still pinned by ...FallsBackAfterSliceReplacementConcurrent, which the write-back mutation proves has bite. But the removal isn't mentioned anywhere.

Mine (APPROVED 2026-09-01): N.A. / superseded. Head commit 5b133f18 landed 6.5 h after that approval and removed sourceCount; the approval's citations (:2286, :2714, sourceCount) no longer resolve. Everything has been re-verified against current head.

@laskoviymishka round 1: element-wise-mutation fallback → Partially fixed (code is right, tests are inert — Blocking). COW concurrency test → Fixed, mutation-verified. Confirm the win at single-digit counts or gate it → Partially fixed: gated at 32 and n=1/4/8/16 added to the grid, but no small-N numbers reported, and the gate makes the machinery pure overhead at those sizes. Dead nil-guard + duplicated clone() block → Fixed. for i, spec := range style → Fixed in buildPartitionSpecIndex, regressed in the new checkPartitionSpecs. Persist-vs-discard the rebuild → Fixed, the other way, with a comment explaining it. sourceCount assertion → N.A. Comment that the O(n) miss is deliberate → Fixed. Reject-vs-tolerate duplicates → Fixed, and settled correctly: reject, matching checkSchemas/checkSnapshots, Java's PartitionUtil.indexSpecs, and the spec's unique-spec-ID rule. That was the open design question across three reviews.

Description

• "Includes fallback coverage for stale in-package slices and read-only concurrent lookups" is overstated — those tests sit below the threshold and never exercise the map path; two separate mutations leave the suite green.
• Benchmark table is not stale. The hardware-independent figures reproduce exactly at head: specs=2048/last → 48 B/op, 1 allocs/op; /miss → 72 B/op, 3 allocs/op. The last two commits can't have invalidated it (the benchmarks pre-build the index outside the loop).
• Omissions: the partitionSpecIndexMinSize = 32 threshold is never named (only "Small slices use the existing linear-scan path"); no small-N numbers despite being asked twice; no mention of the for _, s := range → for i := range rewrite that supplies most of the measured win; no mention that preValidate, MetadataBuilderFromBase, clone() and Build() now allocate more; no mention that a regression test was removed.
• Accurate: exported API unchanged, defensive-copy behaviour preserved, index updated on add/remove with COW, duplicate rejection during validation — all mutation-verified.

---

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]

Prior blocking (inert tests below the 32-spec gate) and prior allocation-regression findings are both genuinely fixed and mutation-verified; only tuning/dead-code minors remain, chief among them that the 32 threshold buys nothing on hits and costs 32% on misses at exactly 32 specs.

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

• not fixed — laskoviymishka round 1 + zeroshade major: threshold 32 isn't justified by any benchmark; measure the crossover or say it's a guess
• not fixed — zeroshade major: ~90% of the headline 2,048-spec win comes from the for _, s := range -> for i := range loop rewrite, not the index; worth saying so in the description
• partially fixed — laskoviymishka round 1: the two concurrent tests don't exercise the copy-on-write path they look like they're guarding

Verification performed

In worktree .pi-worktrees/pr1910 at faafebd: 'go build ./...' PASS; 'go vet ./table' PASS; 'gofmt -l' on all 4 changed files clean; 'go test ./table -count=1 -timeout=15m' PASS (ok 5.374s); 'go test ./table -count=3 -race -run Test.*PartitionSpec|TestRejectsDuplicatePartitionSpecIDs|TestMetadataBuilderClone' PASS (ok 2.361s); 4 throwaway probes in table/pr1910_probe_test.go under -race -count=3 PASS (ok 2.037s) covering the Transaction.apply clone-vs-reader race, a 29->35->27->33 threshold boundary walk, clone-then-diverge across the threshold, and the concurrent-fixture nil-index check; 6 mutations M1-M6 all caught (M6 under -race); 1 panic-mutation confirming the AddPartitionSpec nil sub-guards are unreachable across the whole suite; base-vs-head AllocsPerRun and 3-way benchstat A/B (base aa76a28 / head-nomap / head) at n=1,4,8,16,32,64,256,2048. 'go test ./catalog/rest' PASS (the pi-lens setup failure was my own temporary file moves during the A/B runs). CI on the PR: 15/15 green. Worktree restored to a clean 'git status'; golangci-lint crashed locally on a Go toolchain mismatch (built with 1.26.5, worktree on 1.27.0), unrelated to this PR.

---

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]

minor — partitionSpecIndexMinSize = 32 is not supported by the benchmarks its own comment cites

The comment says '32 is a conservative cutoff; keep it aligned with the lookup benchmarks', but at exactly 32 specs the map gives no measurable hit benefit and makes misses materially slower (a miss pays the map probe and then the full scan, by design). The first size where hits demonstrably win is 64. Either raise the constant to 64 or drop the claim that it tracks the benchmarks. No correctness impact and no regression versus main - head beats base at every size >= 32.

[zeroshade]

minor — Dead nil sub-guards in AddPartitionSpec's incremental index update

'b.partitionSpecIndex == nil || b.partitionSpecIndex.positions == nil' cannot hold when len(b.specs) > partitionSpecIndexMinSize: ensurePartitionSpecIndexMutable() two lines above calls ensurePartitionSpecIndex(), and for any pre-append length >= 32 partitionSpecIndexNeedsRebuild returns true for both a nil index and a nil positions map, so buildPartitionSpecIndex has already installed a non-nil index with a non-nil map. Reduce the condition to 'len(b.specs) == partitionSpecIndexMinSize'. This is the same class of never-firing guard already removed twice in this PR at laskoviymishka's request.

[zeroshade]

minor — TestCommonMetadataPartitionSpecLookupsConcurrent exercises none of the new index machinery

The fixture is partitionSpecIndexTestSpecs(1, 2), which is below the construction gate, so metadata.partitionSpecIndex is nil, partitionSpecIndexForLookup returns immediately without rebuilding, and partitionSpecIndexPosition's map branch is skipped. The test only proves concurrent linear scans are safe - which the Go memory model already gives for free. This is the one test from laskoviymishka's round-1 comment that wasn't lifted above the threshold (its sibling ...CopyOnWriteConcurrent was, and has bite). Size it from partitionSpecIndexMinSize like the other fixtures, or rename/comment it to say it covers unindexed concurrent reads.

[zeroshade]

nit — Unreachable sub-expressions in the new helpers

At :285 'len(specs) > 0 &&' can never be false - partitionSpecIndexNeedsRebuild already returned for len(specs) < partitionSpecIndexMinSize. At :296 'i >= 0' is dead for map-sourced positions. Both shapes are inherited from snapshotIndexNeedsRebuild (:109) and snapshotIndexPosition (:118), where the length guard IS live because the snapshot index has no size gate; keeping them here is defensible for symmetry but a one-line note would stop a reader inferring the gate doesn't exist.

[zeroshade]

nit — Half of TestMetadataBuilderRemoveUnknownPartitionSpecKeepsIndex is a nil-vs-nil assertion

builderWithoutChanges(2) has 1 spec, so originalIndex and builder.partitionSpecIndex are both nil and assert.Same compares nil to nil. The following assert.Same(&builder.specs[0]) is the assertion carrying the test's meaning. Consider dropping the index assertion or moving the fixture above the threshold.

[zeroshade]

nit — Single-spec lookups are ~0.5 ns slower than base

The extra partitionSpecIndexForLookup + partitionSpecIndexNeedsRebuild calls cost about half a nanosecond on the most common table shape. Statistically strong but practically irrelevant next to the 96 B / 2 allocs of the defensive clonePartitionSpec on the hit path. Noted only so the description's framing doesn't imply a strict improvement everywhere; no action needed.

[zeroshade]

nit — Description omits the threshold, the loop rewrite, and the new load-time hard failure

Three gaps worth closing before merge: (a) the 32 threshold - and the fact the index is not even built below it - is never named, only 'Small slices use the existing linear-scan path'; (b) no mention of the 'for _, s := range' -> 'for i := range' rewrite, which now supplies 100% of the win for realistic single-digit spec counts; (c) rejecting duplicate partition spec IDs turns previously-loadable metadata into a hard ErrInvalidMetadata at parse and at Build - correct and matching Java per the settled design discussion, but a user-visible compatibility change that deserves a release note. Error precedence also changed within the same sentinel.

[zeroshade]

The delta's metadata.go refactor is behaviour-preserving and mutation-pinned, but its only new test is vacuous — it passes with the hit-verification it names deleted, because the replacement spec is fieldless and so unpartitioned either way — and all three of my prior minors are untouched.

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

• not fixed — zeroshade minor: partitionSpecIndexMinSize=32 unsupported by its own cited benchmarks (no hit benefit at 32, ~32% worse on misses, first win at 64)
• not fixed — zeroshade minor: dead nil sub-guards in AddPartitionSpec incremental index update
• not fixed — zeroshade minor: TestCommonMetadataPartitionSpecLookupsConcurrent fixture sits below the gate, exercising no index machinery
• partially fixed — zeroshade nit (:286): unreachable len(specs) > 0 && sub-expression in partitionSpecIndexNeedsRebuild

Verification performed

In worktree .pi-worktrees/pr1910 at 5ea88a7: 'go build ./...' PASS; 'go vet ./table' PASS; 'gofmt -l' on both changed files clean; 'go test ./table -count=1 -timeout=10m' PASS (ok 5.426s); 'go test ./table -race -count=2 -run PartitionSpec|TestRejectsDuplicatePartitionSpecIDs' PASS (ok 2.179s). Mutation M1 (delete hit verification in partitionSpecIndexPosition): delta test PASSES (vacuous), sibling :185 FAILS. Mutation M2 (replace the refactored :285 firstSpec comparison with `return false`): TestMetadataBuilderPartitionSpecIndexFallsBackAfterSliceReplacement FAILS, confirming the refactored line is live and pinned. One throwaway probe (table/pr1910_probe_test.go) run under both mutated and unmutated head, then deleted. metadata.go restored from backup after each mutation; final 'git status --porcelain' empty and ./table builds clean.

---

This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg Go maintainer. The findings below are observations, not blockers; an Apache Iceberg Go maintainer — a real person — will take the next look at the PR. If you think a finding is mis-applied, please reply on the PR and a maintainer will weigh in.

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

[zeroshade]

major — New stale-default test is vacuous — passes with the mechanism it names deleted

The delta's only substantive addition claims to assert that PartitionSpec() 'must not return a stale indexed value' after an in-place mutation. The fixture makes that undecidable: partitionSpecIndexTestSpecsAtLeast(40, 1, 2) yields specs[7].ID()==1005, and the mutation writes iceberg.NewPartitionSpecID(3) — a fieldless spec, so IsUnpartitioned() is true for BOTH the stale indexed hit (ID 3) and the correct unpartitioned fallback (ID 0). The assertion therefore cannot fail. This matters because PartitionSpec()'s stale-default path above the 32-spec gate is exercised by no other test, so the coverage gap the test was added to close is still open. It is also the same inert-test class as my prior blocking finding, which this commit was written to answer. The production code is correct — only the test is inert. Fix: assert identity, e.g. assert.Equal(t, iceberg.UnpartitionedSpec.ID(), metadata.PartitionSpec().ID()).

Evidence

Mutation (delete the `i >= 0 && i < len(specs) && specs[i].ID() == id` hit verification in partitionSpecIndexPosition, table/metadata.go:295-297), run per-test:
  --- PASS: TestCommonMetadataPartitionSpecIndexFallsBackAfterDefaultElementMutation (0.00s)  <- delta test survives
  --- FAIL: TestCommonMetadataPartitionSpecIndexFallsBackAfterElementMutation
      partition_spec_index_test.go:185: Expected nil, but got: &iceberg.PartitionSpec{id:3, fields:[]iceberg.PartitionField{}, ...}  <- sibling has bite
Identity probe (table/pr1910_probe_test.go, since removed), same fixture:
  under mutation: 'DefaultSpecID = 1005' / 'PartitionSpec() -> ID=3 IsUnpartitioned=true ; UnpartitionedSpec.ID=0' -> FAIL
  on unmutated 5ea88a7: 'PartitionSpec() -> ID=0 IsUnpartitioned=true' -> PASS
So an ID assertion distinguishes the two states; IsUnpartitioned() cannot.