zeroshade commented on code in PR #1348:
URL: https://github.com/apache/iceberg-go/pull/1348#discussion_r3521378581
##########
table/metadata_builder_internal_test.go:
##########
@@ -1702,13 +1703,61 @@ func
TestAddSnapshotV3AcceptsFirstRowIDEqualToNextRowID(t *testing.T) {
require.Equal(t, int64(100), *builder.nextRowID)
}
-func TestAddSnapshotV3AcceptsPositiveAddedRows(t *testing.T) {
- // Positive added-rows should advance next-row-id.
+func TestAddSnapshotV3AcceptsZeroAddedRows(t *testing.T) {
+ // Asserts spec-permitted no-op behavior: a zero-added-rows snapshot
+ // (delete-only or metadata-only commit) leaves next-row-id unchanged.
The
+ // cursor is first advanced to a non-zero value so the assertion is
+ // falsifiable — a regression that advanced the cursor on every commit
would
+ // be caught. This is NOT coverage of the overflow guard — see
+ // TestAddSnapshotV3RejectsNextRowIDOverflow.
builder := builderWithoutChanges(3)
schemaID := 0
- firstRowID := int64(0)
- addedRows := int64(50)
+ appendFirstRowID := int64(0)
+ appendAddedRows := int64(50)
+ appendSnapshot := Snapshot{
+ SnapshotID: 1,
+ ParentSnapshotID: nil,
+ SequenceNumber: 0,
+ TimestampMs: builder.base.LastUpdatedMillis() + 1,
+ ManifestList: "/snap-1.avro",
+ Summary: &Summary{Operation: OpAppend},
+ SchemaID: &schemaID,
+ FirstRowID: &appendFirstRowID,
+ AddedRows: &appendAddedRows,
+ }
+ require.NoError(t, builder.AddSnapshot(&appendSnapshot))
+ require.Equal(t, int64(50), *builder.nextRowID)
+
+ deleteFirstRowID := int64(50)
+ deleteAddedRows := int64(0)
+ deleteSnapshot := Snapshot{
+ SnapshotID: 2,
+ ParentSnapshotID: &appendSnapshot.SnapshotID,
+ SequenceNumber: 1,
+ TimestampMs: builder.base.LastUpdatedMillis() + 2,
+ ManifestList: "/snap-2.avro",
+ Summary: &Summary{Operation: OpDelete},
+ SchemaID: &schemaID,
+ FirstRowID: &deleteFirstRowID,
+ AddedRows: &deleteAddedRows,
+ }
+ require.NoError(t, builder.AddSnapshot(&deleteSnapshot))
+ require.Equal(t, int64(50), *builder.nextRowID)
+}
+
+func TestAddSnapshotV3RejectsNextRowIDOverflow(t *testing.T) {
+ // next-row-id + added-rows must not overflow int64. Seed the cursor
near
+ // the maximum so a positive added-rows tips it over. This is the only
input
+ // that reaches the guard: negative added-rows is rejected earlier by
+ // Snapshot.ValidateRowLineage.
+ builder := builderWithoutChanges(3)
+ nearMax := int64(math.MaxInt64 - 10)
+ builder.nextRowID = &nearMax
+
+ schemaID := 0
+ firstRowID := nearMax
+ addedRows := int64(20)
Review Comment:
Nit (optional): this rejects at `MaxInt64 - 10 + 20` = `MaxInt64 + 10`. To
pin the exact boundary against a future off-by-one, consider `addedRows :=
int64(11)` (sum == `MaxInt64 + 1`) for the reject case, plus a `MaxInt64 - 1`
accept case.
--
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]
