perf(table): borrow manifest partition summaries

[fallintoplace]

What

• Add a trusted borrowed view for built-in manifest partition summaries.
• Use the borrowed view in manifestEvalVisitor.Eval.
• Fall back to ManifestFile.Partitions() for external implementations.
• Keep the public ManifestFile.Partitions() defensive-copy behavior unchanged.
• Add focused tests and benchmarks.

Why

ManifestFile.Partitions() copies the full summary slice, every lower and upper bound, and every ContainsNaN pointer. The manifest evaluator only reads these values during the current evaluation, so those copies are unnecessary on the built-in manifest path.

Benchmark

Command:

go test ./table -run '^$' -bench '^BenchmarkManifestEvaluatorBuiltInPartitions/manifests=1000/fields=32$' -benchmem -benchtime=200ms -count=5

Median of 5 runs on an Apple M1 Pro with Go 1.26.3:

Case	Before	After
1,000 manifests, 32 fields	4.97 ms/op, 4.27 MB/op, 194,000 allocs/op	2.14 ms/op, 816 KB/op, 33,000 allocs/op

This is about 57% lower latency, 81% fewer bytes, and 83% fewer allocations in this wide-manifest evaluator workload.

The benchmark also covers 1, 100, and 1,000 manifests with 1, 8, and 32 partition fields.

Testing

• go test ./table -count=1 -timeout=5m
• go test ./table -race -run '^(TestManifestFilePartitions|TestManifestEvaluator|TestManifestEvalVisitorEvalRace)' -count=1 -timeout=5m
• go test ./... -run '^$' -count=1
• go vet ./table

[laskoviymishka]

This is a clean optimization and the capability-token shape is exactly right: the zero-size internal.ManifestFileRef gating the borrow, the per-call ev copy that keeps the borrowed slice safe under concurrent scans, and the graceful fallback to public Partitions() for external ManifestFile implementations. I walked the borrow lifetime through the Visit* methods and I'm satisfied the read-only/no-retain contract holds, so the change itself is correct.

I'd fix couple things before merge, mostly on the tests. The zero-alloc assertion uses InDelta(0.0, allocs, 0.5), which passes even if the path allocates 50 times per 100 runs. That's the single guarantee this PR exists to make, and CI wouldn't catch a regression in it today. The borrowed-view test also checks values and alloc count but never proves the view actually aliases the manifest, so a shallow-copy refactor would pass it silently. Those two are the guard rails for the thing we're optimizing, so I'd tighten both here.

Separately, the PR description mentions a TestManifestEvalVisitorEvalRace, but I don't see it in the diff. Since the borrow's safety under parallel scans rests entirely on the per-call ev copy, I'd like that race test actually present so the concurrency claim is verified by CI rather than by reading.

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

• make the zero-alloc test assert exactly zero (assert.Zero), not InDelta(0.5)
• add an aliasing assertion that mutates through a borrowed bound and shows the manifest sees it
• either include the race test the description references, or drop the reference
• move the dispatch helper into internal alongside the DataFileRef borrow helpers so other packages can reuse it
• give each benchmark summary its own bounds instead of sharing three pointers
• rename manifest_file_refs.go to the singular manifest_file_ref.go to match its siblings

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

[laskoviymishka]

The read-only/no-retain contract lives on manifestFilePartitions, but "trusted packages within this module" is really any in-module package, and any of them can construct this token and call the method directly, bypassing the helper.

Since the token is the thing a caller holds, I'd repeat the invariant here: borrowed, read-only, don't retain past the current operation. That way the contract travels with the capability rather than living only on one entry point.

[laskoviymishka]

This returns the same nil/empty result as Partitions() only because of an invariant that isn't written down anywhere: when PartitionList is non-nil it always points at a non-nil slice (the builder never stores &nilSlice, and the Avro path normalizes through ensurePartitionList).

If that ever stops holding, the borrowed path diverges from Partitions() silently. I'd add a one-line comment stating the invariant here so a future change doesn't break it unknowingly.

[laskoviymishka]

These two report static dimensions, so the output reads "100 manifests/op" where the /op implies a per-iteration rate that isn't one. The counts are already in the sub-benchmark name (manifests=100/fields=8), so I'd just drop both ReportMetric calls.

[laskoviymishka]

Every summary here shares the same three pointers (&containsNaN, &lower, &upper). It's safe today only because ManifestBuilder.Partitions() deep-clones each entry via cloneFieldSummaries. But this PR is specifically about borrowing those bounds without copying, so a fixture that aliases across elements is exactly the footgun the borrowed path could trip on if this pattern gets copied into a test that skips the builder. I'd give each element its own copy inside the loop (fresh bool, slices.Clone the bounds).

[laskoviymishka]

The DataFileRef pattern already merged keeps all these borrow helpers in internal (BorrowedDataFileStats, BorrowedDataFileBounds, and friends), so any trusted package in the module can reach zero-copy access without redeclaring the interface. This one lives in table, which means the next caller that wants borrowed partition summaries (scan planning, metrics) has to redeclare the interface and dispatch from scratch. I'd move it to internal as BorrowedManifestFilePartitions(ManifestFile) []FieldSummary following that model, at which point this file collapses to a one-line call.

While we're here, the unexported interface manifestFilePartitionRef and its method ManifestFilePartitionRef differ only by the leading case, which is easy to misread at a glance. Something like manifestPartitionBorrower would read more clearly. wdyt?

[laskoviymishka]

This proves the values are right and (below) that the call doesn't allocate, but it never proves the view actually aliases the manifest. A future refactor that returned a shallow copy sharing the LowerBound pointers, or a pooled deep copy, would pass this test unchanged, so it doesn't distinguish "truly borrowed" from "cheap but not aliasing", which is the property the whole PR turns on.

I'd add an assertion that mutates through the borrowed pointer and shows the manifest sees it: grab partitions[0].LowerBound, write to (*partitions[0].LowerBound)[0], then read the manifest's stored bound back and assert it changed. Label it as the aliasing hazard callers must not trigger, so it documents the contract and locks in the borrow at the same time.

[laskoviymishka]

AllocsPerRun(100, ...) returns total allocations divided by 100, so InDelta(0.0, allocs, 0.5) passes with up to 50 allocations across the run. A regression that allocated on every other call would sail straight through it.

Since a genuine zero-alloc path returns exactly 0.0, I'd assert that directly:

assert.Zero(t, allocs)

This is the guarantee the test exists to make, so it's worth making it exact.