Validate snapshot timestamp is not before the last snapshot log entry

pyiceberg/table/metadata.py

@@ -68,6 +68,10 @@
 
 SUPPORTED_TABLE_FORMAT_VERSION = 2
 
+# Tolerance for clock drift when validating that last-updated-ms is not
+# before the most recent snapshot log entry. Matches the Java implementation.
+ONE_MINUTE_MS = 60_000
+
 
 def cleanup_snapshot_id(data: dict[str, Any]) -> dict[str, Any]:
     """Run before validation."""
@@ -115,6 +119,22 @@ def check_sort_orders(table_metadata: TableMetadata) -> TableMetadata:
     return table_metadata
 
 
+def check_last_updated_ms(table_metadata: TableMetadata) -> TableMetadata:
+    """Check that last-updated-ms is not before the most recent snapshot log entry.
+
+    A one minute tolerance is allowed because commits can happen concurrently
+    from different machines with small clock skew.
+    """
+    if snapshot_log := table_metadata.snapshot_log:
+        last_entry = snapshot_log[-1]
+        if table_metadata.last_updated_ms - last_entry.timestamp_ms < -ONE_MINUTE_MS:
+            raise ValidationError(
+                f"Invalid last updated timestamp {table_metadata.last_updated_ms}: "
+                f"before last snapshot log entry at {last_entry.timestamp_ms}"
+            )
+    return table_metadata
+
+
 def construct_refs(table_metadata: TableMetadata) -> TableMetadata:
     """Set the main branch if missing."""
     if table_metadata.current_snapshot_id is not None:
@@ -378,6 +398,10 @@ def cleanup_snapshot_id(cls, data: dict[str, Any]) -> dict[str, Any]:
     def construct_refs(self) -> TableMetadataV1:
         return construct_refs(self)
 
