fix(table): validate snapshot timestamp drift on add snapshot

[mrutunjay-kinagi]

Rationale for this change

Align snapshot timestamp validation with other Iceberg implementations by rejecting snapshots that drift backwards by more than one minute.

The new guard in AddSnapshotUpdate checks:

• snapshot timestamp vs latest snapshot_log entry
• snapshot timestamp vs table last_updated_ms

Both checks allow up to 60 seconds of clock skew tolerance.

Are these changes tested?

Yes.

• Added test_update_metadata_add_snapshot_rejects_old_timestamp_vs_snapshot_log.
• Added test_update_metadata_add_snapshot_rejects_old_timestamp_vs_last_updated.
• Verified with targeted pytest run.

Are there any user-facing changes?

No API changes.

• Invalid commit metadata now fails fast with explicit ValueError when timestamp drift exceeds tolerance.

[Fokko]

@mrutunjay-kinagi Can you explain more about the use-case? I'm hesitant with these kind of features since it isn't implemented in other OSS implementations AFAIK.

[Fokko]

Maybe CommitFailedException?

[mrutunjay-kinagi]

Good point. I kept ValueError for consistency with the existing validation branches in this same method (sequence/row-id checks all currently raise ValueError). If you prefer, I can switch this validation path to CommitFailedException in a follow-up commit.

[mrutunjay-kinagi]

@Fokko Thanks for the review. The use case here is handling malformed/clock-skewed commit metadata safely and aligning behavior with Java/Rust, which already apply a 1-minute backward-drift tolerance for snapshot timestamps.\n\nWithout this guard, a client can add a snapshot timestamp that is significantly older than current table metadata ( / latest ), which can make timeline semantics inconsistent for timestamp-based operations.\n\nThis change keeps the same 60s tolerance and only rejects larger backward drift; small skew remains allowed.

[mrutunjay-kinagi]

Clarification on my previous comment (formatting issue):

The guard is intended to prevent adding snapshots that are significantly older than the table timeline, specifically older than last_updated_ms or the latest snapshot_log entry by more than 60 seconds.

This follows the same tolerance approach used in Java/Rust and keeps small clock skew allowed while rejecting larger backward drift.

[kevinjqliu]

This follows the same tolerance approach used in Java/Rust and keeps small clock skew allowed while rejecting larger backward drift.

could you point to the java/rust references?

[mrutunjay-kinagi]

The existing validation branches inside are raising , so for symmetry I left this guard as as well. If you’d prefer switching to later that’s easy to follow up on but I’d keep the current behavior for consistency.

[mrutunjay-kinagi]

Reposting cleanly: the guard maintains the same exception type as the surrounding validation branches, so I kept it as ValueError for consistency. If you’d prefer CommitFailedException later, happy to follow up.

[mrutunjay-kinagi]

@kevinjqliu Here are the references we aligned with:

• Java enforces the same tolerance inside TableMetadata’s constructor (lastUpdatedMillis - last.timestampMillis >= -ONE_MINUTE): https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/TableMetadata.java#L360-L377
• Rust’s TableMetadataBuilder applies the same check before adding a snapshot (snapshot.timestamp_ms must not be more than 1 minute behind the latest log entry or last_updated_ms): https://github.com/apache/iceberg-rust/blob/main/crates/iceberg/src/spec/table_metadata_builder.rs#L371-L399

[Fokko]

Hey @mrutunjay-kinagi Looking at the code:

https://github.com/apache/iceberg/blame/d714576c7d53300e4b38d5ee27743dedbb153ace/core/src/main/java/org/apache/iceberg/TableMetadata.java#L360-L377

The first loop will ensure that the snapshot log entries are sorted when parsing the JSON. The second checks that the last snapshot entry is not ahead of the lastUpdatedMillis. If we want to add this to PyIceberg, I think a better place is to do this when constructing the Metadata: https://github.com/apache/iceberg-python/blob/main/pyiceberg/table/metadata.py Similar to what Java/Rust are doing. Thoughts?

[mrutunjay-kinagi]

@Fokko

Thanks, good suggestion. I pushed a follow-up commit that adds metadata-level timestamp validation in pyiceberg/table/metadata.py so we validate on metadata construction as well:

• snapshot-log entries must remain sorted (with the same 1-minute skew tolerance used in Java)
• last-updated-ms cannot be more than 1 minute behind the latest snapshot-log entry

I also added parser-level tests in tests/table/test_metadata.py for both checks.

I kept the existing AddSnapshotUpdate guard too, because that check depends on prior metadata state during update application (metadata parsing alone cannot fully recover that context).