This is an automated email from the ASF dual-hosted git repository.
laskoviymishka pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-go.git
The following commit(s) were added to refs/heads/main by this push:
new ad3a17ea fix(manifest): reject row ID allocation overflow (#1627)
ad3a17ea is described below
commit ad3a17eac5fc08f1242156e7c5af238d7fbe9bb8
Author: Minh Vu <[email protected]>
AuthorDate: Wed Aug 5 19:01:49 2026 +0200
fix(manifest): reject row ID allocation overflow (#1627)
* fix(manifest): reject row ID allocation overflow
* fix(manifest): update row ID cursor after encoding
* fix(manifest): handle unknown legacy row counts
* fix(manifest): keep row ID cursor transactional
---
manifest.go | 56 ++++++++++++++++++++++++++--
manifest_test.go | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 164 insertions(+), 3 deletions(-)
diff --git a/manifest.go b/manifest.go
index d41c008b..f2912eb8 100644
--- a/manifest.go
+++ b/manifest.go
@@ -1587,6 +1587,10 @@ func NewManifestListWriterV2(out io.Writer, snapshotID,
sequenceNumber int64, pa
}
func NewManifestListWriterV3(out io.Writer, snapshotId, sequenceNumber,
firstRowID int64, parentSnapshot *int64) (*ManifestListWriter, error) {
+ if firstRowID < 0 {
+ return nil, fmt.Errorf("%w: first row ID must be non-negative:
%d", ErrInvalidArgument, firstRowID)
+ }
+
m := &ManifestListWriter{
version: 3,
out: out,
@@ -1608,6 +1612,28 @@ func NewManifestListWriterV3(out io.Writer, snapshotId,
sequenceNumber, firstRow
})
}
+func advanceRowID(firstRowID, existingRows, addedRows int64) (int64, error) {
+ if firstRowID < 0 {
+ return 0, fmt.Errorf("%w: first row ID must be non-negative:
%d", ErrInvalidArgument, firstRowID)
+ }
+ if existingRows == -1 {
+ existingRows = 0
+ }
+ if addedRows == -1 {
+ addedRows = 0
+ }
+ if existingRows < 0 || addedRows < 0 {
+ return 0, fmt.Errorf("%w: row counts must be non-negative:
existing=%d added=%d",
+ ErrInvalidArgument, existingRows, addedRows)
+ }
+ if existingRows > math.MaxInt64-firstRowID || addedRows >
math.MaxInt64-firstRowID-existingRows {
+ return 0, fmt.Errorf("%w: assigning %d existing and %d added
rows from first row ID %d overflows int64",
+ ErrInvalidArgument, existingRows, addedRows, firstRowID)
+ }
+
+ return firstRowID + existingRows + addedRows, nil
+}
+
func (m *ManifestListWriter) init(meta map[string][]byte) error {
fileSchema, err := internal.NewManifestFileSchema(m.version)
if err != nil {
@@ -1639,10 +1665,18 @@ func (m *ManifestListWriter) NextRowID() *int64 {
return m.nextRowID
}
-func (m *ManifestListWriter) AddManifests(files []ManifestFile) error {
+func (m *ManifestListWriter) AddManifests(files []ManifestFile) (err error) {
if len(files) == 0 {
return nil
}
+ if m.version == 3 && m.nextRowID != nil {
+ batchStartRowID := *m.nextRowID
+ defer func() {
+ if err != nil {
+ *m.nextRowID = batchStartRowID
+ }
+ }()
+ }
switch m.version {
case 1:
@@ -1663,6 +1697,8 @@ func (m *ManifestListWriter) AddManifests(files
[]ManifestFile) error {
case 2, 3:
for _, file := range files {
+ var assignedNextRowID *int64
+
// Per the Iceberg spec a v2 manifest list may
reference v1 manifest
// files (and a v3 list may reference v1 or v2
manifests) so that a
// table can be upgraded without rewriting historical
manifests. The
@@ -1685,8 +1721,12 @@ func (m *ManifestListWriter) AddManifests(files
[]ManifestFile) error {
if wrapped.FirstRowIDValue == nil {
if m.nextRowID != nil {
firstRowID :=
*m.nextRowID
+ nextRowID, err :=
advanceRowID(firstRowID, wrapped.ExistingRowsCount, wrapped.AddedRowsCount)
+ if err != nil {
+ return
fmt.Errorf("manifest %q: %w", wrapped.Path, err)
+ }
wrapped.FirstRowIDValue
= &firstRowID
- *m.nextRowID +=
wrapped.ExistingRowsCount + wrapped.AddedRowsCount
+ assignedNextRowID =
&nextRowID
}
}
}
@@ -1715,6 +1755,9 @@ func (m *ManifestListWriter) AddManifests(files
[]ManifestFile) error {
if err := m.writer.Encode(wrapped); err != nil {
return err
}
+ if assignedNextRowID != nil {
+ *m.nextRowID = *assignedNextRowID
+ }
}
default:
return fmt.Errorf("unsupported manifest version: %d", m.version)
@@ -1798,6 +1841,10 @@ func WriteManifestV3(
snapshotID int64,
entries []ManifestEntry,
) (mf ManifestFile, nextFirstRowID int64, err error) {
+ if firstRowID < 0 {
+ return nil, 0, fmt.Errorf("%w: first row ID must be
non-negative: %d", ErrInvalidArgument, firstRowID)
+ }
+
cnt := &internal.CountingWriter{W: out}
w, err := NewManifestWriter(3, cnt, spec, schema, snapshotID)
@@ -1824,7 +1871,10 @@ func WriteManifestV3(
raw := mf.(*manifestFile)
v := firstRowID
raw.FirstRowIDValue = &v
- nextFirstRowID = firstRowID + raw.AddedRowsCount + raw.ExistingRowsCount
+ nextFirstRowID, err = advanceRowID(firstRowID, raw.ExistingRowsCount,
raw.AddedRowsCount)
+ if err != nil {
+ return nil, 0, fmt.Errorf("manifest %q: %w", filename, err)
+ }
return raw, nextFirstRowID, nil
}
diff --git a/manifest_test.go b/manifest_test.go
index 002b5fd3..7cc73030 100644
--- a/manifest_test.go
+++ b/manifest_test.go
@@ -1280,6 +1280,22 @@ func (m *ManifestTestSuite) TestWriteManifestV3() {
m.EqualValues(520, nextID) // 500 + 10 + 10
})
+ m.Run("rejects negative first row ID", func() {
+ var buf bytes.Buffer
+ _, _, err := WriteManifestV3("/manifest.avro", &buf, -1,
partitionSpec, testSchema, entrySnapshotID, entries)
+ m.Require().ErrorIs(err, ErrInvalidArgument)
+ m.Require().ErrorContains(err, "first row ID must be
non-negative")
+ })
+
+ m.Run("rejects next row ID overflow", func() {
+ var buf bytes.Buffer
+ _, _, err := WriteManifestV3(
+ "/manifest.avro", &buf, math.MaxInt64-count,
partitionSpec, testSchema, entrySnapshotID, entries,
+ )
+ m.Require().ErrorIs(err, ErrInvalidArgument)
+ m.Require().ErrorContains(err, "overflows int64")
+ })
+
m.Run("read inheritance from manifest", func() {
var buf bytes.Buffer
firstRowID := int64(500)
@@ -2475,6 +2491,82 @@ func (m *ManifestTestSuite)
TestV3ManifestListWriterRowIDTracking() {
m.Require().NoError(err)
}
+func (m *ManifestTestSuite)
TestV3ManifestListWriterRejectsInvalidRowIDRanges() {
+ m.Run("negative first row ID", func() {
+ var buf bytes.Buffer
+ writer, err := NewManifestListWriterV3(&buf, snapshotID, 1, -1,
nil)
+ m.Nil(writer)
+ m.Require().ErrorIs(err, ErrInvalidArgument)
+ m.Require().ErrorContains(err, "first row ID must be
non-negative")
+ })
+
+ m.Run("negative row count", func() {
+ var buf bytes.Buffer
+ writer, err := NewManifestListWriterV3(&buf, snapshotID, 1, 0,
nil)
+ m.Require().NoError(err)
+ manifest := NewManifestFile(3, "negative.avro", 100, 1,
snapshotID).AddedRows(-2).Build()
+ err = writer.AddManifests([]ManifestFile{manifest})
+ m.Require().ErrorIs(err, ErrInvalidArgument)
+ m.Require().ErrorContains(err, "row counts must be
non-negative")
+ })
+
+ m.Run("negative existing row count", func() {
+ var buf bytes.Buffer
+ writer, err := NewManifestListWriterV3(&buf, snapshotID, 1, 0,
nil)
+ m.Require().NoError(err)
+ manifest := NewManifestFile(3, "negative-existing.avro", 100,
1, snapshotID).ExistingRows(-2).Build()
+ err = writer.AddManifests([]ManifestFile{manifest})
+ m.Require().ErrorIs(err, ErrInvalidArgument)
+ m.Require().ErrorContains(err, "row counts must be
non-negative")
+ })
+
+ m.Run("overflow", func() {
+ var buf bytes.Buffer
+ writer, err := NewManifestListWriterV3(&buf, snapshotID, 1,
math.MaxInt64, nil)
+ m.Require().NoError(err)
+ manifest := NewManifestFile(3, "overflow.avro", 100, 1,
snapshotID).AddedRows(1).Build()
+ err = writer.AddManifests([]ManifestFile{manifest})
+ m.Require().ErrorIs(err, ErrInvalidArgument)
+ m.Require().ErrorContains(err, "overflows int64")
+ m.EqualValues(math.MaxInt64, *writer.NextRowID())
+ })
+
+ m.Run("existing row count overflow", func() {
+ var buf bytes.Buffer
+ writer, err := NewManifestListWriterV3(&buf, snapshotID, 1,
math.MaxInt64, nil)
+ m.Require().NoError(err)
+ manifest := NewManifestFile(3, "overflow-existing.avro", 100,
1, snapshotID).ExistingRows(1).Build()
+ err = writer.AddManifests([]ManifestFile{manifest})
+ m.Require().ErrorIs(err, ErrInvalidArgument)
+ m.Require().ErrorContains(err, "overflows int64")
+ m.EqualValues(math.MaxInt64, *writer.NextRowID())
+ })
+
+ m.Run("later validation failure leaves cursor unchanged", func() {
+ var buf bytes.Buffer
+ writer, err := NewManifestListWriterV3(&buf, snapshotID, 1, 10,
nil)
+ m.Require().NoError(err)
+ manifest := NewManifestFile(3, "other-snapshot.avro", 100, 1,
snapshotID+1).AddedRows(5).Build()
+ err = writer.AddManifests([]ManifestFile{manifest})
+ m.Require().ErrorContains(err, "unassigned sequence number")
+ m.EqualValues(10, *writer.NextRowID())
+ })
+
+ m.Run("batch failure leaves cursor unchanged", func() {
+ var buf bytes.Buffer
+ startRowID := int64(math.MaxInt64 - 5)
+ writer, err := NewManifestListWriterV3(&buf, snapshotID, 1,
startRowID, nil)
+ m.Require().NoError(err)
+ manifests := []ManifestFile{
+ NewManifestFile(3, "valid.avro", 100, 1,
snapshotID).AddedRows(5).Build(),
+ NewManifestFile(3, "overflow.avro", 100, 1,
snapshotID).AddedRows(1).Build(),
+ }
+ err = writer.AddManifests(manifests)
+ m.Require().ErrorContains(err, "overflows int64")
+ m.EqualValues(startRowID, *writer.NextRowID())
+ })
+}
+
func (m *ManifestTestSuite) TestV3ManifestListWriterAssignedRowIDDelta() {
// Assigned row-id delta = sum of (existing+added) for all data
manifests in list.
var buf bytes.Buffer
@@ -3135,6 +3227,25 @@ func (m *ManifestTestSuite)
TestV3ManifestListAcceptsV1AndV2Manifests() {
m.Nil(v2Entry.FirstRowID(), "delete manifests must not be assigned
first_row_id")
}
+func (m *ManifestTestSuite)
TestV3ManifestListAssignsZeroForV1ManifestWithUnknownRowCounts() {
+ legacy := *(manifestFileRecordsV1[0].(*manifestFile))
+ legacy.AddedRowsCount = -1
+ legacy.ExistingRowsCount = -1
+
+ var v1Buf bytes.Buffer
+ m.Require().NoError(WriteManifestList(1, &v1Buf, snapshotID, nil, nil,
0, []ManifestFile{&legacy}))
+ manifests, err := ReadManifestList(&v1Buf)
+ m.Require().NoError(err)
+ m.Require().Len(manifests, 1)
+
+ var v3Buf bytes.Buffer
+ writer, err := NewManifestListWriterV3(&v3Buf, snapshotID, 1, 1000, nil)
+ m.Require().NoError(err)
+ m.Require().NoError(writer.AddManifests(manifests))
+ m.EqualValues(1000, *writer.NextRowID())
+ m.Require().NoError(writer.Close())
+}
+
// TestV2ManifestListRejectsV3Manifests confirms that a v2 manifest list still
// refuses to reference v3 manifest files, since the v2 entry schema has no
// place to record first_row_id and accepting them would silently drop data.
