AmadNaseem opened a new pull request, #3632: URL: https://github.com/apache/iceberg-python/pull/3632
<!-- Closes #2938 --> ## 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 #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 #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. -- 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]
