fix(table): return defensive metadata getter copies

[fallintoplace]

Summary

Return defensive copies from table metadata getters so callers cannot mutate persisted metadata through returned maps, slices, pointers, or nested values.

The clones cover schemas and defaults, partition specs, snapshots and refs, sort orders, statistics/blob metadata, partition statistics, encryption keys, and properties. Nil slice behavior is preserved.

Testing

• go test ./table ./cmd/iceberg
• regression coverage mutates every returned getter value—including binary defaults, source ID slices, blob metadata, and encryption properties—and verifies the underlying metadata remains unchanged

[zeroshade]

Good direction on defensive copies for the metadata getters. Two of them are still shallow, which defeats the protection for those types (inline).

[zeroshade]

slices.Clone copies the outer slice but not the *iceberg.Schema pointees, so callers still receive the original Schema objects and can mutate exported fields like ID/IdentifierFieldIDs, corrupting metadata state. To actually protect it, clone each schema (or return immutable copies).

[zeroshade]

Same shallow-copy issue: slices.Clone(c.Specs) copies the PartitionSpec structs but they still share their internal fields slice and each field's SourceIDs backing array, so specs[i].Field(j).SourceIDs[k] = ... mutates the original. A deep clone of each spec (fields + SourceIDs, rebuilding derived maps) is needed here — you already do the SourceIDs clone in the builder clone() around line 1722.

[laskoviymishka]

Thanks for this — defending the metadata getters with copies is the right call, and the SnapshotRef/Snapshot/Properties cases look solid.

I'd hold it before merging, though. There's still one shallow copy live: cloneNestedFields deep-copies each field's Type but leaves InitialDefault/WriteDefault as shared any values, and those can hold a []byte (BinaryLiteral/FixedLiteral) — so a caller can still mutate through schema.Fields()[n].WriteDefault back into the original. I think that's the same class zeroshade flagged in the still-open shallow-copy thread, so worth closing them together with a small cloneDefault helper.

The other thing is the new test. The SourceIDs assertions don't actually exercise the clones they're guarding — PartitionField.SourceIDs and SortField.SourceIDs are json:"-", so the final JSONEq never observes them, and a few of the mutations run against value copies. As written the suite passes whether or not those SourceIDs clones exist, which leaves the regression net with a hole right where it counts. I'd assert slice-distinctness directly instead of leaning on the JSON round-trip.

A few things I'd want before merge:

• deep-copy InitialDefault/WriteDefault in cloneNestedFields
• make the SourceIDs / SortOrder test assertions actually fail if the clone is removed
• decide Statistics()/PartitionStatistics()/EncryptionKeys() — clone them too, or document them as uncopied and defer

The nil→non-nil slice change and the cold-cache cost on cloneSchema are worth a look too, but they're not blockers. Once the copy gap and the test are sorted, happy to take another pass.

[laskoviymishka]

While we're clamping down here, the getters just below still hand out shared internal state — Statistics(), PartitionStatistics() and EncryptionKeys() yield value copies of the top-level structs, but BlobMetadata.Properties/Fields and EncryptionKey.Properties are still the internal maps/slices. metadata.Statistics()...BlobMetadata[0].Properties["x"] = "y" mutates the real StatisticsList.

Same class as what this PR fixes, on the same interface. Either deep-clone these too, or explicitly document which getters guarantee copies and defer the rest — either's fine, but I'd rather not ship an interface where half the getters are safe and half aren't, silently.

[laskoviymishka]

Worth a thought on the hot path: cloning through NewSchemaWithIdentifiers runs init(), so every clone comes back with cold lazy caches (idToName, nameToID, etc.). CurrentSchema()/Schemas() get hit per-file during scan planning, so this is a fair bit of re-derivation and allocation on large tables. (Also schema.Fields() already returns a slices.Clone, so cloneNestedFields clones the top-level slice a second time.)

Not blocking, but I'd consider either caching the clone or shallow-cloning while sharing the atomic cache pointers and copying only the mutable exported fields (ID, IdentifierFieldIDs). wdyt?

[laskoviymishka]

One behavior change to watch: make([]T, len(x)) turns a nil SchemaList into a non-nil empty slice, so Schemas() now returns [] where it used to return nil (and marshals to [] instead of null). Same for clonePartitionSpecs/cloneSortOrders/cloneSnapshots. Any caller using == nil to mean "unset", or comparing JSON round-trips, will shift. I'd guard each with if x == nil { return nil }.

[laskoviymishka]

I think there's still a shallow copy hiding here. We deep-copy Type but leave InitialDefault and WriteDefault as shared any values — and those can hold a []byte (BinaryLiteral/FixedLiteral), so a caller can do schema.Fields()[0].WriteDefault.([]byte)[0] = 0xFF and reach right back into the original.

This looks like the class zeroshade was pointing at in the shallow-copy thread. I'd add a small cloneDefault(v any) any that slices.Clones the []byte-backed cases and passes everything else through, then apply it to both defaults here.

[laskoviymishka]

Minor: this slices.Clone is redundant — NewPartitionSpecID already clones SourceIDs when it builds the field (partitions.go:372), so we clone twice. Can drop the explicit one here.

[laskoviymishka]

This block doesn't actually guard the SourceIDs clone. PartitionField.SourceIDs is json:"-" (partitions.go:47), so the closing JSONEq never sees it change — and Field(0) returns a value copy, so partitionField.Name = "mutated" is a no-op too. The whole assertion would still pass if we deleted slices.Clone(fields[i].SourceIDs) from the clone helper. Same holds for the PartitionSpec() / PartitionSpecByID() blocks just below.

I'd assert the returned SourceIDs is a distinct backing slice directly — mutate it, then read metadata.Specs[0].Field(0).SourceIDs back (the test is in package table) and require it unchanged — rather than relying on the JSON round-trip.

[laskoviymishka]

Same dead-coverage issue here — Fields() yields SortField by value, so field.SourceIDs[0] = 99 mutates a loop-local copy, and SortField.SourceIDs is json:"-" on top of that. Nothing in this block fails if cloneSortOrder is removed. Since we're in package table, I'd reach into sortOrders[0].fields directly and mutate a serializable field (Direction/NullOrder), or compare the slice identity against the original.

[zeroshade]

Re-reviewed — the shallow-copy issue I flagged is addressed: Schemas() and PartitionSpecs() now deep-copy via cloneSchemas/clonePartitionSpecs (including each field's SourceIDs), so callers can't mutate the stored metadata. Thanks @fallintoplace!