perf(table): avoid per-file partition materialization in partitions metadata

[fallintoplace]

What changed

• Partitions() now uses the borrowed partition map for aggregate-tree lookups.
• The coerced map and positional record are built only when a partition is first seen.
• Binary values are copied before they are retained by an aggregate.
• Added coverage for binary, NaN, and binary ownership behavior.
• Added a before/after benchmark for repeated partitions.

Why

InspectTable.Partitions was materializing several partition representations for every manifest entry. Most files share an existing partition, so that work was repeated and then discarded.

The new path keeps the existing partition evolution behavior. It also keeps the public DataFile.Partition() fallback for custom data file implementations.

Benchmark

• Command: go test ./table -run=^\\$ -bench=BenchmarkInspectPartitionAggregation -benchmem -benchtime=200ms -count=5
• Machine: Apple M1 Pro, darwin/arm64
• Scope: aggregation loop only. Manifest decoding and Arrow output are outside the timed section.
• Median of five samples:
  • 10,000 files / 100 partitions / 1 int32 field: 2.01 ms, 2.78 MB, 30.8k allocs -> 0.395 ms, 59.0 KB, 911 allocs (80% lower time, 98% fewer bytes)
  • 100,000 files / 100 partitions / 8 int32 fields: 53.5 ms, 38.7 MB, 304k allocs -> 25.8 ms, 295 KB, 4.11k allocs (52% lower time, 99% fewer bytes)
  • 100,000 files / 100 partitions / 32 binary fields: 610 ms, 732 MB, 13.6M allocs -> 182 ms, 2.89 MB, 33.8k allocs (70% lower time, 99.6% fewer bytes)

Tests

• go test ./... -count=1
• go vet ./table

[laskoviymishka]

Nice optimization, and the direction is right. Deferring per-file materialization and keying the trie off the borrowed partition map is a real win on the hot path, and the output stays equivalent to the old code.

I'd hold this before merging though.

The main thing is the partitionRecord field on inspectPartitionAggregate. Nothing reads it anymore (the output path goes through aggregate.partition, which is deep-cloned), but it still retains the borrowed []byte slice headers from dataFilePartition, so it pins the DataFile's Avro decode buffers for the whole aggregation and quietly re-opens the borrow-contract violation the rest of the PR takes care to respect. I'd just drop the field.

The other thing is coverage. The partition-evolution case (a file from an older spec missing a field that was added later) is the exact equivalence this PR claims to preserve, and it's the one case the new test doesn't hit. I'd want a sub-case for it before merge.

A few things I'd want before merge:

• drop the dead partitionRecord field from inspectPartitionAggregate
• add a lookupPartition test for the missing-field / partition-evolution case
• a short comment tying insert's positional keying to lookupPartition's field-ID keying (they only agree by FieldList ordering)

The rest (the misleading nil-partitionType guard, a couple of benchmark tidy-ups) are minor and inline. Once the field's gone and the evolution case is covered, happy to take another pass and approve.

[laskoviymishka]

partitionRecord here still holds the borrowed []byte slice headers from dataFilePartition(file). inspectCoercePartition only shallow-copies the map, so those byte backing arrays stay aliased to the DataFile's Avro decode buffer and now escape across the whole aggregation.

The saving grace is that nothing reads partitionRecord anymore. appendPartitionAggregate goes through aggregate.partition, which cloneInspectPartition deep-copies (bytes included, now), so this field is dead weight that also pins memory and quietly re-opens the borrow-contract violation the rest of the PR is careful about.

I'd just drop the field from inspectPartitionAggregate and this struct literal. The trie key already owns its copy via comparablePartitionKey, and lookupPartition never touches it.

[laskoviymishka]

This is the equivalence the PR is really leaning on, that lookupPartition matches the old coerce+record+lookup path, and the partition-evolution case is the one I'd most want pinned down but it isn't covered here.

Could we add a sub-case where a file's partition map is missing one of the fields in partitionType (older spec, field added later)? partition[field.ID] returns nil and inspectCoercePartition skips it too, so both paths key on nil at that level and agree. That's exactly the kind of quiet invariant a future change to inspectCoercePartition could break without anything failing.

While we're here, the partitionType == nil early-return branch is also untested. wdyt?

[laskoviymishka]

This guard makes lookupPartition look like it handles a nil partitionType, but the aggregate-creation path after a nil lookup still calls newPartitionRecord(nil, nil) and panics dereferencing the nil FieldList. Same as before this PR, so not a regression, just newly misleading.

I'd either mirror the nil check where the aggregate gets built, or drop a one-line comment that nil is unreachable here and the guard is purely defensive. wdyt?

[laskoviymishka]

insert keys the trie positionally off the partitionRecord slice, while lookupPartition keys off partition[field.ID] walked in FieldList order. They only line up because record is always built from this same partitionType.FieldList, and that coupling is load-bearing now that lookupPartition is the only production reader.

I'd add a short comment on insert (or here) noting the two have to walk fields in the same order. lookup also only has test callers now, so it's worth deciding whether to keep it or move those tests onto lookupPartition.

[laskoviymishka]

This struct is spelled out again in benchmarkInspectPartitionFiles's signature, so adding a field later means matching the shape and order in both places. Could we lift it to a named type at package scope and use it in both spots?

[laskoviymishka]

b.Loop() manages the timer itself (it resets and starts on the first call), and there's nothing expensive between ReportMetric and the loop, so this ResetTimer is redundant. I'd drop it, or add a comment if it's there for older Go.