perf(table): bind partition transforms once

[fallintoplace]

Depends on #1765. This is stacked on top of it, so only the second commit is new here.

What changed

• Bind built-in partition transform functions when the extraction plan is created.
• Feed native Iceberg values directly into identity, bucket, truncate, and time transforms.
• Keep the generic literal path for custom or unsupported transforms.
• Keep unknown and void transforms producing null partition values.

Why

The row loop currently converts each Arrow value into a literal and calls Transform.Apply. Some transforms do more setup there. For example, TruncateTransform.Apply rebuilds its transformer for every value.

The extraction plan has the source type already, so it can bind the transform once and reuse the function for every batch. Java's StructTransform uses the same bind-once shape.

Benchmark

Apple M1 Pro, 65,536 rows, median of 3 runs. The baseline is #1765.

go test ./table -run '^$' -bench '^BenchmarkPartitionTransforms$' -benchmem -benchtime=1s -count=3 -cpu=1

Transform	#1765	this PR
identity int64	2,648,326 ns/op	2,531,346 ns/op
bucket string	5,862,667 ns/op	5,710,295 ns/op
truncate string	15,767,652 ns/op, 7,271,656 B/op, 394,124 allocs/op	7,205,658 ns/op, 3,077,352 B/op, 131,980 allocs/op
day timestamp	3,898,547 ns/op	3,468,050 ns/op
hour timestamp_ns	3,812,458 ns/op	3,621,065 ns/op

The largest change is truncate[string], which is about 2.2x faster with 67% fewer allocations.

Tests

• go test ./...
• go test -race ./table -run '^TestFanoutWriter$' -count=1
• go vet ./...
• git diff --check

[laskoviymishka]

This is a clean win. Pulling schema resolution and transform binding out of the per-row loop is exactly the right lever here, and I like that you added benchmarks to show it rather than assert it.

I'd hold it before merging though. My one real concern is parity: the refactor swaps the per-row Transform.Apply(literal) for a pre-bound closure over a native Arrow value, and correctness now depends on those two paths producing identical partition keys. The new parity test only checks String and timestamp, and it checks them at the literal level: it never runs a value through getArrowValueAsIcebergValue, which is the actual production input. The cases I'd want covered are the ones where the equivalence is non-obvious: bucket on int32/date (the bound path hashes at int32 width, Apply at int64, equal only because Go's integer conversion preserves the value mod 2^64) and bucket/truncate on decimal/uuid/fixed (untested v.(Decimal) / uuid.UUID assertions). A wrong bucket value there wouldn't fail any existing test; it'd just silently mis-assign partitions.

The other thing I'd fix is small but concrete: the fast-path closure passes a nil value straight to bound when the cell is null. It's safe today only because the single caller guards on !IsNull first, but the closure itself has no guard, so a future refactor or a second caller turns it into a panic. Mirroring the nil check the literal path already has closes that.

A few things I'd want before merge:

• parity tests through the full Arrow-native path for bucket int32/decimal/uuid (and a date bucket)
• the nil guard in the fast-path closure
• pre-warm BenchmarkPartitionExtraction before ResetTimer so plan-build cost isn't in the numbers

The rest (the TimeTransform breadcrumb, the unknown fallback test name, a test for the divergent-schema rebuild path) are minor and can ride along or follow up. Once the parity coverage and the nil guard land, happy to take another pass and approve.

[laskoviymishka]

This folds the one-time plan build into the measured loop: the first b.Loop() iteration runs planOnce.Do and the whole plan construction. BenchmarkPartitionTransforms already warms up with a getPartitions call before ResetTimer; I'd do the same here so the rows_0/rows_1 numbers aren't dominated by plan setup.

[laskoviymishka]

The new tests cover the pointer-equal and equivalent-schema reuse paths, but nothing sends a genuinely divergent schema through the fanout writer to exercise this rebuild branch. I'd add a two-batch case with different-but-both-valid schemas and check both produce correct partition assignments.

[laskoviymishka]

getArrowValueAsIcebergValue returns (nil, nil) on a null cell, and this closure hands that straight to bound: for bucket/truncate that's a type assertion on nil, i.e. a panic. It's safe today only because the sole caller guards with col != nil && !col.IsNull(...) before dispatching, while the fallback closure routes through Apply and is fine. I'd give this closure the same nil check the literal path has so it's self-contained:

value, err := getArrowValueAsIcebergValue(column, row, sourceType)
if err != nil {
    return nil, err
}
if value == nil {
    return nil, nil
}
return bound(value), nil

[laskoviymishka]

Truncate guards on the Transformer error and falls back to Apply; Bucket calls Transformer(sourceType) unconditionally. Can BucketTransform.Transformer hand back a transformer that fails on an unsupported source type here? If so we'd panic in the row loop instead of falling back the way Truncate does. wdyt?

[laskoviymishka]

Year/Month/Day/Hour aren't in the switch above; they land here via TimeTransform. Took me a second to convince myself the switch was complete; a one-line comment noting the four time transforms are handled by this interface check would save the next reader that detour.

[laskoviymishka]

This case has fallback: false, so it asserts ok == true: it's checking the UnknownTransform arm returns a binding, which is the opposite of a fallback. The name reads just like the "invalid truncate fallback" case above it; I'd rename to something like "unknown_transform" to avoid the mixup.

[laskoviymishka]

This proves parity at the literal level: bound(test.value.Any()) feeds a literal's native value, so it never touches getArrowValueAsIcebergValue, which is the actual production input. That leaves the cases where equivalence only holds incidentally untested: bucket on int32/date binds hashHelperInt at a narrower width than Apply's int64 path (equal only because two's-complement conversion preserves the value mod 2^64), and bucket/truncate on decimal/uuid/fixed lean on v.(Decimal) / uuid.UUID assertions in the extractor that nothing here hits. Every case here is String or timestamp.

I'd add bucket cases for int32, decimal, and uuid that build a real Arrow array, run it through getArrowValueAsIcebergValue, and compare against Apply(getArrowValueAsIcebergLiteral(...)). That pins the equivalence we're currently relying on rather than trusting it.

[laskoviymishka]

👍

[laskoviymishka]

@badalprasadsingh need to resolve confllicts, once done - ready to merge

[badalprasadsingh]

@badalprasadsingh need to resolve confllicts, once done - ready to merge

@fallintoplace :))

[laskoviymishka]

Ups!