fix(table): normalize stale last partition ID - #1988
Conversation
Signed-off-by: Matt Faltyn <faltyn.matthew@gmail.com>
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
The stale-counter normalization correctly fixes the #1987 ID-collision and preserves the byte-identical fast path, but it also rewrites sub-999 counters on tables with no assigned partition field IDs, which desyncs the client's AssertLastAssignedPartitionID from a catalog that persisted the original value.
Re-review verification: 0 of 1 prior findings confirmed fixed at abc35da (each verified by mutating the fix and observing the suite go red, not by taking the claim on trust).
Smaller observations
table/update_spec_test.go:1061— PR description implies UpdateSpec coverage that the diff does not add: Bullet three claims the PR will 'cover the all-field-IDs-present case that previously allowed UpdateSpec to reuse a historical ID', and the Testing section lists TestUpdateSpecReuseHistoricalFieldID. That test is pre-existing (added by #1641) and is not touched by this diff -- it is only re-run. The diff adds exactly one test. Reword so the description matches what is actually added.
Verification performed
go build ./... (PASS); go test ./table/... (PASS: table, compaction, dv, internal, substrait); go test ./cmd/... (PASS); go test ./catalog/... (PASS: catalog, glue, hadoop, hive, internal, rest, planfake, sql); go test ./table/ -run '^TestP[0-9]+' with throwaway probes pr1988_probe_test.go + pr1988_probe2_test.go (13 probes, all ran; P13 discharged its own hypothesis); regression check -- restored table/metadata.go from HEAD~1 and ran the new test: FAIL 'expected: 1000, actual: 999', confirming the added test is non-vacuous. Probe files deleted and worktree confirmed clean via git status --porcelain.
This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg Go maintainer. The findings below are observations, not blockers; an Apache Iceberg Go maintainer — a real person — will take the next look at the PR. If you think a finding is mis-applied, please reply on the PR and a maintainer will weigh in.
More on how Apache Iceberg Go handles maintainer review: CONTRIBUTING.md.
| } | ||
|
|
||
| lastAssignedID := iceberg.PartitionDataIDStart - 1 | ||
| lastPartitionID := lastAssignedID |
There was a problem hiding this comment.
major — 999 assignment floor is conflated with the persisted counter, rewriting collision-free sub-999 values
lastAssignedID is seeded to iceberg.PartitionDataIDStart-1 (999) at line 2018 and then max'd with the persisted counter and all field IDs. The same variable is written back to last-partition-id at line 2077. For metadata with NO assigned partition field IDs, there is no collision to repair, yet any persisted counter below 999 (0, 5, 998) is rewritten to 999 and the unchanged-byte fast path is lost. Because update_spec.go:190-197 derives AssertLastAssignedPartitionID from this normalized value and rest.go:1626-1636 sends it to the catalog, a spec-changing commit against a REST catalog that persisted 0 now asserts 999 and is rejected -- before this PR the client sent 0 and matched. Fix: compute the persist target as max(persistedCounter, maxAssignedFieldID) as a variable separate from the assignment floor, so tables with no assigned field IDs are left untouched. If the 999 floor is deliberate (the linked issue does request 'the greatest of 999 and every explicit partition field ID'), add explicit test coverage for sub-999 counters and state the intentional mutation in the PR description, since the description currently claims only that already-consistent metadata is preserved.
Evidence
Probe P8 (sweep, unpartitioned v2, partition-specs [{spec-id:0,fields:[]}]): 'persisted=0 -> parsed=999 rewritten=true', 'persisted=5 -> parsed=999 rewritten=true', 'persisted=998 -> parsed=999 rewritten=true', 'persisted=999 -> parsed=999 rewritten=false'. Probe P10: 'client asserts 999 against catalog holding 0 -> requirement failed: last assigned partition id has changed: expected 999, found 0'. Probe P11: 're-serialized last-partition-id = 999 (was 0 on disk)'. Shape present in repo fixtures: cmd/iceberg/snapshots_test.go:45, cmd/iceberg/branch_tag_test.go:49, cmd/iceberg/partition_stats_test.go:44.
There was a problem hiding this comment.
Thanks for catching this. Fixed in ebf0a16 by separating the persisted counter target from the 999 allocation cursor. Empty specs with sub-999 counters now retain the original bytes and catalog-visible value, while stale counters with assigned fields still normalize correctly.
| } | ||
|
|
||
| func TestParseMetadataBytesNormalizesStaleLastPartitionID(t *testing.T) { | ||
| data := strings.Replace(ExampleTableMetadataV2, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| if err != nil { | ||
| return nil, err | ||
| if len(missingFields) > 0 { | ||
| if usesSpecList { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Signed-off-by: Matt Faltyn <faltyn.matthew@gmail.com>
|
Thanks for the thorough review and clear reproduction. The counter-floor regression and requested boundary coverage are addressed in ebf0a16. I also updated the PR description to distinguish the newly added regression tests from the pre-existing historical-ID test. All local unit, race, assert, and lint checks pass. |
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
The prior major is genuinely fixed — the 999 allocation floor is now separate from the persisted counter and sub-999 metadata round-trips byte-identically — leaving only an unpinned-by-tests write site at metadata.go:2081.
Re-review verification: 2 of 3 prior findings confirmed fixed at ebf0a16 (each verified by mutating the fix and observing the suite go red, not by taking the claim on trust).
Verification performed
go build ./table/... ./catalog/... (OK); go vet ./table/ (OK); go test ./table/ -run 'Partition|Preflight|Metadata|Spec' -count=1 (ok); go test ./table/ -count=1 -race (ok, 19.7s); go test ./catalog/sql/ -count=1 (ok); mutation runs M1 (early-return guard reverted -> TestParseMetadataBytesNormalizesStaleLastPartitionID FAIL, correctly pinned) and M2 (persist-site variable reverted -> full table package still ok, gap); throwaway probes table/pr1988_probe_test.go, pr1988_div_test.go, pr1988_repro_test.go all deleted; `git status --porcelain` empty.
This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg Go maintainer. The maintainer approving this PR has read the findings and signed off. If something feels off, please reply on the PR and a maintainer will follow up.
More on how Apache Iceberg Go handles maintainer review: CONTRIBUTING.md.
| if lastPartitionIDSet { | ||
| rawLastPartitionID, err := json.Marshal(lastAssignedID) | ||
| rawLastPartitionID, err := json.Marshal(normalizedLastPartitionID) | ||
| if err != nil { |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| func TestParseMetadataBytesNormalizesStaleLastPartitionID(t *testing.T) { | ||
| data := strings.Replace(ExampleTableMetadataV2, |
There was a problem hiding this comment.
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.
Signed-off-by: Matt Faltyn <faltyn.matthew@gmail.com>
Thanks for the precise mutation-based recommendation. I added TestAssignMissingPartitionFieldIDsNormalizesLegacyStaleCounter and confirmed it fails with actual 999 when the write site uses the allocation cursor.
Great call. I extended TestParseMetadataBytesNormalizesStaleLastPartitionID with a bucket16 assertion for field ID 1001. Both updates are in e01dbb2. The full test suite and golangci-lint pass. Thank you for the thorough review! |
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Test-only delta closes the last open finding: mutating the floor/persist separation at metadata.go:2081 now turns TestAssignMissingPartitionFieldIDsNormalizesLegacyStaleCounter RED (expected 9, actual 999), and the end-to-end AddField assertion demonstrates the 1000-collision.
Re-review verification: 3 of 4 prior findings confirmed fixed at e01dbb2 (each verified by mutating the fix and observing the suite go red, not by taking the claim on trust).
Verification performed
go build ./... (clean); go vet ./table/... (clean); go test ./table/... -run 'Partition|LastPartitionID|StaleCounter|StaleLastPartition' -timeout=180s -count=1 (all ok); go test ./table/ -race -timeout=600s -count=1 (ok, 20.234s); 7 in-place mutations of table/metadata.go (write site, early-return guard, field-loop tracker, post-assignment bump, re-marshal guard, builder prev floor, BindToSchema arg) each restored via git checkout; throwaway probe table/pr1988_probe_test.go written, run, and deleted. Final 'git status --porcelain' empty.
This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg Go maintainer. The maintainer approving this PR has read the findings and signed off. If something feels off, please reply on the PR and a maintainer will follow up.
More on how Apache Iceberg Go handles maintainer review: CONTRIBUTING.md.
| if err != nil { | ||
| return nil, err | ||
| if len(missingFields) > 0 { | ||
| if usesSpecList { |
There was a problem hiding this comment.
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.
2 participants
Closes #1987.
Summary
last-partition-idto the greatest explicitly assigned partition field ID during metadata preflightWhy
Format v2+ partition field IDs are unique across every spec in a table. When persisted metadata contained field ID
1000butlast-partition-idwas999, parsing preserved the stale counter. A later distinct partition transform could then also receive ID1000.The preflight now advances the persisted counter to the greatest explicitly assigned field ID. Its separate allocation cursor still starts at
999when filling omitted field IDs, without rewriting lower counters on specs that have no assigned fields. Valid metadata continues to return the original bytes without re-marshalling.Testing
TestParseMetadataBytesNormalizesStaleLastPartitionIDTestAssignMissingPartitionFieldIDsPreservesConsistentMetadataTestAssignMissingPartitionFieldIDsNormalizesStaleCounterTestUpdateSpecReuseHistoricalFieldIDmake testmake test-racemake test-assert/Users/mattfaltyn/go/bin/golangci-lint run --timeout=10musing v2.12.2git diff --checkNo documentation changes are needed because this restores the existing metadata contract without changing the public API.