fix(spec): clean up -1 snapshot ID sentinel usage and add deserialization test (#2294)

## Which issue does this PR close?

- Closes #352.

## What changes are included in this PR?

- Replaces hardcoded `-1` with `EMPTY_SNAPSHOT_ID` constant in table
metadata deserialization.
- Adds `test_empty_snapshot_id_is_normalized_to_none` to verify that the
Java-style `-1` sentinel for `current-snapshot-id` is normalized to
`None` during deserialization.
- Removes the public `UNASSIGNED_SNAPSHOT_ID` constant and moving it to
a private constant scoped to the manifest writer module.

## Are these changes tested?

Adds a test `test_empty_snapshot_id_is_normalized_to_none` verifying the
deserialization normalization.

Author: geserdugarov

crates/iceberg/src/spec/manifest/writer.rs

@@ -32,10 +32,14 @@ use crate::spec::manifest::_serde::{ManifestEntryV1, ManifestEntryV2};
 use crate::spec::manifest::{manifest_schema_v1, manifest_schema_v2};
 use crate::spec::{
     DataContentType, DataFile, FieldSummary, ManifestEntry, ManifestFile, ManifestMetadata,
-    ManifestStatus, PrimitiveLiteral, SchemaRef, StructType, UNASSIGNED_SNAPSHOT_ID,
+    ManifestStatus, PrimitiveLiteral, SchemaRef, StructType,
 };
 use crate::{Error, ErrorKind};
 
+/// Placeholder for snapshot ID. The field with this value must be replaced
+/// with the actual snapshot ID before it is committed.
+const UNASSIGNED_SNAPSHOT_ID: i64 = -1;
+
 /// The builder used to create a [`ManifestWriter`].
 pub struct ManifestWriterBuilder {
     output: OutputFile,

crates/iceberg/src/spec/snapshot.rs

@@ -33,8 +33,6 @@ use crate::{Error, ErrorKind};
 
 /// The ref name of the main branch of the table.
 pub const MAIN_BRANCH: &str = "main";
-/// Placeholder for snapshot ID. The field with this value must be replaced with the actual snapshot ID before it is committed.
-pub const UNASSIGNED_SNAPSHOT_ID: i64 = -1;
 
 /// Reference to [`Snapshot`].
 pub type SnapshotRef = Arc<Snapshot>;

crates/iceberg/src/spec/table_metadata.rs

@@ -47,6 +47,9 @@ use crate::{Error, ErrorKind};
 static MAIN_BRANCH: &str = "main";
 pub(crate) static ONE_MINUTE_MS: i64 = 60_000;
 
+/// Sentinel value used by the Java implementation and older metadata files
+/// to represent a missing/empty current snapshot ID. During deserialization,
+/// this value is normalized to `None`.
 pub(crate) static EMPTY_SNAPSHOT_ID: i64 = -1;
 pub(crate) static INITIAL_SEQUENCE_NUMBER: i64 = 0;
 
@@ -765,8 +768,8 @@ pub(super) mod _serde {
     use uuid::Uuid;
 
     use super::{
-        DEFAULT_PARTITION_SPEC_ID, FormatVersion, MAIN_BRANCH, MetadataLog, SnapshotLog,
-        TableMetadata,
+        DEFAULT_PARTITION_SPEC_ID, EMPTY_SNAPSHOT_ID, FormatVersion, MAIN_BRANCH, MetadataLog,
+        SnapshotLog, TableMetadata,
     };
     use crate::spec::schema::_serde::{SchemaV1, SchemaV2};
     use crate::spec::snapshot::_serde::{SnapshotV1, SnapshotV2, SnapshotV3};
@@ -950,7 +953,7 @@ pub(super) mod _serde {
                 encryption_keys,
                 snapshots,
             } = value;
-            let current_snapshot_id = if let &Some(-1) = &value.current_snapshot_id {
+            let current_snapshot_id = if value.current_snapshot_id == Some(EMPTY_SNAPSHOT_ID) {
                 None
             } else {
                 value.current_snapshot_id
@@ -1063,7 +1066,7 @@ pub(super) mod _serde {
         fn try_from(value: TableMetadataV2) -> Result<Self, self::Error> {
             let snapshots = value.snapshots;
             let value = value.shared;
-            let current_snapshot_id = if let &Some(-1) = &value.current_snapshot_id {
+            let current_snapshot_id = if value.current_snapshot_id == Some(EMPTY_SNAPSHOT_ID) {
                 None
             } else {
                 value.current_snapshot_id
@@ -1170,7 +1173,7 @@ pub(super) mod _serde {
     impl TryFrom<TableMetadataV1> for TableMetadata {
         type Error = Error;
         fn try_from(value: TableMetadataV1) -> Result<Self, Error> {
-            let current_snapshot_id = if let &Some(-1) = &value.current_snapshot_id {
+            let current_snapshot_id = if value.current_snapshot_id == Some(EMPTY_SNAPSHOT_ID) {
                 None
             } else {
                 value.current_snapshot_id
@@ -3300,6 +3303,18 @@ mod tests {
         check_table_metadata_serde(&metadata, expected);
     }
 
+    #[test]
+    fn test_empty_snapshot_id_is_normalized_to_none() {
+        let metadata =
+            fs::read_to_string("testdata/table_metadata/TableMetadataV1Valid.json").unwrap();
+        let deserialized: TableMetadata = serde_json::from_str(&metadata).unwrap();
+        assert_eq!(
+            deserialized.current_snapshot_id(),
+            None,
+            "current_snapshot_id of -1 should be deserialized as None"
+        );
+    }
+
     #[test]
     fn test_table_metadata_v1_compat() {
         let metadata =

crates/iceberg/src/spec/table_metadata_builder.rs

@@ -570,7 +570,7 @@ impl TableMetadataBuilder {
 
     /// Remove a reference
     ///
-    /// If `ref_name='main'` the current snapshot id is set to -1.
+    /// If `ref_name='main'` the current snapshot id is set to `None`.
     pub fn remove_ref(mut self, ref_name: &str) -> Self {
         if ref_name == MAIN_BRANCH {
             self.metadata.current_snapshot_id = None;