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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