+    @model_validator(mode="after")
+    def check_last_updated_ms(self) -> TableMetadataV1:
+        return check_last_updated_ms(self)
+
     @model_validator(mode="before")
     def set_v2_compatible_defaults(cls, data: dict[str, Any]) -> dict[str, Any]:
         """Set default values to be compatible with the format v2.
@@ -519,6 +543,10 @@ def check_sort_orders(self) -> TableMetadata:
     def construct_refs(self) -> TableMetadata:
         return construct_refs(self)
 
+    @model_validator(mode="after")
+    def check_last_updated_ms(self) -> TableMetadata:
+        return check_last_updated_ms(self)
+
     format_version: Literal[2] = Field(alias="format-version", default=2)
     """An integer version number for the format. Implementations must throw
     an exception if a table’s version is higher than the supported version."""
@@ -563,6 +591,10 @@ def check_sort_orders(self) -> TableMetadata:
     def construct_refs(self) -> TableMetadata:
         return construct_refs(self)
 
+    @model_validator(mode="after")
+    def check_last_updated_ms(self) -> TableMetadata:
+        return check_last_updated_ms(self)
+
     format_version: Literal[3] = Field(alias="format-version", default=3)
     """An integer version number for the format. Implementations must throw
     an exception if a table’s version is higher than the supported version."""

pyiceberg/table/update/__init__.py

@@ -59,6 +59,10 @@
 
 U = TypeVar("U")
 
+# Tolerance for clock drift when validating that a new snapshot is not
+# recorded before the table's last update. Matches the Java and Rust implementations.
+ONE_MINUTE_MS = 60_000
+
 
 class UpdateTableMetadata(ABC, Generic[U]):
     _transaction: Transaction
@@ -444,6 +448,14 @@ def _(update: AddSnapshotUpdate, base_metadata: TableMetadata, context: _TableMe
             f"Cannot add snapshot with sequence number {update.snapshot.sequence_number} "
             f"older than last sequence number {base_metadata.last_sequence_number}"
         )
+    elif (
+        base_metadata.last_updated_ms is not None
+        and update.snapshot.timestamp_ms - base_metadata.last_updated_ms < -ONE_MINUTE_MS
+    ):
+        raise ValueError(
+            f"Invalid snapshot timestamp {update.snapshot.timestamp_ms}: "
+            f"before last updated timestamp {base_metadata.last_updated_ms}"
+        )
     elif base_metadata.format_version >= 3 and update.snapshot.first_row_id is None:
         raise ValueError("Cannot add snapshot without first row id")
     elif (

tests/conftest.py

@@ -819,7 +819,7 @@ def generate_snapshot(
         "table-uuid": "9c12d441-03fe-4693-9a96-a0705ddf69c1",
         "location": "s3://bucket/test/location",
         "last-sequence-number": 34,
-        "last-updated-ms": 1602638573590,
+        "last-updated-ms": max(entry["timestamp-ms"] for entry in snapshot_log),
         "last-column-id": 3,
         "current-schema-id": 1,
         "schemas": [

tests/table/test_init.py

@@ -1053,6 +1053,36 @@ def test_update_metadata_add_snapshot(table_v2: Table) -> None:
     assert new_metadata.last_updated_ms == new_snapshot.timestamp_ms
 
 
+def test_update_metadata_add_snapshot_before_last_updated(table_v2: Table) -> None:
+    new_snapshot = Snapshot(
+        snapshot_id=25,
+        parent_snapshot_id=19,
+        sequence_number=200,
+        timestamp_ms=table_v2.metadata.last_updated_ms - 60_001,
+        manifest_list="s3:/a/b/c.avro",
+        summary=Summary(Operation.APPEND),
+        schema_id=3,
+    )
+
+    with pytest.raises(ValueError, match="Invalid snapshot timestamp"):
+        update_table_metadata(table_v2.metadata, (AddSnapshotUpdate(snapshot=new_snapshot),))
+
+
+def test_update_metadata_add_snapshot_within_clock_drift(table_v2: Table) -> None:
+    new_snapshot = Snapshot(
+        snapshot_id=25,
+        parent_snapshot_id=19,
+        sequence_number=200,
+        timestamp_ms=table_v2.metadata.last_updated_ms - 60_000,
+        manifest_list="s3:/a/b/c.avro",
+        summary=Summary(Operation.APPEND),
+        schema_id=3,
+    )
+
+    new_metadata = update_table_metadata(table_v2.metadata, (AddSnapshotUpdate(snapshot=new_snapshot),))
+    assert new_metadata.snapshots[-1] == new_snapshot
+
+
 def test_update_metadata_set_ref_snapshot(table_v2: Table) -> None:
     update, _ = table_v2.transaction()._set_ref_snapshot(
         snapshot_id=3051729675574597004,

tests/table/test_metadata.py

@@ -145,6 +145,24 @@ def test_parsing_correct_types(example_table_metadata_v2: dict[str, Any]) -> Non
     assert isinstance(table_metadata.schemas[0].fields[0].field_type, LongType)
 
 
+def test_last_updated_ms_before_snapshot_log(example_table_metadata_v2: dict[str, Any]) -> None:
+    metadata = copy(example_table_metadata_v2)
+    last_entry_ms = metadata["snapshot-log"][-1]["timestamp-ms"]
+    metadata["last-updated-ms"] = last_entry_ms - 60_001
+
+    with pytest.raises(ValidationError, match="Invalid last updated timestamp"):
+        TableMetadataV2(**metadata)
+
+
+def test_last_updated_ms_within_clock_drift(example_table_metadata_v2: dict[str, Any]) -> None:
+    metadata = copy(example_table_metadata_v2)
+    last_entry_ms = metadata["snapshot-log"][-1]["timestamp-ms"]
+    metadata["last-updated-ms"] = last_entry_ms - 60_000
+
+    table_metadata = TableMetadataV2(**metadata)
+    assert table_metadata.last_updated_ms == last_entry_ms - 60_000
+
+
 def test_updating_metadata(example_table_metadata_v2: dict[str, Any]) -> None:
     """Test creating a new TableMetadata instance that's an updated version of
     an existing TableMetadata instance"""