Skip to content

fix(table): normalize stale last partition ID #1988

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mattfaltyn wants to merge 3 commits into
base: main
Choose a base branch
from
+107 −16
Open

fix(table): normalize stale last partition ID #1988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
abc35da
fix(table): normalize stale last partition ID
mattfaltyn Sep 3, 2026
ebf0a16
fix(table): preserve unassigned partition counters
mattfaltyn Sep 3, 2026
e01dbb2
test(table): cover partition counter normalization
mattfaltyn Sep 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 24 additions & 16 deletions table/metadata.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2016,11 +2016,15 @@ func assignMissingPartitionFieldIDsFromMetadata(b []byte, metadata map[string]js
}

lastAssignedID := iceberg.PartitionDataIDStart - 1
lastPartitionID := 0
normalizedLastPartitionID := 0
lastPartitionIDSet := false
if rawLastPartitionID, ok := metadata["last-partition-id"]; ok {
var lastPartitionID *int
if err := json.Unmarshal(rawLastPartitionID, &lastPartitionID); err == nil && lastPartitionID != nil {
lastAssignedID = max(lastAssignedID, *lastPartitionID)
var parsedLastPartitionID *int
if err := json.Unmarshal(rawLastPartitionID, &parsedLastPartitionID); err == nil && parsedLastPartitionID != nil {
lastPartitionID = *parsedLastPartitionID
normalizedLastPartitionID = lastPartitionID
lastAssignedID = max(lastAssignedID, lastPartitionID)
lastPartitionIDSet = true
}
}
Expand All @@ -2038,11 +2042,12 @@ func assignMissingPartitionFieldIDsFromMetadata(b []byte, metadata map[string]js
var fieldID *int
if err := json.Unmarshal(rawFieldID, &fieldID); err == nil && fieldID != nil {
lastAssignedID = max(lastAssignedID, *fieldID)
normalizedLastPartitionID = max(normalizedLastPartitionID, *fieldID)
}
}
}

if len(missingFields) == 0 {
if len(missingFields) == 0 && (!lastPartitionIDSet || lastPartitionID == normalizedLastPartitionID) {
return b, nil
}

Expand All @@ -2053,24 +2058,27 @@ func assignMissingPartitionFieldIDsFromMetadata(b []byte, metadata map[string]js
return nil, err
}
field["field-id"] = rawFieldID
normalizedLastPartitionID = max(normalizedLastPartitionID, lastAssignedID)
}

