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.
• 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	84 ns/op
Metadata, 2,048 specs, miss	2,443 ns/op	8.34 ns/op
Builder, 2,048 specs, last hit	54.9 us, 98,304 B/op, 2,048 allocs/op	33.8 ns/op, 48 B/op, 1 alloc/op
Builder, 2,048 specs, miss	56.5 us, 98,403 B/op, 2,051 allocs/op	138 ns/op, 72 B/op, 3 allocs/op

The metadata path still clones the returned spec. Builder misses still format the existing error. The lookup itself is now constant time.

Checks

• go test ./table -count=1 -timeout=5m
• go test ./table -race -run ^(TestCommonMetadataPartitionSpecIndex|TestParsedMetadataBuildsPartitionSpecIndex|TestMetadataBuilderPartitionSpecIndex|TestMetadataBuilderRemoveUnknownPartitionSpec|TestCommonMetadataPartitionSpecLookupsConcurrent|TestMetadataBuilderPartitionSpecLookupsConcurrent)$ -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.