zeroshade commented on code in PR #1988:
URL: https://github.com/apache/iceberg-go/pull/1988#discussion_r3937952489
##########
table/metadata.go:
##########
@@ -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 {
+ 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 {
Review 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.
##########
table/metadata_preflight_test.go:
##########
@@ -85,6 +85,66 @@ func TestParseMetadataBytesAssignsMissingPartitionFieldIDs(t
*testing.T) {
}
}
+func TestParseMetadataBytesNormalizesStaleLastPartitionID(t *testing.T) {
+ data := strings.Replace(ExampleTableMetadataV2,
Review 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.
--
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]
