perf(metadata): avoid re-cloning defensive schema copies

[fallintoplace]

Summary

This keeps the defensive copies, but avoids doing the same work more than once.

• Schema copies: reuse the deep copy already returned by Schema.Fields().
• Partition specs: add a cache-aware PartitionSpec.Clone() so fields are copied once and the source index is rebuilt.
• Sort orders: deep-copy mutable bucket and truncate transforms returned by metadata getters.
• Row-lineage and Hive paths: reuse one copied field slice instead of copying the schema again.
• Added regression tests for nested defaults, mutable transforms, partition-spec cloning, and row-lineage fields.

Testing

• go test ./table ./view
• go test ./catalog/hive ./table ./view
• go test -race ./table ./view
• go vet ./table ./view
• make lint with golangci-lint v2.12.2

Benchmark

• BenchmarkCloneSchemaWithNestedDefaults: about 3.0-3.8 us/op, 4320 B/op, 63 allocs/op to 1.82-1.84 us/op, 2696 B/op, 38 allocs/op.
• BenchmarkClonePartitionSpecs with 32 fields: 108 to 75 allocs/op and 12,968 to 10,024 B/op in local runs.

[zeroshade]

Behavior is provably equivalent (Schema.Fields() already deep-copies), but the view-side deep-copy removal is pinned by a vacuous test and the PR's new partition/transform regression assertions do not bite under mutation.

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

Verification performed

go build ./...; go vet . ./table ./view ./catalog/hive; go test -count=1 . ./table ./view ./catalog/hive (all ok); go test -race -count=1 . ./table ./view ./catalog/hive (ok: iceberg 2.24s, table 19.81s, view 1.72s, hive 2.48s); 5 mutation probes on view/metadata.go:425, table/metadata.go:2407, table/metadata.go:2539, partitions.go:546 (shallow fields) and partitions.go:551 (drop initialize)

---

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 — View cloneSchema deep-copy guarantee is entirely unpinned; guarding test is vacuous

This PR removes view's local cloneNestedFields/cloneSchemaType and relies on Schema.Fields() being a deep copy. No view test verifies that. TestCloneSchemaCopiesNestedValues (view/metadata_test.go:712) mutates cloned.Field(i), but Schema.Field (schema.go:279) itself returns cloneField(...), so every mutation lands on a throwaway copy. A future change to view cloneSchema or to Fields() would silently alias view metadata's internal field slice with no test failure. Fix: mirror the table-side test and read the internal slice via FieldsRef(internal.SchemaRef{}) as TestMetadataSchemaGetterCopiesNestedValues already does.

Evidence

Mutated view/metadata.go:425 `schema.Fields()...` -> `schema.FieldsRef(iceint.SchemaRef{})...` (full aliasing); `go test -count=1 ./view` => `ok github.com/apache/iceberg-go/view 0.408s`. Same mutation on table/metadata.go:2407 correctly fails: `--- FAIL: TestMetadataSchemaGetterCopiesNestedValues` with diff `InitialDefault: 01 02 03 -> 63 02 03` and `Name: "list" -> "changed"`. Restored with git checkout; git status --porcelain empty.

[zeroshade]

minor — TestPartitionSpecCloneCopiesFields does not test that Clone copies fields

The test mutates clone.Field(0), but PartitionSpec.Field (partitions.go:757) already returns clonePartitionField(...), so the mutation never reaches clone.fields. Only the trailing FieldsBySourceID(1) assertion is non-vacuous (it pins initialize()). To actually guard the deep copy, compare spec/clone after mutating through an accessor that returns internal state, or assert on the SourceIDs slice identity of the two specs' FieldsBySourceID results.

[zeroshade]

minor — New transform-mutation assertions in TestMetadataGettersReturnDefensiveCopies are vacuous

The added partitionField.Transform.(*iceberg.BucketTransform).NumBuckets = 32 (line 115) and the sort-order equivalent (line 166) read through PartitionSpec.Field() and SortOrder.Fields(), both of which already deep-copy the transform (partitions.go:757, table/sorting.go:328). The assertions therefore pass regardless of whether cloneSortOrder/PartitionSpec.Clone copy transforms, so they add no regression protection for the behavior the PR description claims they cover. (The cloneSortOrder guarantee itself is still pinned by TestMetadataBuilderFromBaseCopiesBuiltinMetadata, so this is coverage theater rather than a hole.)

[zeroshade]

nit — clonePartitionSpec is now a bare one-line pass-through

After this PR clonePartitionSpec(spec) is just return spec.Clone(). Five call sites could call spec.Clone() directly and drop the wrapper, or keep it if a doc comment explains the indirection.

[zeroshade]

nit — Benchmark helper builds a spec with duplicate partition field names

partitionSpecCloneBenchmarkFields assigns Name: "field" to all N fields, producing a spec UnmarshalJSON would reject and that no real table can have. Using strconv.Itoa(i) for the name (strconv is already imported) makes the benchmark measure a realistic spec, including distinct url.QueryEscape work in initialize().

[laskoviymishka]

Nice optimization, and the equivalence argument holds up: Schema.Fields() already deep-copies the whole field tree on every call, so the extra cloneNestedFields/cloneSchemaType traversal was genuinely redundant, and removing it in all five places is safe. The benchmarks are a good addition too.

