Skip to content

perf(manifest): update partition summaries incrementally - #1770

Merged
zeroshade merged 3 commits into
apache:mainfrom
fallintoplace:perf/incremental-manifest-partition-summaries
Aug 17, 2026
Merged

perf(manifest): update partition summaries incrementally#1770
zeroshade merged 3 commits into
apache:mainfrom
fallintoplace:perf/incremental-manifest-partition-summaries

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

What

  • update partition field stats as manifest entries are written
  • stop retaining every entry's partition map until finalization
  • keep summary errors reported from ToManifestFile, like before
  • add coverage for null, NaN, binary bounds, and failed entry encoding
  • add writer and finalization benchmarks

Why

ManifestWriter currently keeps one partition map per entry. ToManifestFile then walks all of them again to build partition summaries.

This makes retained state and finalization work grow with the number of manifest entries. The Java writer already keeps one PartitionSummary accumulator and updates it for each entry.

The summary values are unchanged. They still follow the spec rules for nulls, NaNs, and lower/upper bounds.

Benchmark

go test . -run '^$' -bench '^BenchmarkManifestWriterPartitionSummaryFinalization$' -benchmem -benchtime=1s -count=5 -cpu=1

Median results on an Apple M1 Pro:

Entries main this PR main B/op this PR B/op main allocs/op this PR allocs/op
100 31.9 us 0.92 us 7,152 568 627 19
1,000 265.6 us 0.71 us 57,556 572 6,028 20
10,000 3.31 ms 0.66 us 561,556 572 60,028 20

This benchmark isolates ToManifestFile after entries have already been written. End-to-end manifest writing is still mostly Avro encoding. At 10,000 entries, the end-to-end benchmark also reduced allocations from about 23.02 MB/op to 22.71 MB/op.

Testing

  • go test ./...
  • go test -race . -run '^(TestManifestWriterUpdatesPartitionSummariesIncrementally|TestConstructPartitionSummariesWithDroppedSource|TestManifests)$' -count=1
  • go vet ./...

@fallintoplace
fallintoplace force-pushed the perf/incremental-manifest-partition-summaries branch from af7e223 to 6aee32c Compare August 15, 2026 22:43
@fallintoplace
fallintoplace marked this pull request as ready for review August 15, 2026 22:46
Comment thread manifest.go
Comment on lines 1270 to 1276
var actualVal T
v := reflect.ValueOf(value)
if !v.CanConvert(reflect.TypeOf(actualVal)) {
return fmt.Errorf("expected type %T, got %T", actualVal, value)
}

actualVal = v.Convert(reflect.TypeOf(actualVal)).Interface().(T)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of the clonePartitionStatValue function, should we just incorporate cloning the []byte slice into this piece of logic? That seems simpler and preferable

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I simplified the update logic.

@zeroshade
zeroshade merged commit f6622e6 into apache:main Aug 17, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants