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