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: [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]
