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/27 12:46:08 UTC
[PR] perf(table): index partition specs by ID [iceberg-go]
fallintoplace opened a new pull request, #1910:
URL: https://github.com/apache/iceberg-go/pull/1910
## Summary
- **Adds an ID to position index for partition specs.**
- Uses it for metadata `PartitionSpecByID`, default `PartitionSpec`, and builder `GetSpecByID`.
- Keeps the existing defensive-copy behavior.
- Updates the index on add/remove and uses copy-on-write for builder clones.
- Includes fallback coverage for stale in-package slices and read-only concurrent lookups.
## Benchmark
Command:
`go test ./table -run ^$ -bench ^(BenchmarkPartitionSpecByID|BenchmarkMetadataBuilderGetSpecByID)$ -benchmem -benchtime=200ms -count=5`
Median of 5 runs on Apple M1 Pro:
| Workload | Before | After |
| --- | ---: | ---: |
| Metadata, 2,048 specs, last hit | 2,528 ns/op | 84 ns/op |
| Metadata, 2,048 specs, miss | 2,443 ns/op | 8.34 ns/op |
| Builder, 2,048 specs, last hit | 54.9 us, 98,304 B/op, 2,048 allocs/op | 33.8 ns/op, 48 B/op, 1 alloc/op |
| Builder, 2,048 specs, miss | 56.5 us, 98,403 B/op, 2,051 allocs/op | 138 ns/op, 72 B/op, 3 allocs/op |
The metadata path still clones the returned spec. Builder misses still format the existing error. The lookup itself is now constant time.
## Checks
- `go test ./table -count=1 -timeout=5m`
- `go test ./table -race -run ^(TestCommonMetadataPartitionSpecIndex|TestParsedMetadataBuildsPartitionSpecIndex|TestMetadataBuilderPartitionSpecIndex|TestMetadataBuilderRemoveUnknownPartitionSpec|TestCommonMetadataPartitionSpecLookupsConcurrent|TestMetadataBuilderPartitionSpecLookupsConcurrent)$ -count=1`
- `go test ./... -run ^$ -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): index partition specs by ID [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1910:
URL: https://github.com/apache/iceberg-go/pull/1910#discussion_r3892539034
##########
table/metadata.go:
##########
@@ -2138,23 +2254,29 @@ func (c *commonMetadata) DefaultPartitionSpec() int {
return c.DefaultSpecID
}
+func (c *commonMetadata) ensurePartitionSpecIndex() {
+ if partitionSpecIndexNeedsRebuild(c.partitionSpecIndex, c.Specs) {
+ c.partitionSpecIndex = buildPartitionSpecIndex(c.Specs)
Review Comment:
This is the one structural thing still bugging me, and it's the same root cause as the duplicate-ID race: we write `c.partitionSpecIndex` back from a read path. `SnapshotByID` deliberately doesn't; it rebuilds into a local and never touches the struct field, so two concurrent readers can't race on it.
`sourceCount` closed the duplicate-ID trigger, but the write-back is still reachable by any in-package fixture that swaps `c.Specs` and then reads concurrently (we already have `...FallsBackAfterSliceReplacement` doing the swap). Mirroring `SnapshotByID` here (rebuild into a local, feed it to `partitionSpecIndexPosition`, don't assign the field) closes the class for good and makes this actually mirror `snapshotIndex` like the comment claims.
If we also reject duplicate IDs above, these two together make the rebuild branch unreachable in production entirely.
##########
table/metadata.go:
##########
@@ -142,6 +142,88 @@ func snapshotIndexPosition(index *snapshotIndexData, snapshots []Snapshot, id in
return 0, false
}
+type partitionSpecIndexData struct {
+ positions map[int]int
+ // sourceCount records the number of specs used to build positions. It is
+ // separate from len(positions) because multiple specs may have the same ID.
+ sourceCount int
+ // firstSpec identifies the spec slice used to build positions. It lets
+ // read-only lookups detect an index left behind by an in-package fixture
+ // that replaced the slice.
+ firstSpec *iceberg.PartitionSpec
+ // shared means positions is owned by more than one builder or metadata
+ // value and must be copied before a builder mutates it.
+ shared bool
+}
+
+// Small spec slices are faster to search directly than to hash-map lookup.
+const partitionSpecIndexMinSize = 32
+
+func partitionSpecListFirst(specs []iceberg.PartitionSpec) *iceberg.PartitionSpec {
+ if len(specs) == 0 {
+ return nil
+ }
+
+ return &specs[0]
+}
+
+func buildPartitionSpecIndex(specs []iceberg.PartitionSpec) *partitionSpecIndexData {
+ positions := make(map[int]int, len(specs))
+ for i := range specs {
+ id := specs[i].ID()
+ if _, exists := positions[id]; !exists {
Review Comment:
The `sourceCount` fix does close the duplicate-ID race, but it leaves us tolerating something Java refuses. `PartitionUtil.indexSpecs` builds an `ImmutableMap` whose `.build()` throws on a duplicate spec ID, so Java won't even load metadata with duplicate IDs, whereas after this we'll happily read it and silently expose only the first spec for the dup'd id. That also diverges from our own `checkSchemas`/`checkSnapshots`, which both reject duplicate IDs, and from the spec's unique-spec-ID rule.
So rather than tolerating duplicates here via `sourceCount`, I'd lean toward rejecting them in `checkPartitionSpecs`: a `seen` set returning `ErrInvalidMetadata`, matching the two sibling checkers and matching Java. As a bonus it makes `sourceCount != len(specs)` unreachable for real metadata, which also takes the read-path rebuild off the table (see the `ensurePartitionSpecIndex` thread). `sourceCount` can stay for in-package fixture safety.
wdyt? Happy either way if you'd rather keep tolerating, but then I think we owe a comment here on why we accept what the sibling checkers and Java reject.
##########
table/partition_spec_index_test.go:
##########
@@ -0,0 +1,383 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "sync"
+ "testing"
+
+ "github.com/apache/iceberg-go"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+// Why: metadata lookups must use the derived ID index without changing the
+// existing default-spec fallback or missing-ID behavior.
+// Condition: metadata contains non-sequential partition spec IDs and the
+// index is initialized from the same slice.
+// Assertion: first, default, and missing lookups return the expected values.
+func TestCommonMetadataPartitionSpecIndexLookups(t *testing.T) {
+ specs := partitionSpecIndexTestSpecs(0, 7, 42)
+ metadata := commonMetadata{
+ Specs: specs,
+ DefaultSpecID: 42,
+ partitionSpecIndex: buildPartitionSpecIndex(specs),
+ }
+
+ byID := metadata.PartitionSpecByID(7)
+ require.NotNil(t, byID)
+ assert.Equal(t, 7, byID.ID())
+
+ defaultSpec := metadata.PartitionSpec()
+ assert.Equal(t, 42, defaultSpec.ID())
+ assert.Nil(t, metadata.PartitionSpecByID(99))
+}
+
+// Why: decoded metadata must initialize the same derived index as metadata
+// built by a writer, rather than rebuilding it on every lookup.
+// Condition: parse a valid metadata document containing partition specs.
+// Assertion: the decoded common metadata owns one index entry per spec.
+func TestParsedMetadataBuildsPartitionSpecIndex(t *testing.T) {
+ metadata, err := ParseMetadataBytes([]byte(ExampleTableMetadataV2))
+ require.NoError(t, err)
+
+ common := metadataCommon(metadata)
+ require.Len(t, common.partitionSpecIndex.positions, len(common.Specs))
Review Comment:
Small one tied to the `sourceCount` change: this asserts `len(positions) == len(Specs)`, but `sourceCount` exists precisely to decouple those. For duplicate-ID metadata `len(positions) < len(Specs)` and this would fail even though the index is correct. I'd assert `sourceCount` instead: `require.Equal(t, len(common.Specs), common.partitionSpecIndex.sourceCount)`.
##########
table/metadata.go:
##########
@@ -142,6 +142,88 @@ func snapshotIndexPosition(index *snapshotIndexData, snapshots []Snapshot, id in
return 0, false
}
+type partitionSpecIndexData struct {
+ positions map[int]int
+ // sourceCount records the number of specs used to build positions. It is
+ // separate from len(positions) because multiple specs may have the same ID.
+ sourceCount int
+ // firstSpec identifies the spec slice used to build positions. It lets
+ // read-only lookups detect an index left behind by an in-package fixture
+ // that replaced the slice.
+ firstSpec *iceberg.PartitionSpec
+ // shared means positions is owned by more than one builder or metadata
+ // value and must be copied before a builder mutates it.
+ shared bool
+}
+
+// Small spec slices are faster to search directly than to hash-map lookup.
+const partitionSpecIndexMinSize = 32
+
+func partitionSpecListFirst(specs []iceberg.PartitionSpec) *iceberg.PartitionSpec {
+ if len(specs) == 0 {
+ return nil
+ }
+
+ return &specs[0]
+}
+
+func buildPartitionSpecIndex(specs []iceberg.PartitionSpec) *partitionSpecIndexData {
+ positions := make(map[int]int, len(specs))
+ for i := range specs {
+ id := specs[i].ID()
+ if _, exists := positions[id]; !exists {
+ positions[id] = i
+ }
+ }
+
+ return &partitionSpecIndexData{
+ positions: positions,
+ sourceCount: len(specs),
+ firstSpec: partitionSpecListFirst(specs),
+ }
+}
+
+func clonePartitionSpecIndex(index *partitionSpecIndexData) *partitionSpecIndexData {
+ if index == nil {
+ return nil
+ }
+
+ return &partitionSpecIndexData{
+ positions: maps.Clone(index.positions),
+ sourceCount: index.sourceCount,
+ firstSpec: index.firstSpec,
+ }
+}
+
+func partitionSpecIndexNeedsRebuild(index *partitionSpecIndexData, specs []iceberg.PartitionSpec) bool {
+ if index == nil || index.sourceCount != len(specs) {
+ return true
+ }
+
+ return len(specs) > 0 && index.firstSpec != &specs[0]
+}
+
+// partitionSpecIndexPosition returns the position for id. The map is the fast
+// path, while the linear scan preserves lookup behavior if an in-package
+// fixture mutates a spec in place without rebuilding the derived index.
+func partitionSpecIndexPosition(index *partitionSpecIndexData, specs []iceberg.PartitionSpec, id int) (int, bool) {
+ if index != nil && len(specs) >= partitionSpecIndexMinSize {
+ if i, ok := index.positions[id]; ok {
+ if i >= 0 && i < len(specs) && specs[i].ID() == id {
+ return i, true
+ }
+ }
+ }
+
+ for i := range specs {
Review Comment:
Not a change request, just noticing this stays O(n) on a clean miss, unlike `snapshotIndexPosition` which returns early. That's actually correct here: the scan is what makes the element-mutation fallback work, since a clean miss is indistinguishable from an in-place mutation that introduced a new id. Worth a one-liner saying so, so nobody "optimizes" the early return back in and reintroduces a false miss.
--
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): index partition specs by ID [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1910:
URL: https://github.com/apache/iceberg-go/pull/1910#discussion_r3937421222
##########
table/partition_spec_index_test.go:
##########
@@ -0,0 +1,447 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "encoding/json"
+ "sync"
+ "testing"
+
+ "github.com/apache/iceberg-go"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+// Why: metadata lookups must use the derived ID index without changing the
+// existing default-spec fallback or missing-ID behavior.
+// Condition: metadata contains non-sequential partition spec IDs and the
+// index is initialized from the same slice.
+// Assertion: first, default, and missing lookups return the expected values.
+func TestCommonMetadataPartitionSpecIndexLookups(t *testing.T) {
+ specs := partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 0, 7, 42)
+ metadata := commonMetadata{
+ Specs: specs,
+ DefaultSpecID: 42,
+ partitionSpecIndex: buildPartitionSpecIndex(specs),
+ }
+
+ byID := metadata.PartitionSpecByID(7)
+ require.NotNil(t, byID)
+ assert.Equal(t, 7, byID.ID())
+
+ defaultSpec := metadata.PartitionSpec()
+ assert.Equal(t, 42, defaultSpec.ID())
+ assert.Nil(t, metadata.PartitionSpecByID(99))
+}
+
+// Why: small metadata should not allocate an index that its linear lookup path
+// will never read.
+// Condition: parse a valid metadata document containing one partition spec.
+// Assertion: the decoded common metadata keeps only the slice identity.
+func TestParsedMetadataSkipsSmallPartitionSpecIndex(t *testing.T) {
+ metadata, err := ParseMetadataBytes([]byte(ExampleTableMetadataV2))
+ require.NoError(t, err)
+
+ common := metadataCommon(metadata)
+ require.Nil(t, common.partitionSpecIndex)
+}
+
+// Why: builders created from existing metadata should use the same small-slice
+// lookup policy as parsed metadata.
+// Condition: create a builder from parsed metadata and look up its final spec.
+// Assertion: the lookup works without allocating a map for one spec.
+func TestMetadataBuilderFromBaseSkipsSmallPartitionSpecIndex(t *testing.T) {
+ metadata, err := ParseMetadataBytes([]byte(ExampleTableMetadataV2))
+ require.NoError(t, err)
+
+ builder, err := MetadataBuilderFromBase(metadata, "")
+ require.NoError(t, err)
+ require.Nil(t, builder.partitionSpecIndex)
+
+ id := builder.specs[len(builder.specs)-1].ID()
+ spec, err := builder.GetSpecByID(id)
+ require.NoError(t, err)
+ require.NotNil(t, spec)
+ assert.Equal(t, id, spec.ID())
+}
+
+// Why: in-package fixtures can replace metadata slices directly, so a stale
+// derived index must not return a spec at the old position or hide a new one.
+// Condition: the indexed slice is replaced with another slice of equal length.
+// Assertion: lookups use the replacement slice without mutating the cached
+// index.
+func TestCommonMetadataPartitionSpecIndexFallsBackAfterSliceReplacement(t *testing.T) {
+ specs := partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 1, 2)
+ metadata := commonMetadata{
+ Specs: specs,
+ DefaultSpecID: 2,
+ partitionSpecIndex: buildPartitionSpecIndex(specs),
+ }
+ originalIndex := metadata.partitionSpecIndex
+
+ metadata.Specs = partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 3, 4)
+ metadata.DefaultSpecID = 4
+ byID := metadata.PartitionSpecByID(4)
+ require.NotNil(t, byID)
+ assert.Equal(t, 4, byID.ID())
+ defaultSpec := metadata.PartitionSpec()
+ assert.Equal(t, 4, defaultSpec.ID())
+ assert.Nil(t, metadata.PartitionSpecByID(2))
+ assert.Same(t, originalIndex, metadata.partitionSpecIndex)
+ assert.Equal(t, 0, metadata.partitionSpecIndex.positions[1])
+ assert.Equal(t, 1, metadata.partitionSpecIndex.positions[2])
+}
+
+func TestCommonMetadataPartitionSpecIndexFallsBackAfterSliceReplacementConcurrent(t *testing.T) {
+ metadata := &commonMetadata{
+ Specs: partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 1, 2),
+ partitionSpecIndex: buildPartitionSpecIndex(partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 1, 2)),
+ }
+ metadata.Specs = partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 3, 4)
+ originalIndex := metadata.partitionSpecIndex
+
+ const goroutineCount = 8
+ var wg sync.WaitGroup
+ wg.Add(goroutineCount)
+ for range goroutineCount {
+ go func() {
+ defer wg.Done()
+ for range 100 {
+ if spec := metadata.PartitionSpecByID(4); spec == nil || spec.ID() != 4 {
+ t.Errorf("expected partition spec 4, got %v", spec)
+ }
+ }
+ }()
+ }
+ wg.Wait()
+
+ assert.Same(t, originalIndex, metadata.partitionSpecIndex)
+}
+
+func TestRejectsDuplicatePartitionSpecIDs(t *testing.T) {
+ var raw map[string]json.RawMessage
+ require.NoError(t, json.Unmarshal([]byte(ExampleTableMetadataV2), &raw))
+
+ var specs []json.RawMessage
+ require.NoError(t, json.Unmarshal(raw["partition-specs"], &specs))
+ specs = append(specs, specs[0])
+ encodedSpecs, err := json.Marshal(specs)
+ require.NoError(t, err)
+ raw["partition-specs"] = encodedSpecs
+ data, err := json.Marshal(raw)
+ require.NoError(t, err)
+
+ _, err = ParseMetadataBytes(data)
+ require.ErrorIs(t, err, ErrInvalidMetadata)
+ assert.ErrorContains(t, err, "duplicate partition spec ID 0")
+}
+
+// Why: builder-produced metadata must enforce the same unique partition spec
+// ID invariant as metadata read from JSON.
+// Condition: an in-package builder is given duplicate spec IDs before Build.
+// Assertion: Build rejects the metadata instead of publishing an ambiguous index.
+func TestMetadataBuilderBuildRejectsDuplicatePartitionSpecIDs(t *testing.T) {
+ builder := builderWithoutChanges(2)
+ builder.specs = append(builder.specs, builder.specs[0])
+
+ _, err := builder.Build()
+ require.ErrorIs(t, err, ErrInvalidMetadata)
+ assert.ErrorContains(t, err, "duplicate partition spec ID 0")
+}
+
+// Why: in-package fixtures can mutate an existing spec slice without changing
+// its length or backing array, which cannot be detected by index metadata alone.
+// Condition: a spec is replaced in place after the index is built.
+// Assertion: lookup still finds the replacement and does not return the old ID.
+func TestCommonMetadataPartitionSpecIndexFallsBackAfterElementMutation(t *testing.T) {
+ specs := partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 1, 2)
+ oldID := specs[7].ID()
+ metadata := commonMetadata{
+ Specs: specs,
+ DefaultSpecID: 1,
+ partitionSpecIndex: buildPartitionSpecIndex(specs),
+ }
+
+ specs[7] = iceberg.NewPartitionSpecID(3)
+
+ byID := metadata.PartitionSpecByID(3)
+ require.NotNil(t, byID)
+ assert.Equal(t, 3, byID.ID())
+ assert.Nil(t, metadata.PartitionSpecByID(oldID))
+}
+
+// Why: the default-spec lookup must not return a stale indexed value after an
+// in-place fixture mutation removes the configured default ID.
+// Condition: the spec at the indexed default position is replaced in place.
+// Assertion: PartitionSpec falls back to the unpartitioned spec, as before the index.
+func TestCommonMetadataPartitionSpecIndexFallsBackAfterDefaultElementMutation(t *testing.T) {
+ specs := partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 1, 2)
+ metadata := commonMetadata{
+ Specs: specs,
+ DefaultSpecID: specs[7].ID(),
+ partitionSpecIndex: buildPartitionSpecIndex(specs),
+ }
+
+ specs[7] = iceberg.NewPartitionSpecID(3)
+
+ assert.True(t, metadata.PartitionSpec().IsUnpartitioned())
+}
Review Comment:
**major** — New stale-default test is vacuous — passes with the mechanism it names deleted
The delta's only substantive addition claims to assert that PartitionSpec() 'must not return a stale indexed value' after an in-place mutation. The fixture makes that undecidable: partitionSpecIndexTestSpecsAtLeast(40, 1, 2) yields specs[7].ID()==1005, and the mutation writes iceberg.NewPartitionSpecID(3) — a fieldless spec, so IsUnpartitioned() is true for BOTH the stale indexed hit (ID 3) and the correct unpartitioned fallback (ID 0). The assertion therefore cannot fail. This matters because PartitionSpec()'s stale-default path above the 32-spec gate is exercised by no other test, so the coverage gap the test was added to close is still open. It is also the same inert-test class as my prior blocking finding, which this commit was written to answer. The production code is correct — only the test is inert. Fix: assert identity, e.g. assert.Equal(t, iceberg.UnpartitionedSpec.ID(), metadata.PartitionSpec().ID()).
<details><summary>Evidence</summary>
```text
Mutation (delete the `i >= 0 && i < len(specs) && specs[i].ID() == id` hit verification in partitionSpecIndexPosition, table/metadata.go:295-297), run per-test:
--- PASS: TestCommonMetadataPartitionSpecIndexFallsBackAfterDefaultElementMutation (0.00s) <- delta test survives
--- FAIL: TestCommonMetadataPartitionSpecIndexFallsBackAfterElementMutation
partition_spec_index_test.go:185: Expected nil, but got: &iceberg.PartitionSpec{id:3, fields:[]iceberg.PartitionField{}, ...} <- sibling has bite
Identity probe (table/pr1910_probe_test.go, since removed), same fixture:
under mutation: 'DefaultSpecID = 1005' / 'PartitionSpec() -> ID=3 IsUnpartitioned=true ; UnpartitionedSpec.ID=0' -> FAIL
on unmutated 5ea88a7: 'PartitionSpec() -> ID=0 IsUnpartitioned=true' -> PASS
So an ID assertion distinguishes the two states; IsUnpartitioned() cannot.
```
</details>
--
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): index partition specs by ID [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1910:
URL: https://github.com/apache/iceberg-go/pull/1910#discussion_r3874753286
##########
table/metadata.go:
##########
@@ -142,6 +142,88 @@ func snapshotIndexPosition(index *snapshotIndexData, snapshots []Snapshot, id in
return 0, false
}
+type partitionSpecIndexData struct {
+ positions map[int]int
+ // firstSpec identifies the spec slice used to build positions. It lets
+ // read-only lookups detect an index left behind by an in-package fixture
+ // that replaced the slice.
+ firstSpec *iceberg.PartitionSpec
+ // shared means positions is owned by more than one builder or metadata
+ // value and must be copied before a builder mutates it.
+ shared bool
+}
+
+func partitionSpecListFirst(specs []iceberg.PartitionSpec) *iceberg.PartitionSpec {
+ if len(specs) == 0 {
+ return nil
+ }
+
+ return &specs[0]
+}
+
+func buildPartitionSpecIndex(specs []iceberg.PartitionSpec) *partitionSpecIndexData {
+ positions := make(map[int]int, len(specs))
+ for i, spec := range specs {
+ if _, exists := positions[spec.ID()]; !exists {
+ positions[spec.ID()] = i
+ }
+ }
+
+ return &partitionSpecIndexData{positions: positions, firstSpec: partitionSpecListFirst(specs)}
+}
+
+func clonePartitionSpecIndex(index *partitionSpecIndexData) *partitionSpecIndexData {
+ if index == nil {
+ return nil
+ }
+
+ return &partitionSpecIndexData{
+ positions: maps.Clone(index.positions),
+ firstSpec: index.firstSpec,
+ }
+}
+
+func partitionSpecIndexNeedsRebuild(index *partitionSpecIndexData, specs []iceberg.PartitionSpec) bool {
+ if index == nil || len(index.positions) != len(specs) {
+ return true
+ }
+
+ return len(specs) > 0 && index.firstSpec != &specs[0]
+}
+
+// partitionSpecIndexPosition returns the position for id. Metadata loaded or
+// built through the normal paths always has a current index, so the linear
+// scan is only a compatibility fallback for in-package fixtures that replace
+// the slice.
+func partitionSpecIndexPosition(index *partitionSpecIndexData, specs []iceberg.PartitionSpec, id int) (int, bool) {
+ if index != nil {
+ if i, ok := index.positions[id]; ok {
+ if i >= 0 && i < len(specs) && specs[i].ID() == id {
+ return i, true
+ }
+
+ index = buildPartitionSpecIndex(specs)
+ if i, ok := index.positions[id]; ok {
+ return i, true
+ }
+
+ return 0, false
+ }
+
+ if !partitionSpecIndexNeedsRebuild(index, specs) {
Review Comment:
The comment above says the linear-scan fallback covers in-package fixtures that "replaced the slice", but it only actually covers whole-slice replacement. An element-wise mutation on the same backing array slips through and returns a silent false miss.
Start from specs `[A(0), B(1)]`, build the index, then do `specs[1] = C(5)` in place: a lookup for id 5 misses the map, `partitionSpecIndexNeedsRebuild` returns false (both `len` and `&specs[0]` are unchanged), and we return `(0, false)`, claiming C isn't there when it's sitting at position 1.
No production path mutates a spec slice element-wise like that today since builders always go through `AddPartitionSpec`/`RemovePartitionSpecs`, so this isn't live. But the comment promises more than the code delivers. I'd either tighten the comment to say we only detect whole-slice replacement, or fall through to the linear scan on a miss when the index looks current, though that second option turns every genuine miss into an O(n) scan, which partly defeats the miss fast-path the benchmark is measuring. A test doing `specs[1] = X` would pin whichever behavior we pick. wdyt?
##########
table/metadata.go:
##########
@@ -142,6 +142,88 @@ func snapshotIndexPosition(index *snapshotIndexData, snapshots []Snapshot, id in
return 0, false
}
+type partitionSpecIndexData struct {
+ positions map[int]int
+ // firstSpec identifies the spec slice used to build positions. It lets
+ // read-only lookups detect an index left behind by an in-package fixture
+ // that replaced the slice.
+ firstSpec *iceberg.PartitionSpec
+ // shared means positions is owned by more than one builder or metadata
+ // value and must be copied before a builder mutates it.
+ shared bool
+}
+
+func partitionSpecListFirst(specs []iceberg.PartitionSpec) *iceberg.PartitionSpec {
+ if len(specs) == 0 {
+ return nil
+ }
+
+ return &specs[0]
+}
+
+func buildPartitionSpecIndex(specs []iceberg.PartitionSpec) *partitionSpecIndexData {
+ positions := make(map[int]int, len(specs))
+ for i, spec := range specs {
Review Comment:
Small thing while we're here: `for i, spec := range specs` copies each `PartitionSpec` by value and then calls `spec.ID()` twice, while the fallback scan below uses `for i := range specs` + `specs[i].ID()`. Worth matching that here: `id := specs[i].ID()` once, index off `i`.
##########
table/metadata.go:
##########
@@ -419,6 +504,16 @@ func (b *MetadataBuilder) clone() *MetadataBuilder {
lastAddedPartitionID: clonePtr(b.lastAddedPartitionID),
lastAddedSortOrderID: clonePtr(b.lastAddedSortOrderID),
}
+ if b.partitionSpecIndex != nil {
Review Comment:
These are two separate `if b.partitionSpecIndex != nil` blocks with nothing mutating the field between them, so the second guard is dead: the split just makes a reader double-check that no reassignment sneaks in. I'd fold them into one block (build the clone and set `b.partitionSpecIndex.shared = true` together). The `snapshotIndex` clone right below has the same shape, so worth aligning both while we're here.
##########
table/metadata.go:
##########
@@ -1203,6 +1313,10 @@ func (b *MetadataBuilder) SetLastUpdatedMS() *MetadataBuilder {
}
func (b *MetadataBuilder) buildCommonMetadata() (*commonMetadata, error) {
+ b.ensurePartitionSpecIndex()
+ if b.partitionSpecIndex != nil {
Review Comment:
`ensurePartitionSpecIndex()` calls `buildPartitionSpecIndex`, which always returns a non-nil `&partitionSpecIndexData{}`, so `b.partitionSpecIndex` is guaranteed non-nil right after it returns and this guard can't fail. I'd drop the check and set `shared = true` unconditionally, since a nil-guard that never fires makes the real nil invariants harder to trust.
##########
table/partition_spec_index_bench_test.go:
##########
@@ -0,0 +1,132 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "strconv"
+ "testing"
+
+ "github.com/apache/iceberg-go"
+)
+
+var partitionSpecLookupBenchmarkSink int
+
+func BenchmarkPartitionSpecByID(b *testing.B) {
+ for _, specCount := range []int{4, 32, 256, 2_048} {
Review Comment:
The benchmark bottoms out at 4 specs and tops out at 2,048, but real tables rarely get past single-digit spec counts: the spec list only grows on intentional partition evolution, not per write. At N of 1-4 a map lookup with its hash and allocation can lose to a plain slice scan, so the interesting crossover is exactly the regime this skips.
I'd add N=1 and N=4 cases and report whether the index actually wins there. If it doesn't, it'd be worth gating the index behind a `len(specs)` threshold so small tables keep the cheaper scan, since the machinery here (pointer-identity check, COW flag, rebuild fallback) is a fair bit of surface to carry if the payoff only shows up at spec counts tables don't reach.
##########
table/metadata.go:
##########
@@ -1313,10 +1428,16 @@ func (b *MetadataBuilder) GetSchemaByID(id int) (*iceberg.Schema, error) {
}
func (b *MetadataBuilder) GetSpecByID(id int) (*iceberg.PartitionSpec, error) {
- for _, s := range b.specs {
- if s.ID() == id {
- return &s, nil
- }
+ index := b.partitionSpecIndex
+ if partitionSpecIndexNeedsRebuild(index, b.specs) {
Review Comment:
When the index is stale this builds a fresh one into a local and throws it away, so N lookups against a stale builder each pay the full O(n) rebuild instead of paying it once. The same discard-the-rebuild pattern is in `PartitionSpec` and `PartitionSpecByID`.
It's deliberate: `TestMetadataBuilderPartitionSpecIndexFallsBackAfterSliceReplacement` asserts `assert.Same` on the original index, so persisting would break that invariant. But `GetSpecByID` sits on hot paths (`SetDefaultSpecID`, the `AddPartitionSpec` dup check), so I'd either call `ensurePartitionSpecIndex()` to persist and relax that test, or drop a comment saying we intentionally don't persist on the fixture-replaced path. wdyt?
##########
table/partition_spec_index_test.go:
##########
@@ -0,0 +1,293 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "sync"
+ "testing"
+
+ "github.com/apache/iceberg-go"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+// Why: metadata lookups must use the derived ID index without changing the
+// existing default-spec fallback or missing-ID behavior.
+// Condition: metadata contains non-sequential partition spec IDs and the
+// index is initialized from the same slice.
+// Assertion: first, default, and missing lookups return the expected values.
+func TestCommonMetadataPartitionSpecIndexLookups(t *testing.T) {
+ specs := partitionSpecIndexTestSpecs(0, 7, 42)
+ metadata := commonMetadata{
+ Specs: specs,
+ DefaultSpecID: 42,
+ partitionSpecIndex: buildPartitionSpecIndex(specs),
+ }
+
+ byID := metadata.PartitionSpecByID(7)
+ require.NotNil(t, byID)
+ assert.Equal(t, 7, byID.ID())
+
+ defaultSpec := metadata.PartitionSpec()
+ assert.Equal(t, 42, defaultSpec.ID())
+ assert.Nil(t, metadata.PartitionSpecByID(99))
+}
+
+// Why: decoded metadata must initialize the same derived index as metadata
+// built by a writer, rather than rebuilding it on every lookup.
+// Condition: parse a valid metadata document containing partition specs.
+// Assertion: the decoded common metadata owns one index entry per spec.
+func TestParsedMetadataBuildsPartitionSpecIndex(t *testing.T) {
+ metadata, err := ParseMetadataBytes([]byte(ExampleTableMetadataV2))
+ require.NoError(t, err)
+
+ common := metadataCommon(metadata)
+ require.Len(t, common.partitionSpecIndex.positions, len(common.Specs))
+ for i, spec := range common.Specs {
+ require.Equal(t, i, common.partitionSpecIndex.positions[spec.ID()])
+ }
+}
+
+// Why: builders created from existing metadata must get an index before their
+// first partition-spec lookup.
+// Condition: create a builder from parsed metadata and look up its final spec.
+// Assertion: the builder index covers every copied partition spec.
+func TestMetadataBuilderFromBaseBuildsPartitionSpecIndex(t *testing.T) {
+ metadata, err := ParseMetadataBytes([]byte(ExampleTableMetadataV2))
+ require.NoError(t, err)
+
+ builder, err := MetadataBuilderFromBase(metadata, "")
+ require.NoError(t, err)
+ require.Len(t, builder.partitionSpecIndex.positions, len(builder.specs))
+
+ id := builder.specs[len(builder.specs)-1].ID()
+ spec, err := builder.GetSpecByID(id)
+ require.NoError(t, err)
+ require.NotNil(t, spec)
+ assert.Equal(t, id, spec.ID())
+}
+
+// Why: in-package fixtures can replace metadata slices directly, so a stale
+// derived index must not return a spec at the old position or hide a new one.
+// Condition: the indexed slice is replaced with another slice of equal length.
+// Assertion: lookups fall back to the replacement slice while the original
+// index remains read-only.
+func TestCommonMetadataPartitionSpecIndexFallsBackAfterSliceReplacement(t *testing.T) {
+ specs := partitionSpecIndexTestSpecs(1, 2)
+ metadata := commonMetadata{
+ Specs: specs,
+ DefaultSpecID: 2,
+ partitionSpecIndex: buildPartitionSpecIndex(specs),
+ }
+ originalIndex := metadata.partitionSpecIndex
+
+ metadata.Specs = partitionSpecIndexTestSpecs(3, 4)
+ metadata.DefaultSpecID = 4
+ byID := metadata.PartitionSpecByID(4)
+ require.NotNil(t, byID)
+ assert.Equal(t, 4, byID.ID())
+ defaultSpec := metadata.PartitionSpec()
+ assert.Equal(t, 4, defaultSpec.ID())
+ assert.Nil(t, metadata.PartitionSpecByID(2))
+ assert.Same(t, originalIndex, metadata.partitionSpecIndex)
+ assert.Equal(t, map[int]int{1: 0, 2: 1}, originalIndex.positions)
+}
+
+// Why: builders can also be used by package-level fixtures that replace their
+// spec slice without updating derived state.
+// Condition: an indexed builder receives a replacement slice of equal length.
+// Assertion: GetSpecByID resolves the replacement slice and leaves the old
+// index untouched.
+func TestMetadataBuilderPartitionSpecIndexFallsBackAfterSliceReplacement(t *testing.T) {
+ specs := partitionSpecIndexTestSpecs(1, 2)
+ builder := MetadataBuilder{
+ specs: specs,
+ partitionSpecIndex: buildPartitionSpecIndex(specs),
+ }
+ originalIndex := builder.partitionSpecIndex
+
+ builder.specs = partitionSpecIndexTestSpecs(3, 4)
+ byID, err := builder.GetSpecByID(4)
+ require.NoError(t, err)
+ require.NotNil(t, byID)
+ assert.Equal(t, 4, byID.ID())
+ _, err = builder.GetSpecByID(2)
+ assert.ErrorIs(t, err, ErrPartitionSpecNotFound)
+ assert.Same(t, originalIndex, builder.partitionSpecIndex)
+ assert.Equal(t, map[int]int{1: 0, 2: 1}, originalIndex.positions)
+}
+
+// Why: builder updates and clones must keep the spec index aligned without
+// sharing mutable lookup state with a built metadata value or sibling builder.
+// Condition: add a spec, clone the builder, add another spec to the clone, and
+// remove the first added spec from the original.
+// Assertion: each builder resolves the IDs in its own current spec slice.
+func TestMetadataBuilderPartitionSpecIndexFollowsUpdates(t *testing.T) {
+ builder := builderWithoutChanges(2)
+ require.Len(t, builder.specs, 1)
+ require.Equal(t, map[int]int{0: 0}, builder.partitionSpecIndex.positions)
+
+ added := iceberg.NewPartitionSpecID(99, iceberg.PartitionField{
+ SourceIDs: []int{1}, Name: "x", Transform: iceberg.IdentityTransform{},
+ })
+ require.NoError(t, builder.AddPartitionSpec(&added, false))
+ require.Equal(t, 1, builder.partitionSpecIndex.positions[1])
+
+ got, err := builder.GetSpecByID(1)
+ require.NoError(t, err)
+ require.NotNil(t, got)
+
+ clone := builder.clone()
+ cloneAdded := iceberg.NewPartitionSpecID(99, iceberg.PartitionField{
+ SourceIDs: []int{3}, Name: "z", Transform: iceberg.IdentityTransform{},
+ })
+ require.NoError(t, clone.AddPartitionSpec(&cloneAdded, false))
+ assert.NotContains(t, builder.partitionSpecIndex.positions, 2)
+ assert.Equal(t, 2, clone.partitionSpecIndex.positions[2])
+
+ require.NoError(t, builder.RemovePartitionSpecs([]int{1}))
+ assert.NotContains(t, builder.partitionSpecIndex.positions, 1)
+ _, err = builder.GetSpecByID(1)
+ assert.ErrorIs(t, err, ErrPartitionSpecNotFound)
+ got, err = clone.GetSpecByID(1)
+ require.NoError(t, err)
+ assert.Equal(t, 1, got.ID())
+}
+
+// Why: removing unknown IDs is a no-op and must not leave the derived index
+// pointing at a newly allocated but unindexed slice.
+// Condition: remove an ID that is not present in the builder.
+// Assertion: the specs slice and its index remain unchanged.
+func TestMetadataBuilderRemoveUnknownPartitionSpecKeepsIndex(t *testing.T) {
+ builder := builderWithoutChanges(2)
+ originalIndex := builder.partitionSpecIndex
+ originalFirst := &builder.specs[0]
+
+ require.NoError(t, builder.RemovePartitionSpecs([]int{99}))
+ assert.Same(t, originalIndex, builder.partitionSpecIndex)
+ assert.Same(t, originalFirst, &builder.specs[0])
+}
+
+// Why: a built metadata value shares the derived index with its builder until
+// the builder mutates its spec list.
+// Condition: build metadata, then add a new partition spec to the builder.
+// Assertion: the builder sees the new spec while the already-built metadata
+// remains unchanged.
+func TestMetadataBuilderPartitionSpecIndexIsolatedFromBuiltMetadata(t *testing.T) {
+ builder := builderWithoutChanges(2)
+ metadata, err := builder.Build()
+ require.NoError(t, err)
+ common := metadataCommon(metadata)
+ originalIndex := common.partitionSpecIndex
+
+ added := iceberg.NewPartitionSpecID(99, iceberg.PartitionField{
+ SourceIDs: []int{3}, Name: "z", Transform: iceberg.IdentityTransform{},
+ })
+ require.NoError(t, builder.AddPartitionSpec(&added, false))
+
+ assert.NotSame(t, originalIndex, builder.partitionSpecIndex)
+ assert.Contains(t, builder.partitionSpecIndex.positions, 1)
+ assert.NotContains(t, originalIndex.positions, 1)
+ assert.Nil(t, common.PartitionSpecByID(1))
+ got, err := builder.GetSpecByID(1)
+ require.NoError(t, err)
+ assert.Equal(t, 1, got.ID())
+}
+
+func TestCommonMetadataPartitionSpecLookupsConcurrent(t *testing.T) {
Review Comment:
These two concurrent tests don't actually exercise the copy-on-write path they look like they're guarding.
Every goroutine only reads from an index that's built once and never mutated: the `positions` map is written before any goroutine starts and `shared`/`firstSpec` are never touched, so the Go memory model already makes these reads safe. A `-race` run here would pass even if we deleted the `shared` flag and the whole clone-on-write dance.
The race that matters is a builder marking its index `shared` in `Build()`, then `AddPartitionSpec` triggering `ensurePartitionSpecIndexMutable` (which clones) while another goroutine reads the metadata built before the mutation. I'd rework one of these to spin up a reader on the built `commonMetadata` and concurrently call `builder.AddPartitionSpec` on the builder that produced it, which is what proves the isolation holds under `-race`. If the intent is only "concurrent reads after init are safe," a comment saying so would keep the next reader from trusting it for more than it tests.
--
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): index partition specs by ID [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1910:
URL: https://github.com/apache/iceberg-go/pull/1910
--
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): index partition specs by ID [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1910:
URL: https://github.com/apache/iceberg-go/pull/1910#discussion_r3926374352
##########
table/metadata.go:
##########
@@ -216,6 +216,98 @@ func schemaIndexLookup(index *schemaIndexData, schemas []*iceberg.Schema, id int
return nil, false
}
+type partitionSpecIndexData struct {
+ positions map[int]int
+ // firstSpec identifies the first element of the spec slice used to build
+ // positions. It lets read-only lookups detect an index left behind by an
+ // in-package fixture that replaced the slice.
+ firstSpec *iceberg.PartitionSpec
+ // shared means positions is owned by more than one builder or metadata
+ // value and must be copied before a builder mutates it.
+ shared bool
+}
+
+// Small spec slices are faster to search directly than to hash-map lookup.
+// 32 is a conservative cutoff; keep it aligned with the lookup benchmarks.
+const partitionSpecIndexMinSize = 32
+
+func partitionSpecListFirst(specs []iceberg.PartitionSpec) *iceberg.PartitionSpec {
+ if len(specs) == 0 {
+ return nil
+ }
+
+ return &specs[0]
+}
+
+func buildPartitionSpecIndex(specs []iceberg.PartitionSpec) *partitionSpecIndexData {
+ if len(specs) < partitionSpecIndexMinSize {
+ return nil
+ }
+
+ positions := make(map[int]int, len(specs))
+ for i := range specs {
+ id := specs[i].ID()
+ if _, exists := positions[id]; !exists {
+ positions[id] = i
+ }
+ }
+
+ return &partitionSpecIndexData{
+ positions: positions,
+ firstSpec: partitionSpecListFirst(specs),
+ }
+}
+
+func clonePartitionSpecIndex(index *partitionSpecIndexData) *partitionSpecIndexData {
+ if index == nil {
+ return nil
+ }
+
+ return &partitionSpecIndexData{
+ positions: maps.Clone(index.positions),
+ firstSpec: index.firstSpec,
+ }
+}
+
+func partitionSpecIndexNeedsRebuild(index *partitionSpecIndexData, specs []iceberg.PartitionSpec) bool {
+ if len(specs) < partitionSpecIndexMinSize {
+ return false
+ }
+ if index == nil {
+ return true
+ }
+ // Persisted metadata rejects duplicate IDs, so map and source cardinality
+ // are equal on the indexed read path.
+ if index.positions == nil || len(index.positions) != len(specs) {
+ return true
+ }
+
+ return len(specs) > 0 && index.firstSpec != &specs[0]
+}
Review Comment:
**nit** — Unreachable sub-expressions in the new helpers
At :285 'len(specs) > 0 &&' can never be false - partitionSpecIndexNeedsRebuild already returned for len(specs) < partitionSpecIndexMinSize. At :296 'i >= 0' is dead for map-sourced positions. Both shapes are inherited from snapshotIndexNeedsRebuild (:109) and snapshotIndexPosition (:118), where the length guard IS live because the snapshot index has no size gate; keeping them here is defensible for symmetry but a one-line note would stop a reader inferring the gate doesn't exist.
##########
table/metadata.go:
##########
@@ -742,14 +857,23 @@ func (b *MetadataBuilder) AddPartitionSpec(spec *iceberg.PartitionSpec, initial
}
lastPartitionID := max(maxFieldID, prev)
- var specs []iceberg.PartitionSpec
if initial {
- specs = []iceberg.PartitionSpec{freshSpec}
+ b.specs = []iceberg.PartitionSpec{freshSpec}
+ b.partitionSpecIndex = buildPartitionSpecIndex(b.specs)
} else {
- specs = append(b.specs, freshSpec)
+ b.ensurePartitionSpecIndexMutable()
+ b.specs = append(b.specs, freshSpec)
+ if len(b.specs) >= partitionSpecIndexMinSize {
+ if len(b.specs) == partitionSpecIndexMinSize ||
+ b.partitionSpecIndex == nil || b.partitionSpecIndex.positions == nil {
+ b.partitionSpecIndex = buildPartitionSpecIndex(b.specs)
Review Comment:
**minor** — Dead nil sub-guards in AddPartitionSpec's incremental index update
'b.partitionSpecIndex == nil || b.partitionSpecIndex.positions == nil' cannot hold when len(b.specs) > partitionSpecIndexMinSize: ensurePartitionSpecIndexMutable() two lines above calls ensurePartitionSpecIndex(), and for any pre-append length >= 32 partitionSpecIndexNeedsRebuild returns true for both a nil index and a nil positions map, so buildPartitionSpecIndex has already installed a non-nil index with a non-nil map. Reduce the condition to 'len(b.specs) == partitionSpecIndexMinSize'. This is the same class of never-firing guard already removed twice in this PR at `laskoviymishka`'s request.
##########
table/metadata.go:
##########
@@ -216,6 +216,98 @@ func schemaIndexLookup(index *schemaIndexData, schemas []*iceberg.Schema, id int
return nil, false
}
+type partitionSpecIndexData struct {
+ positions map[int]int
+ // firstSpec identifies the first element of the spec slice used to build
+ // positions. It lets read-only lookups detect an index left behind by an
+ // in-package fixture that replaced the slice.
+ firstSpec *iceberg.PartitionSpec
+ // shared means positions is owned by more than one builder or metadata
+ // value and must be copied before a builder mutates it.
+ shared bool
+}
+
+// Small spec slices are faster to search directly than to hash-map lookup.
+// 32 is a conservative cutoff; keep it aligned with the lookup benchmarks.
+const partitionSpecIndexMinSize = 32
+
Review Comment:
**minor** — partitionSpecIndexMinSize = 32 is not supported by the benchmarks its own comment cites
The comment says '32 is a conservative cutoff; keep it aligned with the lookup benchmarks', but at exactly 32 specs the map gives no measurable hit benefit and makes misses materially slower (a miss pays the map probe and then the full scan, by design). The first size where hits demonstrably win is 64. Either raise the constant to 64 or drop the claim that it tracks the benchmarks. No correctness impact and no regression versus main - head beats base at every size >= 32.
##########
table/metadata.go:
##########
@@ -2292,23 +2425,34 @@ func (c *commonMetadata) DefaultPartitionSpec() int {
return c.DefaultSpecID
}
+func (c *commonMetadata) partitionSpecIndexForLookup() *partitionSpecIndexData {
+ index := c.partitionSpecIndex
Review Comment:
**nit** — Single-spec lookups are ~0.5 ns slower than base
The extra partitionSpecIndexForLookup + partitionSpecIndexNeedsRebuild calls cost about half a nanosecond on the most common table shape. Statistically strong but practically irrelevant next to the 96 B / 2 allocs of the defensive clonePartitionSpec on the hit path. Noted only so the description's framing doesn't imply a strict improvement everywhere; no action needed.
##########
table/partition_spec_index_test.go:
##########
@@ -0,0 +1,430 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "encoding/json"
+ "sync"
+ "testing"
+
+ "github.com/apache/iceberg-go"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+// Why: metadata lookups must use the derived ID index without changing the
+// existing default-spec fallback or missing-ID behavior.
+// Condition: metadata contains non-sequential partition spec IDs and the
+// index is initialized from the same slice.
+// Assertion: first, default, and missing lookups return the expected values.
+func TestCommonMetadataPartitionSpecIndexLookups(t *testing.T) {
+ specs := partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 0, 7, 42)
+ metadata := commonMetadata{
+ Specs: specs,
+ DefaultSpecID: 42,
+ partitionSpecIndex: buildPartitionSpecIndex(specs),
+ }
+
+ byID := metadata.PartitionSpecByID(7)
+ require.NotNil(t, byID)
+ assert.Equal(t, 7, byID.ID())
+
+ defaultSpec := metadata.PartitionSpec()
+ assert.Equal(t, 42, defaultSpec.ID())
+ assert.Nil(t, metadata.PartitionSpecByID(99))
+}
+
+// Why: small metadata should not allocate an index that its linear lookup path
+// will never read.
+// Condition: parse a valid metadata document containing one partition spec.
+// Assertion: the decoded common metadata keeps only the slice identity.
+func TestParsedMetadataSkipsSmallPartitionSpecIndex(t *testing.T) {
+ metadata, err := ParseMetadataBytes([]byte(ExampleTableMetadataV2))
+ require.NoError(t, err)
+
+ common := metadataCommon(metadata)
+ require.Nil(t, common.partitionSpecIndex)
+}
+
+// Why: builders created from existing metadata should use the same small-slice
+// lookup policy as parsed metadata.
+// Condition: create a builder from parsed metadata and look up its final spec.
+// Assertion: the lookup works without allocating a map for one spec.
+func TestMetadataBuilderFromBaseSkipsSmallPartitionSpecIndex(t *testing.T) {
+ metadata, err := ParseMetadataBytes([]byte(ExampleTableMetadataV2))
+ require.NoError(t, err)
+
+ builder, err := MetadataBuilderFromBase(metadata, "")
+ require.NoError(t, err)
+ require.Nil(t, builder.partitionSpecIndex)
+
+ id := builder.specs[len(builder.specs)-1].ID()
+ spec, err := builder.GetSpecByID(id)
+ require.NoError(t, err)
+ require.NotNil(t, spec)
+ assert.Equal(t, id, spec.ID())
+}
+
+// Why: in-package fixtures can replace metadata slices directly, so a stale
+// derived index must not return a spec at the old position or hide a new one.
+// Condition: the indexed slice is replaced with another slice of equal length.
+// Assertion: lookups use the replacement slice without mutating the cached
+// index.
+func TestCommonMetadataPartitionSpecIndexFallsBackAfterSliceReplacement(t *testing.T) {
+ specs := partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 1, 2)
+ metadata := commonMetadata{
+ Specs: specs,
+ DefaultSpecID: 2,
+ partitionSpecIndex: buildPartitionSpecIndex(specs),
+ }
+ originalIndex := metadata.partitionSpecIndex
+
+ metadata.Specs = partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 3, 4)
+ metadata.DefaultSpecID = 4
+ byID := metadata.PartitionSpecByID(4)
+ require.NotNil(t, byID)
+ assert.Equal(t, 4, byID.ID())
+ defaultSpec := metadata.PartitionSpec()
+ assert.Equal(t, 4, defaultSpec.ID())
+ assert.Nil(t, metadata.PartitionSpecByID(2))
+ assert.Same(t, originalIndex, metadata.partitionSpecIndex)
+ assert.Equal(t, 0, metadata.partitionSpecIndex.positions[1])
+ assert.Equal(t, 1, metadata.partitionSpecIndex.positions[2])
+}
+
+func TestCommonMetadataPartitionSpecIndexFallsBackAfterSliceReplacementConcurrent(t *testing.T) {
+ metadata := &commonMetadata{
+ Specs: partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 1, 2),
+ partitionSpecIndex: buildPartitionSpecIndex(partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 1, 2)),
+ }
+ metadata.Specs = partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 3, 4)
+ originalIndex := metadata.partitionSpecIndex
+
+ const goroutineCount = 8
+ var wg sync.WaitGroup
+ wg.Add(goroutineCount)
+ for range goroutineCount {
+ go func() {
+ defer wg.Done()
+ for range 100 {
+ if spec := metadata.PartitionSpecByID(4); spec == nil || spec.ID() != 4 {
+ t.Errorf("expected partition spec 4, got %v", spec)
+ }
+ }
+ }()
+ }
+ wg.Wait()
+
+ assert.Same(t, originalIndex, metadata.partitionSpecIndex)
+}
+
+func TestRejectsDuplicatePartitionSpecIDs(t *testing.T) {
+ var raw map[string]json.RawMessage
+ require.NoError(t, json.Unmarshal([]byte(ExampleTableMetadataV2), &raw))
+
+ var specs []json.RawMessage
+ require.NoError(t, json.Unmarshal(raw["partition-specs"], &specs))
+ specs = append(specs, specs[0])
+ encodedSpecs, err := json.Marshal(specs)
+ require.NoError(t, err)
+ raw["partition-specs"] = encodedSpecs
+ data, err := json.Marshal(raw)
+ require.NoError(t, err)
+
+ _, err = ParseMetadataBytes(data)
+ require.ErrorIs(t, err, ErrInvalidMetadata)
+ assert.ErrorContains(t, err, "duplicate partition spec ID 0")
+}
+
+// Why: builder-produced metadata must enforce the same unique partition spec
+// ID invariant as metadata read from JSON.
+// Condition: an in-package builder is given duplicate spec IDs before Build.
+// Assertion: Build rejects the metadata instead of publishing an ambiguous index.
+func TestMetadataBuilderBuildRejectsDuplicatePartitionSpecIDs(t *testing.T) {
+ builder := builderWithoutChanges(2)
+ builder.specs = append(builder.specs, builder.specs[0])
+
+ _, err := builder.Build()
+ require.ErrorIs(t, err, ErrInvalidMetadata)
+ assert.ErrorContains(t, err, "duplicate partition spec ID 0")
+}
+
+// Why: in-package fixtures can mutate an existing spec slice without changing
+// its length or backing array, which cannot be detected by index metadata alone.
+// Condition: a spec is replaced in place after the index is built.
+// Assertion: lookup still finds the replacement and does not return the old ID.
+func TestCommonMetadataPartitionSpecIndexFallsBackAfterElementMutation(t *testing.T) {
+ specs := partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 1, 2)
+ oldID := specs[7].ID()
+ metadata := commonMetadata{
+ Specs: specs,
+ DefaultSpecID: 1,
+ partitionSpecIndex: buildPartitionSpecIndex(specs),
+ }
+
+ specs[7] = iceberg.NewPartitionSpecID(3)
+
+ byID := metadata.PartitionSpecByID(3)
+ require.NotNil(t, byID)
+ assert.Equal(t, 3, byID.ID())
+ assert.Nil(t, metadata.PartitionSpecByID(oldID))
+}
+
+// Why: builders can also be used by package-level fixtures that replace their
+// spec slice without updating derived state.
+// Condition: an indexed builder receives a replacement slice of equal length.
+// Assertion: GetSpecByID resolves the replacement slice and refreshes its index.
+func TestMetadataBuilderPartitionSpecIndexFallsBackAfterSliceReplacement(t *testing.T) {
+ specs := partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 1, 2)
+ builder := MetadataBuilder{
+ specs: specs,
+ partitionSpecIndex: buildPartitionSpecIndex(specs),
+ }
+ originalIndex := builder.partitionSpecIndex
+
+ builder.specs = partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 3, 4)
+ byID, err := builder.GetSpecByID(4)
+ require.NoError(t, err)
+ require.NotNil(t, byID)
+ assert.Equal(t, 4, byID.ID())
+ _, err = builder.GetSpecByID(2)
+ assert.ErrorIs(t, err, ErrPartitionSpecNotFound)
+ assert.NotSame(t, originalIndex, builder.partitionSpecIndex)
+ assert.Equal(t, 0, builder.partitionSpecIndex.positions[3])
+ assert.Equal(t, 1, builder.partitionSpecIndex.positions[4])
+ assert.Equal(t, 0, originalIndex.positions[1])
+ assert.Equal(t, 1, originalIndex.positions[2])
+}
+
+// Why: builder updates and clones must keep the spec index aligned without
+// sharing mutable lookup state with a built metadata value or sibling builder.
+// Condition: add a spec, clone the builder, add another spec to the clone, and
+// remove the first added spec from the original.
+// Assertion: each builder resolves the IDs in its own current spec slice.
+func TestMetadataBuilderPartitionSpecIndexFollowsUpdates(t *testing.T) {
+ builder := builderWithoutChanges(2)
+ for id := 1; len(builder.specs) < partitionSpecIndexMinSize; id++ {
+ builder.specs = append(builder.specs, iceberg.NewPartitionSpecID(id))
+ }
+ builder.partitionSpecIndex = buildPartitionSpecIndex(builder.specs)
+ require.Len(t, builder.specs, partitionSpecIndexMinSize)
+ require.Equal(t, 0, builder.partitionSpecIndex.positions[0])
+
+ added := iceberg.NewPartitionSpecID(99, iceberg.PartitionField{
+ SourceIDs: []int{1}, Name: "x", Transform: iceberg.IdentityTransform{},
+ })
+ require.NoError(t, builder.AddPartitionSpec(&added, false))
+ require.Equal(t, partitionSpecIndexMinSize, builder.partitionSpecIndex.positions[partitionSpecIndexMinSize])
+
+ got, err := builder.GetSpecByID(partitionSpecIndexMinSize)
+ require.NoError(t, err)
+ require.NotNil(t, got)
+ assert.Equal(t, partitionSpecIndexMinSize, got.ID())
+
+ clone := builder.clone()
+ cloneAdded := iceberg.NewPartitionSpecID(99, iceberg.PartitionField{
+ SourceIDs: []int{3}, Name: "z", Transform: iceberg.IdentityTransform{},
+ })
+ require.NoError(t, clone.AddPartitionSpec(&cloneAdded, false))
+ assert.NotContains(t, builder.partitionSpecIndex.positions, partitionSpecIndexMinSize+1)
+ assert.Equal(t, partitionSpecIndexMinSize+1, clone.partitionSpecIndex.positions[partitionSpecIndexMinSize+1])
+
+ require.NoError(t, builder.RemovePartitionSpecs([]int{1}))
+ assert.NotContains(t, builder.partitionSpecIndex.positions, 1)
+ _, err = builder.GetSpecByID(1)
+ assert.ErrorIs(t, err, ErrPartitionSpecNotFound)
+ got, err = clone.GetSpecByID(1)
+ require.NoError(t, err)
+ assert.Equal(t, 1, got.ID())
+}
+
+func TestMetadataBuilderPartitionSpecIndexBuildsAtThreshold(t *testing.T) {
+ builder := builderWithoutChanges(2)
+ for id := 1; len(builder.specs) < partitionSpecIndexMinSize-1; id++ {
+ builder.specs = append(builder.specs, iceberg.NewPartitionSpecID(id))
+ }
+ builder.partitionSpecIndex = buildPartitionSpecIndex(builder.specs)
+ require.Nil(t, builder.partitionSpecIndex)
+
+ added := iceberg.NewPartitionSpecID(99, iceberg.PartitionField{
+ SourceIDs: []int{1}, Name: "threshold", Transform: iceberg.IdentityTransform{},
+ })
+ require.NoError(t, builder.AddPartitionSpec(&added, false))
+ require.NotNil(t, builder.partitionSpecIndex.positions)
+ assert.Equal(t, partitionSpecIndexMinSize-1,
+ builder.partitionSpecIndex.positions[partitionSpecIndexMinSize-1])
+}
+
+// Why: removing unknown IDs is a no-op and must not leave the derived index
+// pointing at a newly allocated but unindexed slice.
+// Condition: remove an ID that is not present in the builder.
+// Assertion: the specs slice and its index remain unchanged.
+func TestMetadataBuilderRemoveUnknownPartitionSpecKeepsIndex(t *testing.T) {
+ builder := builderWithoutChanges(2)
+ originalIndex := builder.partitionSpecIndex
+ originalFirst := &builder.specs[0]
+
+ require.NoError(t, builder.RemovePartitionSpecs([]int{99}))
+ assert.Same(t, originalIndex, builder.partitionSpecIndex)
+ assert.Same(t, originalFirst, &builder.specs[0])
+}
+
+// Why: a built metadata value shares the derived index with its builder until
+// the builder mutates its spec list.
+// Condition: build metadata, then add a new partition spec to the builder.
+// Assertion: the builder sees the new spec while the already-built metadata
+// remains unchanged.
+func TestMetadataBuilderPartitionSpecIndexIsolatedFromBuiltMetadata(t *testing.T) {
+ builder := builderWithoutChanges(2)
+ for id := 1; len(builder.specs) < partitionSpecIndexMinSize; id++ {
+ builder.specs = append(builder.specs, iceberg.NewPartitionSpecID(id))
+ }
+ builder.partitionSpecIndex = buildPartitionSpecIndex(builder.specs)
+ metadata, err := builder.Build()
+ require.NoError(t, err)
+ common := metadataCommon(metadata)
+ originalIndex := common.partitionSpecIndex
+
+ added := iceberg.NewPartitionSpecID(99, iceberg.PartitionField{
+ SourceIDs: []int{3}, Name: "z", Transform: iceberg.IdentityTransform{},
+ })
+ require.NoError(t, builder.AddPartitionSpec(&added, false))
+
+ assert.NotSame(t, originalIndex, builder.partitionSpecIndex)
+ assert.NotContains(t, originalIndex.positions, partitionSpecIndexMinSize)
+ assert.Contains(t, builder.partitionSpecIndex.positions, partitionSpecIndexMinSize)
+ assert.Nil(t, common.PartitionSpecByID(partitionSpecIndexMinSize))
+ got, err := builder.GetSpecByID(1)
+ require.NoError(t, err)
+ assert.Equal(t, 1, got.ID())
+}
+
+func TestCommonMetadataPartitionSpecLookupsConcurrent(t *testing.T) {
+ specs := partitionSpecIndexTestSpecs(1, 2)
Review Comment:
**minor** — TestCommonMetadataPartitionSpecLookupsConcurrent exercises none of the new index machinery
The fixture is partitionSpecIndexTestSpecs(1, 2), which is below the construction gate, so metadata.partitionSpecIndex is nil, partitionSpecIndexForLookup returns immediately without rebuilding, and partitionSpecIndexPosition's map branch is skipped. The test only proves concurrent linear scans are safe - which the Go memory model already gives for free. This is the one test from `laskoviymishka`'s round-1 comment that wasn't lifted above the threshold (its sibling ...CopyOnWriteConcurrent was, and has bite). Size it from partitionSpecIndexMinSize like the other fixtures, or rename/comment it to say it covers unindexed concurrent reads.
##########
table/metadata.go:
##########
@@ -2703,11 +2848,25 @@ func (c *commonMetadata) checkSchemas() error {
}
func (c *commonMetadata) checkPartitionSpecs() error {
- for _, spec := range c.Specs {
- if spec.ID() == c.DefaultSpecID {
- return nil
+ // Partition spec IDs are unique in persisted metadata. Keep this validation
+ // aligned with the schema and snapshot ID checks so normal read paths never
+ // need to tolerate duplicate IDs.
+ seen := make(map[int]struct{}, len(c.Specs))
+ defaultFound := false
+ for i := range c.Specs {
+ id := c.Specs[i].ID()
+ if _, ok := seen[id]; ok {
+ return fmt.Errorf("%w: duplicate partition spec ID %d", ErrInvalidMetadata, id)
+ }
+ seen[id] = struct{}{}
+
Review Comment:
**nit** — Description omits the threshold, the loop rewrite, and the new load-time hard failure
Three gaps worth closing before merge: (a) the 32 threshold - and the fact the index is not even built below it - is never named, only 'Small slices use the existing linear-scan path'; (b) no mention of the 'for _, s := range' -> 'for i := range' rewrite, which now supplies 100% of the win for realistic single-digit spec counts; (c) rejecting duplicate partition spec IDs turns previously-loadable metadata into a hard ErrInvalidMetadata at parse and at Build - correct and matching Java per the settled design discussion, but a user-visible compatibility change that deserves a release note. Error precedence also changed within the same sentinel.
##########
table/partition_spec_index_test.go:
##########
@@ -0,0 +1,430 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "encoding/json"
+ "sync"
+ "testing"
+
+ "github.com/apache/iceberg-go"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+// Why: metadata lookups must use the derived ID index without changing the
+// existing default-spec fallback or missing-ID behavior.
+// Condition: metadata contains non-sequential partition spec IDs and the
+// index is initialized from the same slice.
+// Assertion: first, default, and missing lookups return the expected values.
+func TestCommonMetadataPartitionSpecIndexLookups(t *testing.T) {
+ specs := partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 0, 7, 42)
+ metadata := commonMetadata{
+ Specs: specs,
+ DefaultSpecID: 42,
+ partitionSpecIndex: buildPartitionSpecIndex(specs),
+ }
+
+ byID := metadata.PartitionSpecByID(7)
+ require.NotNil(t, byID)
+ assert.Equal(t, 7, byID.ID())
+
+ defaultSpec := metadata.PartitionSpec()
+ assert.Equal(t, 42, defaultSpec.ID())
+ assert.Nil(t, metadata.PartitionSpecByID(99))
+}
+
+// Why: small metadata should not allocate an index that its linear lookup path
+// will never read.
+// Condition: parse a valid metadata document containing one partition spec.
+// Assertion: the decoded common metadata keeps only the slice identity.
+func TestParsedMetadataSkipsSmallPartitionSpecIndex(t *testing.T) {
+ metadata, err := ParseMetadataBytes([]byte(ExampleTableMetadataV2))
+ require.NoError(t, err)
+
+ common := metadataCommon(metadata)
+ require.Nil(t, common.partitionSpecIndex)
+}
+
+// Why: builders created from existing metadata should use the same small-slice
+// lookup policy as parsed metadata.
+// Condition: create a builder from parsed metadata and look up its final spec.
+// Assertion: the lookup works without allocating a map for one spec.
+func TestMetadataBuilderFromBaseSkipsSmallPartitionSpecIndex(t *testing.T) {
+ metadata, err := ParseMetadataBytes([]byte(ExampleTableMetadataV2))
+ require.NoError(t, err)
+
+ builder, err := MetadataBuilderFromBase(metadata, "")
+ require.NoError(t, err)
+ require.Nil(t, builder.partitionSpecIndex)
+
+ id := builder.specs[len(builder.specs)-1].ID()
+ spec, err := builder.GetSpecByID(id)
+ require.NoError(t, err)
+ require.NotNil(t, spec)
+ assert.Equal(t, id, spec.ID())
+}
+
+// Why: in-package fixtures can replace metadata slices directly, so a stale
+// derived index must not return a spec at the old position or hide a new one.
+// Condition: the indexed slice is replaced with another slice of equal length.
+// Assertion: lookups use the replacement slice without mutating the cached
+// index.
+func TestCommonMetadataPartitionSpecIndexFallsBackAfterSliceReplacement(t *testing.T) {
+ specs := partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 1, 2)
+ metadata := commonMetadata{
+ Specs: specs,
+ DefaultSpecID: 2,
+ partitionSpecIndex: buildPartitionSpecIndex(specs),
+ }
+ originalIndex := metadata.partitionSpecIndex
+
+ metadata.Specs = partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 3, 4)
+ metadata.DefaultSpecID = 4
+ byID := metadata.PartitionSpecByID(4)
+ require.NotNil(t, byID)
+ assert.Equal(t, 4, byID.ID())
+ defaultSpec := metadata.PartitionSpec()
+ assert.Equal(t, 4, defaultSpec.ID())
+ assert.Nil(t, metadata.PartitionSpecByID(2))
+ assert.Same(t, originalIndex, metadata.partitionSpecIndex)
+ assert.Equal(t, 0, metadata.partitionSpecIndex.positions[1])
+ assert.Equal(t, 1, metadata.partitionSpecIndex.positions[2])
+}
+
+func TestCommonMetadataPartitionSpecIndexFallsBackAfterSliceReplacementConcurrent(t *testing.T) {
+ metadata := &commonMetadata{
+ Specs: partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 1, 2),
+ partitionSpecIndex: buildPartitionSpecIndex(partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 1, 2)),
+ }
+ metadata.Specs = partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 3, 4)
+ originalIndex := metadata.partitionSpecIndex
+
+ const goroutineCount = 8
+ var wg sync.WaitGroup
+ wg.Add(goroutineCount)
+ for range goroutineCount {
+ go func() {
+ defer wg.Done()
+ for range 100 {
+ if spec := metadata.PartitionSpecByID(4); spec == nil || spec.ID() != 4 {
+ t.Errorf("expected partition spec 4, got %v", spec)
+ }
+ }
+ }()
+ }
+ wg.Wait()
+
+ assert.Same(t, originalIndex, metadata.partitionSpecIndex)
+}
+
+func TestRejectsDuplicatePartitionSpecIDs(t *testing.T) {
+ var raw map[string]json.RawMessage
+ require.NoError(t, json.Unmarshal([]byte(ExampleTableMetadataV2), &raw))
+
+ var specs []json.RawMessage
+ require.NoError(t, json.Unmarshal(raw["partition-specs"], &specs))
+ specs = append(specs, specs[0])
+ encodedSpecs, err := json.Marshal(specs)
+ require.NoError(t, err)
+ raw["partition-specs"] = encodedSpecs
+ data, err := json.Marshal(raw)
+ require.NoError(t, err)
+
+ _, err = ParseMetadataBytes(data)
+ require.ErrorIs(t, err, ErrInvalidMetadata)
+ assert.ErrorContains(t, err, "duplicate partition spec ID 0")
+}
+
+// Why: builder-produced metadata must enforce the same unique partition spec
+// ID invariant as metadata read from JSON.
+// Condition: an in-package builder is given duplicate spec IDs before Build.
+// Assertion: Build rejects the metadata instead of publishing an ambiguous index.
+func TestMetadataBuilderBuildRejectsDuplicatePartitionSpecIDs(t *testing.T) {
+ builder := builderWithoutChanges(2)
+ builder.specs = append(builder.specs, builder.specs[0])
+
+ _, err := builder.Build()
+ require.ErrorIs(t, err, ErrInvalidMetadata)
+ assert.ErrorContains(t, err, "duplicate partition spec ID 0")
+}
+
+// Why: in-package fixtures can mutate an existing spec slice without changing
+// its length or backing array, which cannot be detected by index metadata alone.
+// Condition: a spec is replaced in place after the index is built.
+// Assertion: lookup still finds the replacement and does not return the old ID.
+func TestCommonMetadataPartitionSpecIndexFallsBackAfterElementMutation(t *testing.T) {
+ specs := partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 1, 2)
+ oldID := specs[7].ID()
+ metadata := commonMetadata{
+ Specs: specs,
+ DefaultSpecID: 1,
+ partitionSpecIndex: buildPartitionSpecIndex(specs),
+ }
+
+ specs[7] = iceberg.NewPartitionSpecID(3)
+
+ byID := metadata.PartitionSpecByID(3)
+ require.NotNil(t, byID)
+ assert.Equal(t, 3, byID.ID())
+ assert.Nil(t, metadata.PartitionSpecByID(oldID))
+}
+
+// Why: builders can also be used by package-level fixtures that replace their
+// spec slice without updating derived state.
+// Condition: an indexed builder receives a replacement slice of equal length.
+// Assertion: GetSpecByID resolves the replacement slice and refreshes its index.
+func TestMetadataBuilderPartitionSpecIndexFallsBackAfterSliceReplacement(t *testing.T) {
+ specs := partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 1, 2)
+ builder := MetadataBuilder{
+ specs: specs,
+ partitionSpecIndex: buildPartitionSpecIndex(specs),
+ }
+ originalIndex := builder.partitionSpecIndex
+
+ builder.specs = partitionSpecIndexTestSpecsAtLeast(partitionSpecIndexMinSize+8, 3, 4)
+ byID, err := builder.GetSpecByID(4)
+ require.NoError(t, err)
+ require.NotNil(t, byID)
+ assert.Equal(t, 4, byID.ID())
+ _, err = builder.GetSpecByID(2)
+ assert.ErrorIs(t, err, ErrPartitionSpecNotFound)
+ assert.NotSame(t, originalIndex, builder.partitionSpecIndex)
+ assert.Equal(t, 0, builder.partitionSpecIndex.positions[3])
+ assert.Equal(t, 1, builder.partitionSpecIndex.positions[4])
+ assert.Equal(t, 0, originalIndex.positions[1])
+ assert.Equal(t, 1, originalIndex.positions[2])
+}
+
+// Why: builder updates and clones must keep the spec index aligned without
+// sharing mutable lookup state with a built metadata value or sibling builder.
+// Condition: add a spec, clone the builder, add another spec to the clone, and
+// remove the first added spec from the original.
+// Assertion: each builder resolves the IDs in its own current spec slice.
+func TestMetadataBuilderPartitionSpecIndexFollowsUpdates(t *testing.T) {
+ builder := builderWithoutChanges(2)
+ for id := 1; len(builder.specs) < partitionSpecIndexMinSize; id++ {
+ builder.specs = append(builder.specs, iceberg.NewPartitionSpecID(id))
+ }
+ builder.partitionSpecIndex = buildPartitionSpecIndex(builder.specs)
+ require.Len(t, builder.specs, partitionSpecIndexMinSize)
+ require.Equal(t, 0, builder.partitionSpecIndex.positions[0])
+
+ added := iceberg.NewPartitionSpecID(99, iceberg.PartitionField{
+ SourceIDs: []int{1}, Name: "x", Transform: iceberg.IdentityTransform{},
+ })
+ require.NoError(t, builder.AddPartitionSpec(&added, false))
+ require.Equal(t, partitionSpecIndexMinSize, builder.partitionSpecIndex.positions[partitionSpecIndexMinSize])
+
+ got, err := builder.GetSpecByID(partitionSpecIndexMinSize)
+ require.NoError(t, err)
+ require.NotNil(t, got)
+ assert.Equal(t, partitionSpecIndexMinSize, got.ID())
+
+ clone := builder.clone()
+ cloneAdded := iceberg.NewPartitionSpecID(99, iceberg.PartitionField{
+ SourceIDs: []int{3}, Name: "z", Transform: iceberg.IdentityTransform{},
+ })
+ require.NoError(t, clone.AddPartitionSpec(&cloneAdded, false))
+ assert.NotContains(t, builder.partitionSpecIndex.positions, partitionSpecIndexMinSize+1)
+ assert.Equal(t, partitionSpecIndexMinSize+1, clone.partitionSpecIndex.positions[partitionSpecIndexMinSize+1])
+
+ require.NoError(t, builder.RemovePartitionSpecs([]int{1}))
+ assert.NotContains(t, builder.partitionSpecIndex.positions, 1)
+ _, err = builder.GetSpecByID(1)
+ assert.ErrorIs(t, err, ErrPartitionSpecNotFound)
+ got, err = clone.GetSpecByID(1)
+ require.NoError(t, err)
+ assert.Equal(t, 1, got.ID())
+}
+
+func TestMetadataBuilderPartitionSpecIndexBuildsAtThreshold(t *testing.T) {
+ builder := builderWithoutChanges(2)
+ for id := 1; len(builder.specs) < partitionSpecIndexMinSize-1; id++ {
+ builder.specs = append(builder.specs, iceberg.NewPartitionSpecID(id))
+ }
+ builder.partitionSpecIndex = buildPartitionSpecIndex(builder.specs)
+ require.Nil(t, builder.partitionSpecIndex)
+
+ added := iceberg.NewPartitionSpecID(99, iceberg.PartitionField{
+ SourceIDs: []int{1}, Name: "threshold", Transform: iceberg.IdentityTransform{},
+ })
+ require.NoError(t, builder.AddPartitionSpec(&added, false))
+ require.NotNil(t, builder.partitionSpecIndex.positions)
+ assert.Equal(t, partitionSpecIndexMinSize-1,
+ builder.partitionSpecIndex.positions[partitionSpecIndexMinSize-1])
+}
+
+// Why: removing unknown IDs is a no-op and must not leave the derived index
+// pointing at a newly allocated but unindexed slice.
+// Condition: remove an ID that is not present in the builder.
+// Assertion: the specs slice and its index remain unchanged.
+func TestMetadataBuilderRemoveUnknownPartitionSpecKeepsIndex(t *testing.T) {
+ builder := builderWithoutChanges(2)
Review Comment:
**nit** — Half of TestMetadataBuilderRemoveUnknownPartitionSpecKeepsIndex is a nil-vs-nil assertion
builderWithoutChanges(2) has 1 spec, so originalIndex and builder.partitionSpecIndex are both nil and assert.Same compares nil to nil. The following assert.Same(&builder.specs[0]) is the assertion carrying the test's meaning. Consider dropping the index assertion or moving the fixture above the threshold.
--
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): index partition specs by ID [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1910:
URL: https://github.com/apache/iceberg-go/pull/1910#discussion_r3892709157
##########
schema_test.go:
##########
@@ -2293,3 +2294,48 @@ func TestVisitGeoSchemaWithSchemaVisitorPerPrimitiveType(t *testing.T) {
assert.Equal(t, 1, v.geometryCalls)
assert.Equal(t, 1, v.geographyCalls)
}
+
+func TestSchemaMarshalJSONConcurrentLazyLookups(t *testing.T) {
+ for range 32 {
+ schema := iceberg.NewSchemaWithIdentifiers(17, nil,
Review Comment:
The test always builds with `nil` identifier IDs, so the non-nil `ids` branch in `MarshalJSON` never actually gets raced. I'd add a parallel case with something like `[]int{1}` and assert the encoded JSON carries `"identifier-field-ids":[1]`.
Related: the closing `assert.Nil(t, schema.IdentifierFieldIDs)` passes on both old and new code (the old copy mutated `aliasCopy`, never `*s`), so it doesn't actually guard this fix; `-race` is what catches the regression. I'd either drop it or leave a one-line note that the real invariant is that marshaling mustn't mutate the receiver. wdyt?
##########
schema.go:
##########
@@ -346,8 +346,7 @@ func (s *Schema) MarshalJSON() ([]byte, error) {
type Alias Schema
- aliasCopy := *(*Alias)(s)
- aliasCopy.IdentifierFieldIDs = ids
+ aliasCopy := Alias{ID: s.ID, IdentifierFieldIDs: ids}
Review Comment:
This is the right fix. The old `*(*Alias)(s)` copied the five `atomic.Pointer` cache fields by value, which is the copy-after-use case `go vet` flags, and the selective literal keeps the JSON byte-identical (only `schema-id` and `identifier-field-ids` are exported off `Alias`, and the nil-to-`[]int{}` coercion is preserved).
The one thing I'd guard: the named literal is point-in-time. If a new exported JSON-tagged field gets added to `Schema` later, marshaling silently zeroes it, with no compiler error and no failing test. I'd drop a one-line comment above it noting only `ID` and `IdentifierFieldIDs` are exported here, so anyone adding a field knows to list it. Non-blocking.
--
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