Skip to content

Validate snapshot timestamp is not before the last snapshot log entry#3632

Open
AmadNaseem wants to merge 1 commit into
apache:mainfrom
AmadNaseem:fix/snapshot-timestamp-validity-2938
Open

Validate snapshot timestamp is not before the last snapshot log entry#3632
AmadNaseem wants to merge 1 commit into
apache:mainfrom
AmadNaseem:fix/snapshot-timestamp-validity-2938

Conversation

@AmadNaseem

Copy link
Copy Markdown

What

Aligns PyIceberg with the Java and Rust implementations by rejecting a snapshot whose timestamp is meaningfully before the table's most recent state, using the same one-minute clock-drift tolerance the other implementations use (ONE_MINUTE = 60_000 ms).

Two checks are added, matching what was discussed on the earlier attempts (#3062, #3443, both closed by the stale bot for inactivity rather than on merit):

  1. Metadata construction time (pyiceberg/table/metadata.py): a check_last_updated_ms model_validator on TableMetadataV1/V2/V3 raises ValidationError when last_updated_ms is more than one minute before the most recent snapshot_log entry. This mirrors Java's TableMetadata constructor check, which is where @Fokko suggested it belongs in fix(table): validate snapshot timestamp drift on add snapshot #3062. It is tolerant of an empty/absent snapshot log (like Java's if (last != null)).
  2. Add-snapshot time (pyiceberg/table/update/__init__.py): the AddSnapshotUpdate handler rejects a new snapshot dated more than one minute before last_updated_ms, beside the existing sequence-number and first-row-id checks.

References: Iceberg spec (snapshot validity); Java TableMetadata; Rust TableMetadataBuilder::add_snapshot.

Why it matters

decimal-style silent acceptance aside, PyIceberg currently accepts snapshots/metadata that Java and Rust reject, and last_updated_ms could move backward. Aligning avoids producing metadata other engines consider invalid.

Notes for reviewers

  • Behavior change at parse time: the construction-time validator runs on every load, so metadata whose last_updated_ms is >60s behind its newest snapshot-log entry (spec-invalid, but previously loadable in PyIceberg) will now raise on load. This is intentional alignment with Java/Rust; flagging it so it's approved with eyes open.
  • Scope: this implements the last_updated_ms vs last-snapshot-log-entry check. Java additionally asserts snapshot-log sortedness; I've left that out to keep the change focused (it's an internal invariant rather than the behavior reported in Align with other Iceberg implementations of snapshot timestamp validity #2938) — happy to add it as a follow-up if you'd like.
  • Error type: kept ValueError on the add-snapshot path for consistency with the sibling checks in that handler (rather than CommitFailedException); easy to change if you prefer.
  • Test fixture: example_table_metadata_v2_with_extensive_snapshots hardcoded a 2020 last-updated-ms while generating current-time snapshot-log entries (internally inconsistent / spec-invalid), so it's updated to derive last-updated-ms from the log. No test asserted on the old value.

Tests

Added boundary tests on both paths (-60_000 ms accepted, -60_001 ms rejected) and a construction-time test. Local run of tests/table/test_init.py test_metadata.py test_snapshots.py passes (pre-existing Windows-only StaticTable path errors aside); ruff clean.

Signed-off-by: Amad Naseem <rajaamad19@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant