toutane commented on PR #2395:
URL: https://github.com/apache/iceberg-rust/pull/2395#issuecomment-4378092220
Thanks for catching this! The fix works, but I'd suggest considering an
alternative that addresses the root cause rather than relaxing the assertion.
**The underlying issue** is in `TableMetadataBuilder::build()`
(`crates/iceberg/src/spec/table_metadata_builder.rs`):
```rust
self.metadata.last_updated_ms = self
.last_updated_ms
.unwrap_or_else(|| chrono::Utc::now().timestamp_millis());
```
When no explicit timestamp is provided, `build()` calls `Utc::now()`. If a
table creation and an update both call `build()` within the same millisecond,
they get the same timestamp — hence the flaky test.
**Alternative fix** — guarantee strict monotonic increase in the builder
itself:
```rust
self.metadata.last_updated_ms = self.last_updated_ms.unwrap_or_else(|| {
// Guarantee strict monotonic increase when building from existing
metadata.
chrono::Utc::now()
.timestamp_millis()
.max(self.metadata.last_updated_ms + 1)
});
```
At the point of this assignment, `self.metadata.last_updated_ms` still holds
the *previous* metadata's timestamp (set by `new_from_metadata`). Taking
`max(now, prev + 1)` ensures the new timestamp is always strictly greater,
without ever going backwards if the clock happens to be the same.
**Why this is preferable:**
- The original `<` assertion correctly captures the invariant that every
commit must advance `last_updated_ms`. Relaxing it to `<=` hides the fact that
two successive updates could share the same timestamp, which could silently
break any downstream logic that relies on ordering (snapshot logs, metadata
logs, conflict detection, etc.).
- The builder fix benefits all callers — not just this one test — and makes
the invariant hold by construction.
For a fresh table creation (`new` / `from_table_creation`), the initial
`last_updated_ms` is `0`, so `max(now, 1) = now` — no behavior change there.
Happy to open a follow-up PR if useful!
--
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]
