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