zeroshade commented on code in PR #1988:
URL: https://github.com/apache/iceberg-go/pull/1988#discussion_r3926375136
##########
table/metadata.go:
##########
@@ -2016,11 +2016,13 @@ func assignMissingPartitionFieldIDsFromMetadata(b
[]byte, metadata map[string]js
}
lastAssignedID := iceberg.PartitionDataIDStart - 1
+ lastPartitionID := lastAssignedID
Review 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.
<details><summary>Evidence</summary>
```text
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.
```
</details>
##########
table/metadata.go:
##########
@@ -2055,18 +2057,20 @@ func assignMissingPartitionFieldIDsFromMetadata(b
[]byte, metadata map[string]js
field["field-id"] = rawFieldID
}
- 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 {
Review 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.
##########
table/metadata_preflight_test.go:
##########
@@ -85,6 +85,16 @@ func TestParseMetadataBytesAssignsMissingPartitionFieldIDs(t
*testing.T) {
}
}
+func TestParseMetadataBytesNormalizesStaleLastPartitionID(t *testing.T) {
+ data := strings.Replace(ExampleTableMetadataV2,
Review 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.
--
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]