I'd hold this before merging though, because the tests that are supposed to pin the removed defensive copies don't actually exercise them. TestPartitionSpecCloneCopiesFields and the new transform assertions in TestMetadataGettersReturnDefensiveCopies route every mutation through Field(0) / Fields(), which return fresh copies, then read the result back through the same accessors. So they pass for any implementation of Clone()/cloneSchema, including a no-op that returned the original. The suite is green, but for the wrong reason. Since these test files are in-package, the fix is to reach into the raw fields directly so a real sharing bug turns them red.

Separately, the cloneSortOrder change quietly closes a genuine bug: the old version left the Transform pointer aliased between the clone and the source, so a caller could mutate a *BucketTransform through a SortOrders() result and corrupt parent metadata. That's a real improvement worth stating in the description, and it's exactly what a non-vacuous sort-order assertion would guard.

A few things I'd want before merge:

• Make the partition-spec and getter transform assertions bite by mutating the clone's raw fields and asserting the source is unchanged (details inline).
• Add a view-side test equivalent to TestMetadataSchemaGetterCopiesNestedValues; the view cloneSchema removal currently has no non-vacuous guard.
• Note the cloneSortOrder aliasing fix in the description.
• Benchmark the pointer-cloning path (*BucketTransform), not just IdentityTransform.

Once the assertions actually exercise the isolation, happy to take another pass and approve.

[laskoviymishka]

This doesn't actually pin what it's meant to. clone.Field(0) returns a fresh clonePartitionField(...) copy, so mutating field.Transform and field.SourceIDs touches a throwaway, and the assertions read back through spec.Field(0) which copies again. The test would stay green even if Clone() were a no-op that returned the original.

Since this file is package iceberg, I'd reach into the raw fields so a real sharing bug turns it red:

cloneTransform, ok := clone.fields[0].Transform.(*iceberg.BucketTransform)
require.True(t, ok)
require.False(t, cloneTransform == transform, "Clone must not share the transform pointer")
cloneTransform.NumBuckets = 32
require.Equal(t, 16, spec.fields[0].Transform.(*iceberg.BucketTransform).NumBuckets)

Same idea for SourceIDs (mutate clone.fields[0].SourceIDs[0] and assert spec.fields[0].SourceIDs is unchanged). The comma-ok form is worth using on these so a fixture change surfaces as a failed assertion rather than a panic.

[laskoviymishka]

Same problem as the Clone test: partitionSpecs[0].Field(0) and sortOrders[0].Fields() (the loop lower down) both hand back fresh copies via clonePartitionField / cloneSortField, so the new NumBuckets = 32 mutations land on throwaways and the readbacks go through accessors that were never touched. Both new transform assertions pass regardless of whether the getters copy the transform pointer.

This test is package table, so it can read the raw fields directly. For the sort order that matters especially: asserting on metadata.SortOrderList[0].fields[0].Transform after mutating the returned copy would actually catch the pre-PR cloneSortOrder aliasing (see my note on cloneSortOrder), where the transform pointer was shared. I'd mutate the returned copy's transform and assert the raw source field is still 16 for both the partition and sort-order cases.

[laskoviymishka]

The table-side cloneSchema change now has a rigorous guard in TestMetadataSchemaGetterCopiesNestedValues, which reaches the clone's raw fields via FieldsRef and mutates nested defaults. This view-side removal is the same edit but gets no equivalent test; the existing TestCloneSchemaCopiesNestedValues reads through Field(0) on both sides, so it's vacuous for nested-type mutations in the same way.

Schema.Fields() is shared, so the behavior is covered transitively by the table test, but the view cloneSchema call chain isn't exercised at the unit level. I'd add a parallel view test mirroring the table one (FieldsRef on the cloned result) so this file has its own guard.

[laskoviymishka]

Clone() is the first exported clone method on a spec type here; the rest of the codebase surfaces isolation through per-getter copies (Field, FieldsBySourceID), not an explicit Clone. The only callers are the internal clonePartitionSpec wrapper and tests, so I'd either keep it unexported as clone(), or add a godoc line stating the full-independence guarantee, since exporting locks that contract in.

Minor while we're here: the value receiver copies the source struct (including the sourceIdToFields map header) on every call, and that copy is discarded once initialize() allocates a fresh map. Every other read and mutating method on PartitionSpec is a pointer receiver. A *PartitionSpec receiver would match the pattern and skip the struct copy, and the body doesn't need to change. wdyt?

[laskoviymishka]

Worth calling this out explicitly: the old cloneSortOrder only cloned SourceIDs and left the Transform pointer aliased between the clone and the source, so a caller mutating a *BucketTransform through a SortOrders() result could corrupt the parent metadata. cloneSortField closes that.

It's a real correctness fix riding along in a perf PR, so I'd note it in the description (or a short comment here) rather than leaving it silent. The hardened sort-order assertion I suggested in the getters test is what would pin it.

[laskoviymishka]

clonePartitionField's switch only allocates for *BucketTransform / *TruncateTransform; IdentityTransform{} falls straight through with no copy. So the benchmark measures the cheapest path and never touches the pointer-cloning branch that's the actual behavior change here. I'd swap in (or add a variant with) &iceberg.BucketTransform{NumBuckets: 16} so the numbers reflect the work the clone actually does.