danielcweeks commented on code in PR #12334: URL: https://github.com/apache/iceberg/pull/12334#discussion_r1966349930
########## format/spec.md: ########## @@ -1754,6 +1754,14 @@ Snapshot summary can include metrics fields to track numeric stats of the snapsh | **`engine-name`** | "spark" | Name of the engine that created the snapshot | | **`engine-version`** | "3.5.4" | Version of the engine that created the snapshot | +### Assignment of Snapshot IDs and `current-snapshot-id` + +Writers should produce positive values for snapshot ids in a manner that minimizes the probability of id collisions and should verify the id does not conflict with existing snapshots. Producing snapshot ids based on timestamps alone is not recommended as it increases the potential for collisions. Review Comment: There's nothing in the spec that that explicitly prohibits negative values, but the approach by the reference implementation intentionally avoids negative snapshot ids. There is commentary on this in #57 and #2506, so I think there is an expectation even if not explicitly called out. It's not entirely clear why other than negative snapshots is somewhat awkward. Originally, it was just `System.currentTimeMillis()` (which resulted in collisions), so I think just historically positive. -- 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: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
