perf(table): reuse row-group metrics maps

[fallintoplace]

Summary

• Reuse the row-group metric maps between TestRowGroup calls.
• Pre-size valueCounts from the requested column count.
• Create null and bound maps only when the row group has those metrics.
• Clear reused maps before each row group so old metrics cannot leak.

Performance

Benchmark: BenchmarkParquetRowGroupMetricsMaps

Apple M1 Pro, Go 1.26.3, 100 ms per case.

Case	Before	After
128 row groups, 8 columns, no stats	184889 ns/op, 385033 B/op, 3712 allocs/op	152865 ns/op, 360448 B/op, 3200 allocs/op
128 row groups, 8 columns, all stats	916939 ns/op, 3305896 B/op, 18560 allocs/op	886321 ns/op, 3170730 B/op, 17536 allocs/op
1024 row groups, 32 columns, no stats	4778387 ns/op, 11534344 B/op, 103424 allocs/op	4672652 ns/op, 11337776 B/op, 99328 allocs/op
1024 row groups, 32 columns, all stats	31211177 ns/op, 115150928 B/op, 594951 allocs/op	23795442 ns/op, 101258094 B/op, 558089 allocs/op

Numbers are local reference points and will vary by machine.

Tests

• go test ./table/... -count=1
• go test -race ./table -count=1
• go vet ./table
• go test ./table -run '^$' -bench '^BenchmarkParquetRowGroupMetricsMaps$' -benchmem -benchtime=100ms -count=1

[laskoviymishka]

Really nice change.

One blocking thing before merge: the nullCounts handling changes pruning behavior. The old code allocated a fresh nullCounts every call, so mayContainNull never hit its == nil branch and a field with no null stats was always prune-eligible. Now nullCounts starts nil and, once some row group populates it, later no-stats row groups run clear() and leave a non-nil empty map, so mayContainNull (and NOT STARTS WITH pruning through it) returns different results for identical row groups depending on call order. Since this is meant to be allocation-only, I'd restore the pre-change invariant by handling nullCounts the same as valueCounts (== nil -> make, else clear) and dropping the lazy make in the loop.

I'd also want the test to exercise that path: the lifecycle test only runs AlwaysTrue today, so the maps get built but the pruning code that reads them never runs, and the third-call assert.Empty passes for both nil and empty. A NOT STARTS WITH case across a no-stats, with-stats, no-stats sequence, asserting the result matches on the identical calls, would lock the contract.

The rest looks good and the benchmark structure is solid. Once those are addressed, happy to take another pass and approve.

[laskoviymishka]

I think this quietly changes pruning behavior, which is the one thing I'd want fixed before merge on an allocation-only PR.

Before this change nullCounts got a fresh make every call, so mayContainNull's if m.nullCounts == nil { return true } branch was effectively dead and a field with no null stats always came back false (prune-eligible). Now nullCounts starts nil and is only allocated once some row group has null stats; after that, a later no-null-stats row group runs clear() and leaves a non-nil empty map instead of nil. So two structurally identical row groups can return opposite results from mayContainNull depending on whether an earlier call populated it, and VisitNotStartsWith keys its pruning decision on exactly that.

Cleanest fix for a perf PR is to keep the pre-change invariant by handling nullCounts the same as valueCounts, and dropping the lazy make in the loop:

if m.nullCounts == nil {
	m.nullCounts = make(map[int]int64, len(colIndices))
} else {
	clear(m.nullCounts)
}

valueCounts is already fine, and lowerBounds/upperBounds are fine too since every consumer reads them by key and nil-checks the returned []byte; nil and empty behave identically there. It's only mayContainNull that hard-branches on == nil. wdyt?

[laskoviymishka]

While we're in here: upperBounds only ever gets allocated inside this lowerBounds == nil branch, so lowerBounds is effectively the sentinel for the pair. A one-line comment noting they're always allocated together would keep a future change from setting one without the other.

[laskoviymishka]

benchmarkTestRowGroupBefore is a hand-copied snapshot of the old TestRowGroup, and nothing keeps it in sync. If TestRowGroup changes later, the "before" baseline silently goes stale. A one-line comment marking it an intentional frozen copy of the pre-#1899 implementation (that shouldn't track future changes) would save the confusion.

[laskoviymishka]

This lifecycle test only ever runs AlwaysTrue, so VisitTrue returns before any of these maps get consulted: the reuse paths get built up, but the pruning logic that actually reads them never runs.

I'd add a case that evaluates NOT STARTS WITH (or IS NULL/NOT NULL) across the same no-stats, with-stats, no-stats sequence and asserts the result is identical on the first and third calls. That's what locks the contract, and it would catch the nullCounts leak from the other comment. wdyt?

[laskoviymishka]

assert.Empty passes for both a nil map and a non-nil empty one, so this third block doesn't distinguish "map reused and cleared" from "map reset to nil": it can't catch the behavior change above or prove the reuse invariant the PR is going for.

Exact assertions depend on which fix we take for nullCounts, but the shape I'd want is assert.NotNil + assert.Empty on the reused maps so it actually pins "same map, emptied." Heads up that flips the first-call assert.Nil(eval.nullCounts) too if we go the always-non-nil route.

[laskoviymishka]

👍