laskoviymishka commented on code in PR #1054:
URL: https://github.com/apache/iceberg-go/pull/1054#discussion_r3215442814
##########
table/metadata.go:
##########
@@ -505,6 +505,11 @@ func (b *MetadataBuilder)
validateAndUpdateRowLineage(snapshot *Snapshot) error
}
newNextRowID := nextRowID + *snapshot.AddedRows
+ if newNextRowID <= nextRowID {
+ return fmt.Errorf("%w: next-row-id must advance from previous
%d to new %d (added-rows %d)",
+ ErrInvalidRowLineage, nextRowID, newNextRowID,
*snapshot.AddedRows)
+ }
Review Comment:
The v3 spec permits added-rows=0 explicitly — first-row-id is required "even
if a commit does not assign any ID space" — and Java's `TableMetadata` applies
`addedRows` unconditionally with no `>0` guard. More concretely,
`manifest.go:1518` in this repo only advances `nextRowID` for
`ManifestContentData` manifests, so any merge-on-read delete via
`performMergeOnReadDeletion` will land here with addedRows=0 and fail. The
existing checks (ValidateRowLineage rejecting added-rows<0 plus
first-row-id≥next-row-id) already cover what the spec requires. Suggest
dropping this block, or at minimum changing `<=` to `<` so the check only fires
on genuine regression.
##########
table/metadata.go:
##########
@@ -505,6 +505,11 @@ func (b *MetadataBuilder)
validateAndUpdateRowLineage(snapshot *Snapshot) error
}
newNextRowID := nextRowID + *snapshot.AddedRows
+ if newNextRowID <= nextRowID {
+ return fmt.Errorf("%w: next-row-id must advance from previous
%d to new %d (added-rows %d)",
Review Comment:
When AddedRows is 0 this prints `from previous 50 to new 50` — same value
twice, reads like a formatting bug. If the check stays in some form, consider:
`next-row-id did not advance: added-rows is %d (table next-row-id %d)`.
##########
table/rebuild_manifest_test.go:
##########
@@ -64,18 +64,22 @@ func newV3MetadataWithNextRowID(t *testing.T, nextRowID
int64) Metadata {
txn, _ := createTestTransactionWithMemIO(t, spec)
txn.meta.formatVersion = 3
- firstRowID := int64(0)
- addedRows := nextRowID
- snap := Snapshot{
- SnapshotID: 1000,
- SequenceNumber: 1,
- TimestampMs: txn.meta.base.LastUpdatedMillis() + 1,
- Summary: &Summary{Operation: OpAppend},
- FirstRowID: &firstRowID,
- AddedRows: &addedRows,
+ // Only add a snapshot if nextRowID > 0. A brand-new table has
NextRowID = 0
+ // without any snapshots.
+ if nextRowID > 0 {
+ firstRowID := int64(0)
+ addedRows := nextRowID
+ snap := Snapshot{
+ SnapshotID: 1000,
+ SequenceNumber: 1,
+ TimestampMs: txn.meta.base.LastUpdatedMillis() + 1,
+ Summary: &Summary{Operation: OpAppend},
+ FirstRowID: &firstRowID,
+ AddedRows: &addedRows,
+ }
+ require.NoError(t, txn.meta.AddSnapshot(&snap))
+ require.NoError(t, txn.meta.SetSnapshotRef(MainBranch, 1000,
BranchRef))
}
Review Comment:
The `if nextRowID > 0` guard is the tell — the helper had to be modified to
dodge the new check because adding a snapshot with addedRows=0 now fails.
That's bending the test around an over-restriction rather than fixing the
validation. Once the metadata.go check is corrected, this helper should revert
to its previous shape.
##########
table/metadata.go:
##########
@@ -505,6 +505,11 @@ func (b *MetadataBuilder)
validateAndUpdateRowLineage(snapshot *Snapshot) error
}
newNextRowID := nextRowID + *snapshot.AddedRows
+ if newNextRowID <= nextRowID {
+ return fmt.Errorf("%w: next-row-id must advance from previous
%d to new %d (added-rows %d)",
+ ErrInvalidRowLineage, nextRowID, newNextRowID,
*snapshot.AddedRows)
Review Comment:
`ErrInvalidRowLineage` signals a lineage-chain break — cursor backwards,
missing first-row-id. AddedRows=0 doesn't break the chain, it just doesn't
extend it. If the check is kept, a different sentinel matches the failure mode
better.
##########
table/metadata_builder_internal_test.go:
##########
@@ -1669,6 +1669,79 @@ func
TestAddSnapshotV3AcceptsFirstRowIDEqualToNextRowID(t *testing.T) {
require.Equal(t, int64(100), *builder.nextRowID)
}
+func TestAddSnapshotV3NextRowIDMustAdvance(t *testing.T) {
+ // Test that next-row-id must advance after applying a snapshot
+ builder := builderWithoutChanges(3)
+ schemaID := 0
+ firstRowID := int64(0)
+ addedRows := int64(50)
+
+ snapshot := Snapshot{
+ SnapshotID: 1,
+ ParentSnapshotID: nil,
+ SequenceNumber: 0,
+ TimestampMs: builder.base.LastUpdatedMillis() + 1,
+ ManifestList: "/snap-1.avro",
+ Summary: &Summary{Operation: OpAppend},
+ SchemaID: &schemaID,
+ FirstRowID: &firstRowID,
+ AddedRows: &addedRows,
+ }
+
+ require.NoError(t, builder.AddSnapshot(&snapshot))
+ require.Equal(t, int64(50), *builder.nextRowID)
+}
Review Comment:
The name implies this verifies the must-advance invariant, but the body only
adds 50 rows and asserts NoError + the resulting nextRowID — that behavior was
already in place before this PR. Either rename to
`TestAddSnapshotV3AcceptsPositiveAddedRows` (mirroring
`TestAddSnapshotV3AcceptsFirstRowIDEqualToNextRowID` just above) or fold it
with the zero-rows case into a single table-driven test once the underlying
check is fixed.
##########
table/metadata_builder_internal_test.go:
##########
@@ -1669,6 +1669,79 @@ func
TestAddSnapshotV3AcceptsFirstRowIDEqualToNextRowID(t *testing.T) {
require.Equal(t, int64(100), *builder.nextRowID)
}
+func TestAddSnapshotV3NextRowIDMustAdvance(t *testing.T) {
+ // Test that next-row-id must advance after applying a snapshot
+ builder := builderWithoutChanges(3)
+ schemaID := 0
+ firstRowID := int64(0)
+ addedRows := int64(50)
+
+ snapshot := Snapshot{
+ SnapshotID: 1,
+ ParentSnapshotID: nil,
+ SequenceNumber: 0,
+ TimestampMs: builder.base.LastUpdatedMillis() + 1,
+ ManifestList: "/snap-1.avro",
+ Summary: &Summary{Operation: OpAppend},
+ SchemaID: &schemaID,
+ FirstRowID: &firstRowID,
+ AddedRows: &addedRows,
+ }
+
+ require.NoError(t, builder.AddSnapshot(&snapshot))
+ require.Equal(t, int64(50), *builder.nextRowID)
+}
+
+func TestAddSnapshotV3RejectsZeroAddedRows(t *testing.T) {
+ // Test that a snapshot with AddedRows = 0 is rejected (next-row-id
would not advance)
+ builder := builderWithoutChanges(3)
+ schemaID := 0
+ firstRowID := int64(0)
+ addedRows := int64(0)
+
+ snapshot := Snapshot{
+ SnapshotID: 1,
+ ParentSnapshotID: nil,
+ SequenceNumber: 0,
+ TimestampMs: builder.base.LastUpdatedMillis() + 1,
+ ManifestList: "/snap-1.avro",
+ Summary: &Summary{Operation: OpAppend},
+ SchemaID: &schemaID,
+ FirstRowID: &firstRowID,
+ AddedRows: &addedRows,
+ }
+
+ err := builder.AddSnapshot(&snapshot)
+ require.ErrorIs(t, err, ErrInvalidRowLineage)
+ require.ErrorContains(t, err, "next-row-id must advance")
+ require.ErrorContains(t, err, "added-rows 0")
+}
+
+func TestAddSnapshotV3RejectsNegativeAddedRowsAtUpdate(t *testing.T) {
+ // Test that negative AddedRows would not advance next-row-id and is
rejected
+ builder := builderWithoutChanges(3)
+ schemaID := 0
+ firstRowID := int64(50)
+ negativeAddedRows := int64(-10)
+
+ snapshot := Snapshot{
+ SnapshotID: 1,
+ ParentSnapshotID: nil,
+ SequenceNumber: 0,
+ TimestampMs: builder.base.LastUpdatedMillis() + 1,
+ ManifestList: "/snap-1.avro",
+ Summary: &Summary{Operation: OpAppend},
+ SchemaID: &schemaID,
+ FirstRowID: &firstRowID,
+ AddedRows: &negativeAddedRows,
+ }
+
+ err := builder.AddSnapshot(&snapshot)
+ // First rejection should be from ValidateRowLineage (added-rows cannot
be negative)
+ require.ErrorIs(t, err, ErrInvalidRowLineage)
+ require.ErrorContains(t, err, "added-rows cannot be negative")
Review Comment:
The function name says this covers the new monotonic-advance branch, but the
comment on line 1740 admits the rejection comes from `ValidateRowLineage`. The
new branch never runs for negative AddedRows — `ValidateRowLineage` fires
first. So this test gives no coverage for the change being made. Drop it or
rename to reflect that it's re-verifying the upstream gate.
--
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]
