fix(manifest): preserve 64-bit list metadata ids#1334
Open
fallintoplace wants to merge 2 commits into
Open
Conversation
tanmayrauth
reviewed
Jun 27, 2026
tanmayrauth
left a comment
tanmayrauth
left a comment
Contributor
There was a problem hiding this comment.
LGTM, correct 64-bit fix across all three writer versions. One optional test idea inline.
| } | ||
|
|
||
| return m, m.init(map[string][]byte{ | ||
| "format-version": []byte(strconv.Itoa(m.version)), |
Contributor
There was a problem hiding this comment.
Nice that you caught first-row-id here too, not just the fields shared with v1/v2, FormatInt is the right call for all of these since they're all 64-bit per the spec.
| m.EqualValues(5022, *writer.NextRowID()) | ||
| } | ||
|
|
||
| func (m *ManifestTestSuite) TestManifestListWriterMetadataPreservesInt64Values() { |
Contributor
There was a problem hiding this comment.
1<<40 already proves the >2^32 case well. Might be worth also asserting a value up near math.MaxInt64 (a realistic snapshot id like 9223372036854775807), since that's the boundary a future regression to a
32-bit path would corrupt first.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
format manifest-list header snapshot ids, sequence numbers, parent snapshot ids, and first-row ids directly from int64
remove the intermediate
intconversion in V1/V2/V3 manifest list writersadd regression coverage that reads the OCF header metadata back for large 64-bit values
Why
The manifest-list writers were serializing several 64-bit metadata fields by converting them to
intand then callingstrconv.Itoa. That is conceptually wrong for Iceberg's 64-bit ids and unsafe on 32-bit builds.Using
strconv.FormatInt(..., 10)keeps the full value intact for snapshot ids, sequence numbers, parent snapshot ids, and v3 first-row ids.Testing
go test . -run 'TestManifests/TestManifestListWriterMetadataPreservesInt64Values$' -count=1
go test . -count=1
go test ./table -count=1
go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.11.4 run --timeout=10m . ./table/...
git diff --check