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