fix(schema): protect field getters from mutation

[fallintoplace]

Summary

• deep-clone nested types returned by Field and Fields
• return independent values from FindFieldByID and FlatFields
• keep cached schema indexes private from caller mutation

This completes the immutable-schema behavior already used by AsStruct and by the Java and Rust implementations.

Testing

• go test . -count=1
• all repository packages except io/gocloud

[zeroshade]

Correctness is right and the coverage is real — reverting the source makes TestSchemaFieldGettersReturnDefensiveCopies fail on the WriteDefault leak, and FindFieldByName/FindTypeByID/etc. inherit the clone by delegating to FindFieldByID; cloneDefault matches the existing table.cloneDefault.

My concern is cost on read paths. FindFieldByID now deep-clones (nested Type + InitialDefault/WriteDefault) on every call, and arrowAccessor.FieldPartner (arrow_utils.go:754) + castIfNeeded (:937) call it for every field of every record batch out of ToRequestedSchema, needing only Name/Type. On a struct field with ~50 children I measured FindFieldByID go from ~7ns/0 allocs to ~1.2µs/4.9KB/2 allocs — a per-field-per-batch O(subtree) copy on the write path. Please add an internal non-cloning lookup for those call sites (the codebase already has this pattern with asStructRef), or post numbers showing it doesn't matter.

Also: the same aliasing hole is still open on exported IndexByID and Schema.NameMapping() — belong in the same pass or a follow-up. Not blocking correctness, but I'd like the hot-path cost addressed before merge.

[laskoviymishka]

Nice work — the correctness fix here is real (reverting the getters makes TestSchemaFieldGettersReturnDefensiveCopies fail on the WriteDefault leak), the defensive-copy coverage is genuine, and cloneDefault covers every default shape we actually produce and lines up with the existing table.cloneDefault. The response to Matt's hot-path concern also looks right to me — FindFieldByIDRef plus switching FieldPartner/castIfNeeded onto it is the clean way to keep the write path zero-alloc.

I'd hold before merge though, mostly around one thing: the "completes immutable-schema behavior" framing isn't quite true yet. The per-getter paths are locked down, but IndexByID still returns fields whose .Type aliases the live tree, NameMapping() hands back the memoized slice directly, and IdentifierFieldIDs is a public mutable slice. Matt already flagged the IndexByID/NameMapping side as same-pass-or-follow-up — I don't think all of it has to land here, but I'd like us to pick explicitly: which holes close in this PR, and which get a tracked issue, so we're not merging under a "complete" banner that has known gaps.

The other thing I'd tighten is FindFieldByIDRef itself, since it's the new surface that deliberately hands out a live reference. Right now the doc comment is the only guard against a future internal caller mutating through f.Type, and it doesn't actually say the Type tree is shared. Worth making that contract explicit (details inline).

A few things I'd like to settle before merge:

• Document the FindFieldByIDRef contract — the returned Type tree is shared, treat it as read-only.
• Decide this-PR vs. tracked follow-up for the IndexByID / NameMapping / IdentifierFieldIDs holes, and adjust the description if they're deferred.
• Document the cloneDefault default-branch invariant (everything reaching it is a value type).

The rest is small — a cloneField idiom nit, the three-way cloneDefault duplication (follow-up), and a couple of test tightenings. Once the contract doc and the scope call are settled, happy to take another pass and approve.

[laskoviymishka]

This mutate-a-copy-and-return shape has a small trap: cloneField(f) with the result dropped compiles fine and silently skips the clone, which is the exact leak we're closing here. A struct-literal return would make an omitted assignment impossible to write, and let the compiler catch a forgotten field while we're at it. Not blocking — wdyt?

[laskoviymishka]

This is now the third byte-for-byte copy of cloneDefault (table/metadata.go and view/metadata.go have the others). Not asking to fix it in this PR, but could we file a follow-up to hoist cloneDefault/cloneField/cloneType into one place? Right now a new default type has to be handled in three files or it silently aliases in two of them.

[laskoviymishka]

The default branch is correct today — everything that reaches it (int64, float64, string, bool, uuid.UUID, Decimal) is a value type, so the shallow return can't alias. That's a real invariant though, and it's invisible here. Could we add a short comment listing the expected types and noting each must be a value type or immutable reference? A future default backed by a pointer would otherwise slip through silently.

[laskoviymishka]

This is the one surface that deliberately hands back a live reference, so I'd make the contract explicit. The returned NestedField is a value copy, but f.Type is the same *StructType/*ListType/*MapType pointer the schema owns, so f.Type.(*StructType).FieldList[0].Name = "x" reaches straight through and corrupts the live schema.

The two callers today (FieldPartner, castIfNeeded) are read-only so they're fine, but the doc is the only thing guarding this, and right now it says "limited to internal callers" without saying the Type tree is shared. I'd spell it out: the returned field shares its Type pointer with the schema, and callers must treat Type and anything reachable from it as read-only.

While we're here — the Ref suffix doesn't really signal "skips clone-on-read" to a reader, but that's a naming taste call; the doc is the important part.

[laskoviymishka]

FindFieldByName, FindFieldByNameCaseInsensitive, and FindTypeByID all get their clone transitively by delegating to FindFieldByID, so they're correct today. But nothing exercises them directly — a refactor that short-circuits the delegation would still pass this test. Since assertIndependent already does the work, a couple more subtests through those finders would lock the contract cheaply. wdyt?

[laskoviymishka]

Two small things on this test. The zero-alloc check leans on field being read outside the lambda to stay live — a future escape-analysis change could elide the assignment and pass spuriously; runtime.KeepAlive(field) inside the closure pins it. And since this is the one path that intentionally aliases, a companion assertion that a write through the returned ref does reach the schema would document that contract right next to the defensive-copy test. Both optional.