stevenzwu commented on code in PR #16293:
URL: https://github.com/apache/iceberg/pull/16293#discussion_r4211266693
##########
core/src/test/java/org/apache/iceberg/TestTableMetadata.java:
##########
@@ -2186,4 +2187,60 @@ public void testSetRefRejectsTagForMainBranch() {
.build();
assertThat(withTag.ref("tag1").isTag()).isTrue();
}
+
+ @Test
+ public void addSnapshotRejectsNonMonotonicTimestampForV4() {
+ TableMetadata base =
+ TableMetadata.newTableMetadata(
+ TEST_SCHEMA,
+ PartitionSpec.unpartitioned(),
+ TEST_LOCATION,
+ ImmutableMap.of(TableProperties.FORMAT_VERSION, "4"));
+
+ Snapshot parent =
+ new BaseSnapshot(1, 1L, null, 1_000L, null, null, null,
"file:/s1.avro", 0L, 0L, null);
+ TableMetadata withParent =
TableMetadata.buildFrom(base).addSnapshot(parent).build();
+
+ Snapshot child =
+ new BaseSnapshot(2, 2L, 1L, 1_000L, null, null, null, "file:/s2.avro",
0L, 0L, null);
+
+ assertThatThrownBy(() ->
TableMetadata.buildFrom(withParent).addSnapshot(child))
+ .isInstanceOf(ValidationException.class)
+ .hasMessage("Invalid snapshot timestamp 1000: not after parent
snapshot 1 at 1000");
+ }
+
+ @Test
+ public void addSnapshotAllowsNonMonotonicTimestampBeforeV4() {
+ TableMetadata base =
+ TableMetadata.newTableMetadata(
+ TEST_SCHEMA, PartitionSpec.unpartitioned(), "location",
ImmutableMap.of());
Review Comment:
`format-version` is now passed in the properties map for each version in
`V3_AND_BELOW`.
##########
core/src/test/java/org/apache/iceberg/TestTableMetadata.java:
##########
@@ -2186,4 +2187,60 @@ public void testSetRefRejectsTagForMainBranch() {
.build();
assertThat(withTag.ref("tag1").isTag()).isTrue();
}
+
+ @Test
+ public void addSnapshotRejectsNonMonotonicTimestampForV4() {
+ TableMetadata base =
+ TableMetadata.newTableMetadata(
+ TEST_SCHEMA,
+ PartitionSpec.unpartitioned(),
+ TEST_LOCATION,
+ ImmutableMap.of(TableProperties.FORMAT_VERSION, "4"));
+
+ Snapshot parent =
+ new BaseSnapshot(1, 1L, null, 1_000L, null, null, null,
"file:/s1.avro", 0L, 0L, null);
+ TableMetadata withParent =
TableMetadata.buildFrom(base).addSnapshot(parent).build();
+
+ Snapshot child =
+ new BaseSnapshot(2, 2L, 1L, 1_000L, null, null, null, "file:/s2.avro",
0L, 0L, null);
+
+ assertThatThrownBy(() ->
TableMetadata.buildFrom(withParent).addSnapshot(child))
+ .isInstanceOf(ValidationException.class)
+ .hasMessage("Invalid snapshot timestamp 1000: not after parent
snapshot 1 at 1000");
+ }
+
+ @Test
+ public void addSnapshotAllowsNonMonotonicTimestampBeforeV4() {
Review Comment:
Parameterized on `TestHelpers.V3_AND_BELOW`.
v1 still uses sequence number 0, and v3 sets row-lineage fields, since those
versions do not share one snapshot shape.
##########
core/src/test/java/org/apache/iceberg/TestTableMetadata.java:
##########
@@ -2186,4 +2187,60 @@ public void testSetRefRejectsTagForMainBranch() {
.build();
assertThat(withTag.ref("tag1").isTag()).isTrue();
}
+
+ @Test
+ public void addSnapshotRejectsNonMonotonicTimestampForV4() {
Review Comment:
Used `TestHelpers.V4_AND_ABOVE` instead of `assumeThat`.
##########
core/src/test/java/org/apache/iceberg/TestTableMetadata.java:
##########
@@ -2186,4 +2187,60 @@ public void testSetRefRejectsTagForMainBranch() {
.build();
assertThat(withTag.ref("tag1").isTag()).isTrue();
}
+
+ @Test
+ public void addSnapshotRejectsNonMonotonicTimestampForV4() {
+ TableMetadata base =
+ TableMetadata.newTableMetadata(
+ TEST_SCHEMA,
+ PartitionSpec.unpartitioned(),
+ TEST_LOCATION,
+ ImmutableMap.of(TableProperties.FORMAT_VERSION, "4"));
+
+ Snapshot parent =
+ new BaseSnapshot(1, 1L, null, 1_000L, null, null, null,
"file:/s1.avro", 0L, 0L, null);
+ TableMetadata withParent =
TableMetadata.buildFrom(base).addSnapshot(parent).build();
+
+ Snapshot child =
+ new BaseSnapshot(2, 2L, 1L, 1_000L, null, null, null, "file:/s2.avro",
0L, 0L, null);
+
+ assertThatThrownBy(() ->
TableMetadata.buildFrom(withParent).addSnapshot(child))
+ .isInstanceOf(ValidationException.class)
+ .hasMessage("Invalid snapshot timestamp 1000: not after parent
snapshot 1 at 1000");
+ }
+
+ @Test
+ public void addSnapshotAllowsNonMonotonicTimestampBeforeV4() {
+ TableMetadata base =
+ TableMetadata.newTableMetadata(
+ TEST_SCHEMA, PartitionSpec.unpartitioned(), "location",
ImmutableMap.of());
+
+ Snapshot parent =
+ new BaseSnapshot(1, 1L, null, 1_000L, null, null, null,
"file:/s1.avro", null, null, null);
+ TableMetadata withParent =
TableMetadata.buildFrom(base).addSnapshot(parent).build();
+
+ Snapshot child =
+ new BaseSnapshot(2, 2L, 1L, 999L, null, null, null, "file:/s2.avro",
null, null, null);
+
+ TableMetadata withChild =
TableMetadata.buildFrom(withParent).addSnapshot(child).build();
+ assertThat(withChild.snapshot(2L).timestampMillis()).isEqualTo(999L);
+ }
+
+ @Test
+ public void snapshotTimestampMayBeAheadOfLastUpdatedMillis() {
Review Comment:
This behavior is not limited to versions below 4, so the test runs on
`ALL_VERSIONS`. It is renamed to
`snapshotTimestampMayBeAheadOfTableMetadataLastUpdatedMillis` to be more clear
on the scenario
--
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]
