You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/08/28 08:05:28 UTC
[PR] perf(table): speed up bulk schema and partition spec removals [iceberg-go]
fallintoplace opened a new pull request, #1930:
URL: https://github.com/apache/iceberg-go/pull/1930
## What
- Build a set of requested IDs once in `RemovePartitionSpecs` and `RemoveSchemas`.
- Reuse it for protected-ID validation and list filtering.
- Keep removal order, update payloads, and no-op behavior unchanged.
- Add coverage for duplicates, requested-order differences, and atomic validation errors.
- Add benchmarks for small and historical metadata workloads.
## Why
Both methods used `slices.Contains` for every stored schema or partition spec. For `n` stored entries and `k` requested IDs, that makes the membership work O(n*k).
## Benchmark
Apple M1 Pro. Median of 3 runs:
```text
go test ./table -run '^$' -bench '^BenchmarkRemove(PartitionSpecs|Schemas)$' -benchmem -count=3 -benchtime=50ms
```
| Workload | Before | After | Speedup |
| --- | ---: | ---: | ---: |
| Partition specs 128/64 | 4.65 us/op | 3.02 us/op | 1.5x |
| Partition specs 1,024/512 | 168 us/op | 21.0 us/op | 8.0x |
| Partition specs 8,192/4,096 | 8.69 ms/op | 183 us/op | 47x |
| Schemas 128/64 | 4.17 us/op | 3.52 us/op | 1.2x |
| Schemas 1,024/512 | 163 us/op | 18.6 us/op | 8.7x |
| Schemas 8,192/4,096 | 8.33 ms/op | 140 us/op | 60x |
The indexed path uses extra memory for the ID set, in exchange for avoiding repeated linear scans. The one-entry cases are not the target workload.
## Tests
- `go test ./table -count=1`
- `go vet ./table`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf(table): speed up bulk schema and partition spec removals [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1930:
URL: https://github.com/apache/iceberg-go/pull/1930#discussion_r3890230640
##########
table/metadata.go:
##########
@@ -1506,14 +1506,30 @@ func (b *MetadataBuilder) RemovePartitionSpecs(ints []int) error {
return nil
}
- if slices.Contains(ints, b.defaultSpecID) {
+ var removedIDs map[int]struct{}
+ if len(ints) > 1 {
Review Comment:
The `> 1` threshold means we build the map as soon as there are two IDs to remove, and dropping a schema plus its rollback (or two specs) is a pretty common shape.
For two elements `slices.Contains` is two int comparisons and zero allocations, whereas the map is a heap allocation plus two hashes and inserts, so this path is strictly slower and allocates more than the code it replaces. In a PR that's specifically about cutting allocations that feels backwards. Break-even for map vs linear scan over ints usually lands somewhere around 8 to 16 elements.
The benchmark cases jump straight from removedCount 1 to 64, so the small-removal regression never shows up. I'd raise the threshold to something like `> 8` (pinned by a benchmark) and add a removedCount 2 or 4 case so the break-even is actually exercised. wdyt?
##########
table/metadata.go:
##########
@@ -1506,14 +1506,30 @@ func (b *MetadataBuilder) RemovePartitionSpecs(ints []int) error {
return nil
}
- if slices.Contains(ints, b.defaultSpecID) {
+ var removedIDs map[int]struct{}
+ if len(ints) > 1 {
+ removedIDs = make(map[int]struct{}, len(ints))
+ for _, id := range ints {
+ removedIDs[id] = struct{}{}
+ }
+ }
+ containsID := func(id int) bool {
Review Comment:
This block is byte-for-byte identical in `RemoveSchemas` just below, so the threshold change above has to land in two places, and so does any future tweak to the fallback. I'd pull it into a small unexported helper:
```go
// makeIntContainsFn reports whether id is in ints. ints must be non-empty.
func makeIntContainsFn(ints []int) func(int) bool {
if len(ints) <= 8 {
return func(id int) bool { return slices.Contains(ints, id) }
}
set := make(map[int]struct{}, len(ints))
for _, id := range ints {
set[id] = struct{}{}
}
return func(id int) bool {
_, ok := set[id]
return ok
}
}
```
Both methods collapse to `containsID := makeIntContainsFn(ints)`, and the `removedIDs == nil` / `ints[0]` dance goes away since the small case just scans the slice directly (which also drops the question of the closure capturing `ints[0]` while pinning the whole backing array). Thoughts?
##########
table/metadata_builder_bench_test.go:
##########
@@ -42,6 +45,67 @@ var removeSnapshotsBenchmarkCases = []struct {
{snapshotCount: 10_000, removedCount: 5_000},
}
+var bulkRemovalBenchmarkCases = []struct {
+ entryCount int
+ removedCount int
+}{
+ {entryCount: 8, removedCount: 1},
+ {entryCount: 128, removedCount: 64},
+ {entryCount: 1_024, removedCount: 512},
+ {entryCount: 8_192, removedCount: 4_096},
+}
+
+func BenchmarkRemovePartitionSpecs(b *testing.B) {
+ for _, tc := range bulkRemovalBenchmarkCases {
+ b.Run(fmt.Sprintf("specs=%d/removed=%d", tc.entryCount, tc.removedCount), func(b *testing.B) {
+ template := benchmarkPartitionSpecBuilder(tc.entryCount)
+ removed := benchmarkRemovedIDs(tc.removedCount)
+
+ b.ReportAllocs()
+ b.ReportMetric(float64(tc.entryCount), "spec_entries")
+ b.ReportMetric(float64(len(removed)), "removed_specs")
+ b.ResetTimer()
+
+ for range b.N {
+ builder := template
Review Comment:
`BenchmarkRemoveSchemas` wraps its setup in StopTimer/StartTimer to keep the O(n) clone out of the measurement; this one doesn't, so the two benchmarks aren't measuring on the same terms. It happens to be correct today only because `RemovePartitionSpecs` allocates a fresh `newSpecs` instead of mutating in place. If it ever switched to `slices.DeleteFunc` like `RemoveSchemas`, this loop would quietly start timing shrinking input.
Also `builder.updates = nil` here is a no-op, since `builder := template` already gives you a nil `updates`.
I'd either mirror the StopTimer/StartTimer block, or drop a one-line comment noting the struct copy is O(1) and deliberately left in-window.
##########
table/metadata_builder_internal_test.go:
##########
@@ -373,6 +373,37 @@ func TestRemovePartitionSpecsEmptyDoesNotUpdate(t *testing.T) {
require.Len(t, builder.specs, 1)
}
+func TestRemovePartitionSpecsPreservesStoredOrderAndIgnoresDuplicates(t *testing.T) {
+ builder := MetadataBuilder{
+ specs: []iceberg.PartitionSpec{
+ iceberg.NewPartitionSpecID(4),
+ iceberg.NewPartitionSpecID(1),
+ iceberg.NewPartitionSpecID(7),
+ },
+ defaultSpecID: 99,
+ }
+
+ require.NoError(t, builder.RemovePartitionSpecs([]int{7, 1, 7, 123}))
+ require.Len(t, builder.specs, 1)
+ require.Equal(t, 4, builder.specs[0].ID())
+ require.Len(t, builder.updates, 1)
+ require.Equal(t, []int{1, 7}, builder.updates[0].(*removeSpecUpdate).SpecIds)
Review Comment:
Worth flagging what this assertion pins: `removed` is built from the stored entries we actually find, so removing an absent ID (123 here) is silently dropped and the emitted `spec-ids` come out as `[1, 7]`. Java passes the caller's full requested set straight through, so it'd emit `[1, 7, 123]`, and PyIceberg raises on an unknown ID instead. None of this is introduced by your change, and in practice REST servers treat removing an absent spec as a no-op, so I'm not asking to change behavior.
The one thing I'd do is note it: this test now locks in the go-specific payload without saying so. A short comment (or a follow-up issue) marking the emitted IDs as intentionally the found subset would keep it from reading like an oversight later. Non-blocking.
##########
table/metadata_builder_internal_test.go:
##########
@@ -373,6 +373,37 @@ func TestRemovePartitionSpecsEmptyDoesNotUpdate(t *testing.T) {
require.Len(t, builder.specs, 1)
}
+func TestRemovePartitionSpecsPreservesStoredOrderAndIgnoresDuplicates(t *testing.T) {
Review Comment:
Every new test here passes three or more IDs, so they all drive the map path. The single-ID branch, which is the one bit of logic that's actually new in this PR, has no happy-path coverage, and the existing single-element tests only hit the error branch.
A refactor that broke the `removedIDs == nil` guard or the `ints[0]` compare wouldn't be caught by anything. I'd add two small cases: a one-element input that removes a matching non-default spec, and a one-element input that matches nothing, then the same pair for `RemoveSchemas`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf(table): speed up bulk schema and partition spec removals [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1930:
URL: https://github.com/apache/iceberg-go/pull/1930
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org