perf(table): compile metrics modes once

[fallintoplace]

What changed

• Compile metrics modes once. Parse the default mode and column overrides when building the stats plan.
• Use a direct schema traversal. Avoid temporary visitor closures and slices that are not needed for this plan.
• Reduce map growth. Pre-size the stats plan and parsed override maps.
• Carry nesting state. Avoid rescanning every column name to decide whether a field is nested.
• Keep the existing behavior for invalid overrides that are not used by the schema.
• Add nested-schema coverage and a focused benchmark.

Why

The old code reparsed the default metrics mode for every primitive and variant field. It also built temporary values through the generic schema visitor. That becomes expensive for wide schemas.

The Java implementation parses these settings once in MetricsConfig. This change follows the same approach in Go and keeps the compiled modes with the stats-plan build.

Benchmark

Measured locally on an Apple M1 Pro with:

go test -run '^$' -bench '^BenchmarkComputeStatsPlan$' -benchmem -count=3 ./table

For a 10,000-field schema using the default mode:

• Time: 3.82 ms/op -> 0.70 ms/op
• Memory: 6.56 MB/op -> 1.31 MB/op
• Allocations: 50,089 allocs/op -> 35 allocs/op

With 1% column overrides, pre-sizing reduces allocations from 46 to 39 allocs/op.

Checks

• go test ./table
• go test -race ./table
• go test -run '^$' ./...
• go vet ./table
• golangci-lint run --timeout=10m

[zeroshade]

Optimization is behaviorally equivalent (verified by a 3000-schema differential probe against a verbatim reconstruction of the old path), but it strands the PreOrderSchemaVisitor methods as dead production code and silently detaches TestStatsTypes from computeStatsPlan.

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

Verification performed

Baseline (worktree, PR head c280bfa): `go build ./...` clean; `go vet ./table/ ./table/internal/` clean; `go test ./table/` ok 6.222s; `go test -race ./table/ -run 'TestStatsTypes|TestComputeStatsPlan|TestFileMetrics|TestRollingDataWriter'` ok 3.012s; `go test ./table/internal/` ok 0.475s; `go test -run '^$' -bench '^BenchmarkComputeStatsPlan$' -benchmem ./table/` reproduces the PR's numbers (10000/default: 833963 ns/op, 1312449 B/op, 35 allocs/op; one_percent_overrides: 39 allocs/op). Throwaway probe table/pr1991_probe_test.go (deleted): TestProbeDifferentialStatsPlan compared a verbatim reconstruction of the pre-PR collector+PreOrderVisit path against the new computeStatsPlan over 3000 seeded random schemas -> PASS, no plan or error divergence; TestProbeInvalidUsedOverride and TestProbeInvalidDefault -> PASS with identical error strings. Mutations (each reverted via git checkout): (A) collectStatsPlanField no-op -> TestStatsTypes PASSES, 26 other tests fail; (B) isNested forced false -> full ./table FAILs at TestFileMetrics/TestMetricsModeNonDefaultTrunc; (C) eager return on unused invalid override -> TestComputeStatsPlanIgnoresInvalidUnusedColumnMode FAILs; (D) drop map pre-sizing -> ./table still ok (perf hint only, as expected); (E) drop struct recursion -> TestComputeStatsPlanTraversesNestedFields FAILs.

---

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 — Visitor traversal is now dead production code; TestStatsTypes no longer guards computeStatsPlan

computeStatsPlan no longer calls iceberg.PreOrderVisit; it uses the new collectStatsPlanField traversal. That leaves arrowStatsCollector.Schema/Struct/Field/List/Map (lines 1675-1707) and the slice-wrapping Primitive (1754) / Variant (1781) reachable only from arrow_utils_internal_test.go:431. The package now maintains two independent traversals of the same schema, and the single test that asserts exact per-field MetricsMode values validates the orphaned one. Fix: delete the visitor methods and rewrite TestStatsTypes against computeStatsPlan, or keep PreOrderVisit as the only traversal.

Evidence

Mutating collectStatsPlanField to `if true { return }` (production plan always empty): `go test ./table/ -run 'TestStatsTypes$' -v` => `--- PASS: TestStatsTypes (0.00s)` / `ok github.com/apache/iceberg-go/table 0.565s`, while the full package reports 26 `--- FAIL` tests. grep confirms the only arrowStatsCollector+PreOrderVisit construction outside arrow_utils.go is arrow_utils_internal_test.go:432.

[zeroshade]

minor — Primitive resolves the column name twice per field

Primitive calls a.schema.FindColumnName(a.fieldID) at line 1755 solely to compute strings.Contains(colName, "."), then primitiveCollector repeats the identical lookup at line 1726 and repeats the !ok check. In a PR whose stated goal is removing redundant per-field work this is a doubled map lookup on the exact path being optimized. It is currently harmless only because this method is test-only (see the major finding).

[zeroshade]

nit — Override maps pre-sized to len(props) rather than the override count

columnModes and columnModeErrors are allocated with capacity len(props), i.e. the count of ALL table properties, not the count of write.metadata.metrics.column.* keys. Real tables carry many unrelated properties, so this over-allocates the very maps the PR is pre-sizing to save allocations.

[zeroshade]

nit — Benchmark field overrideFields is a stride, not a field count

The field is named as a count but is used as the loop increment (i += benchmarkCase.overrideFields), so the value 100 yields fieldCount/100 overrides. The arithmetic happens to produce the intended 1% for all three sizes, but the name inverts the meaning and will mislead the next person who tunes it. Rename to overrideStride, or express it as a count and derive the stride.