if usesSpecList {
rawSpecs, err := json.Marshal(specs)
if err != nil {
return nil, err
}
metadata["partition-specs"] = rawSpecs
} else {
rawFields, err := json.Marshal(specs[0].Fields)
if err != nil {
return nil, err
if len(missingFields) > 0 {
if usesSpecList {

@zeroshade zeroshade Sep 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor — Missing-field re-marshal path silently drops unknown partition-spec keys

The new 'if len(missingFields) > 0' guard correctly keeps the stale-counter-only path from round-tripping specs through rawPartitionSpec (which carries only spec-id and fields). But the missing-field path it now wraps still does, so any other key on a partition-spec object is dropped on rewrite. This is pre-existing rather than introduced -- flagging it because the PR restructured exactly this block and the asymmetry between the two paths is now visible in the diff.

@mattfaltyn mattfaltyn Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for flagging this. I confirmed the unknown partition-spec key loss predates this change and left it unchanged to keep #1987 focused. The stale-counter-only path introduced here continues to avoid partition-spec re-marshalling.

@zeroshade zeroshade Sep 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit — Unknown-spec-key fidelity guard is itself unpinned by tests

The 'if len(missingFields) > 0' guard prevents the stale-counter-only path from round-tripping specs through rawPartitionSpec, which would drop spec-level keys the struct cannot represent. Replacing the condition with 'true' leaves the entire table package green, so the preservation behaviour has no regression test. TestAssignMissingPartitionFieldIDsNormalizesLegacyStaleCounter passes through this path but only asserts last-partition-id, never the spec bytes. Non-blocking; the guard is an improvement over pre-PR behaviour either way.

rawSpecs, err := json.Marshal(specs)
if err != nil {
return nil, err
}
metadata["partition-specs"] = rawSpecs
} else {
rawFields, err := json.Marshal(specs[0].Fields)
if err != nil {
return nil, err
}
metadata["partition-spec"] = rawFields
}
metadata["partition-spec"] = rawFields
}

if lastPartitionIDSet {
rawLastPartitionID, err := json.Marshal(lastAssignedID)
rawLastPartitionID, err := json.Marshal(normalizedLastPartitionID)
if err != nil {

@zeroshade zeroshade Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor — Floor/persist separation at the write site survives mutation of the whole table package

metadata.go:2081 persists normalizedLastPartitionID rather than lastAssignedID — the headline guarantee of commit ebf0a16. Substituting lastAssignedID there leaves every test in ./table passing. The two values only diverge at this line when no field IDs are missing and the persisted counter plus all partition field IDs are below 999, which is reachable with legacy v1 metadata whose partition field IDs predate the 1000 floor. All three added test cases hit the early return at metadata.go:2050 and never reach line 2081, so the separation is asserted in only one of its two directions. Behavior at head is correct; this is a coverage gap, not a live bug. Adding one case with a sub-1000 partition field ID and a lower counter (expect the greatest field ID, not 999) would pin it.

return nil, err
}
Expand Down
83 changes: 83 additions & 0 deletions table/metadata_preflight_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"strings"
"testing"

"github.com/apache/iceberg-go"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
Expand Down Expand Up @@ -85,6 +86,88 @@ func TestParseMetadataBytesAssignsMissingPartitionFieldIDs(t *testing.T) {
}
}

func TestParseMetadataBytesNormalizesStaleLastPartitionID(t *testing.T) {
data := strings.Replace(ExampleTableMetadataV2,

@zeroshade zeroshade Sep 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor — New early-return condition adds three branches, only one is tested

The condition at metadata.go:2047 introduces distinct branches: counter below max field ID (tested), counter above max field ID (must stay untouched), counter below the 999 floor with no field IDs, and stale counter combined with a missing field-id. Only the first has a test. I verified the untested ones behave as follows -- add cases for them so the condition is pinned: counter-above-max stays untouched, and stale-counter-plus-missing-field-id assigns correctly.

@mattfaltyn mattfaltyn Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for identifying the missing branches. Added coverage in ebf0a16 for sub-999 counters with no assigned fields, counters above the greatest assigned field ID, and stale counters combined with a missing field ID. The first two cases also verify the unchanged-byte fast path.

@zeroshade zeroshade Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit — End-to-end allocation half of the issue's suggested regression coverage is not asserted

Issue #1987 asks for coverage that asserts both that the parsed counter becomes 1000 and that the next distinct partition field receives 1001. TestParseMetadataBytesNormalizesStaleLastPartitionID asserts only the former. I verified the latter holds today, so this is purely about locking in the user-visible symptom (the cross-spec ID collision) rather than only its parse-level cause.

`"last-partition-id": 1000`, `"last-partition-id": 999`, 1)

parsed, err := ParseMetadataBytes([]byte(data))
require.NoError(t, err)
require.NotNil(t, parsed.LastPartitionSpecID())
assert.Equal(t, 1000, *parsed.LastPartitionSpecID())

update := NewUpdateSpec(New(nil, parsed, "", nil, nil).NewTransaction(), false).
AddField("x", iceberg.BucketTransform{NumBuckets: 16}, "x_bucket")
_, _, err = update.BuildUpdates()
require.NoError(t, err)
updated, err := update.Apply()
require.NoError(t, err)
require.Equal(t, 2, updated.NumFields())
assert.Equal(t, 1001, updated.Field(1).FieldID)
}

func TestAssignMissingPartitionFieldIDsPreservesConsistentMetadata(t *testing.T) {
for _, tt := range []struct {
name string
input string
}{
{
name: "counter below assignment floor with no fields",
input: `{"last-updated-ms":0,"last-partition-id":0,"partition-specs":[{"spec-id":0,"fields":[]}]}`,
},
{
name: "counter above greatest field ID",
input: `{"last-updated-ms":0,"last-partition-id":1001,"partition-specs":[{"spec-id":0,"fields":[{"field-id":1000}]}]}`,
},
} {
t.Run(tt.name, func(t *testing.T) {
normalized, err := assignMissingPartitionFieldIDs([]byte(tt.input))
require.NoError(t, err)
assert.Equal(t, tt.input, string(normalized))
})
}
}

func TestAssignMissingPartitionFieldIDsNormalizesStaleCounter(t *testing.T) {
input := []byte(`{
"last-updated-ms": 0,
"last-partition-id": 999,
"partition-specs": [{
"spec-id": 0,
"fields": [{"field-id": 1000}, {}]
}]
}`)

normalized, err := assignMissingPartitionFieldIDs(input)
require.NoError(t, err)

var parsed struct {
LastPartitionID int `json:"last-partition-id"`
Specs []struct {
Fields []struct {
FieldID int `json:"field-id"`
} `json:"fields"`
} `json:"partition-specs"`
}
require.NoError(t, json.Unmarshal(normalized, &parsed))
assert.Equal(t, 1001, parsed.LastPartitionID)
require.Len(t, parsed.Specs, 1)
require.Len(t, parsed.Specs[0].Fields, 2)
assert.Equal(t, 1001, parsed.Specs[0].Fields[1].FieldID)
}

func TestAssignMissingPartitionFieldIDsNormalizesLegacyStaleCounter(t *testing.T) {
input := []byte(`{"last-updated-ms":0,"last-partition-id":8,"partition-specs":[{"spec-id":0,"fields":[{"field-id":9}]}]}`)

normalized, err := assignMissingPartitionFieldIDs(input)
require.NoError(t, err)

var parsed struct {
LastPartitionID int `json:"last-partition-id"`
}
require.NoError(t, json.Unmarshal(normalized, &parsed))
assert.Equal(t, 9, parsed.LastPartitionID)
}

func TestParseMetadataBytesRejectsCaseFoldedFormatVersionCollision(t *testing.T) {
data := strings.Replace(
ExampleTableMetadataV2,
Expand Down
Loading