Validate snapshot timestamp is not before the last snapshot log entry#3632
Open
AmadNaseem wants to merge 1 commit into
Open
Validate snapshot timestamp is not before the last snapshot log entry#3632AmadNaseem wants to merge 1 commit into
AmadNaseem wants to merge 1 commit into
Conversation
Signed-off-by: Amad Naseem <rajaamad19@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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):
pyiceberg/table/metadata.py): acheck_last_updated_msmodel_validatoronTableMetadataV1/V2/V3raisesValidationErrorwhenlast_updated_msis more than one minute before the most recentsnapshot_logentry. This mirrors Java'sTableMetadataconstructor 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'sif (last != null)).pyiceberg/table/update/__init__.py): theAddSnapshotUpdatehandler rejects a new snapshot dated more than one minute beforelast_updated_ms, beside the existing sequence-number and first-row-id checks.References: Iceberg spec (snapshot validity); Java
TableMetadata; RustTableMetadataBuilder::add_snapshot.Why it matters
decimal-style silent acceptance aside, PyIceberg currently accepts snapshots/metadata that Java and Rust reject, andlast_updated_mscould move backward. Aligning avoids producing metadata other engines consider invalid.Notes for reviewers
last_updated_msis >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.last_updated_msvs 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.ValueErroron the add-snapshot path for consistency with the sibling checks in that handler (rather thanCommitFailedException); easy to change if you prefer.example_table_metadata_v2_with_extensive_snapshotshardcoded a 2020last-updated-mswhile generating current-time snapshot-log entries (internally inconsistent / spec-invalid), so it's updated to derivelast-updated-msfrom the log. No test asserted on the old value.Tests
Added boundary tests on both paths (
-60_000 msaccepted,-60_001 msrejected) and a construction-time test. Local run oftests/table/test_init.py test_metadata.py test_snapshots.pypasses (pre-existing Windows-onlyStaticTablepath errors aside);ruffclean.