fixed by review note

Author: Vova Kolmakov

rust/lance-core/src/datatypes/field.rs

@@ -1023,17 +1023,22 @@ impl Field {
     ///
     /// Call this after modifying `field.metadata` directly (e.g., via UpdateConfig)
     /// to keep structured fields like `unenforced_primary_key_position` in sync.
-    pub fn sync_embedded_metadata(&mut self) {
-        self.unenforced_primary_key_position = self
-            .metadata
-            .get(LANCE_UNENFORCED_PRIMARY_KEY_POSITION)
-            .and_then(|s| s.parse::<u32>().ok())
-            .or_else(|| {
+    pub fn sync_embedded_metadata(&mut self) -> Result<()> {
+        self.unenforced_primary_key_position =
+            if let Some(s) = self.metadata.get(LANCE_UNENFORCED_PRIMARY_KEY_POSITION) {
+                Some(s.parse::<u32>().map_err(|e| {
+                    Error::invalid_input(format!(
+                        "Invalid value '{}' for {}: {}",
+                        s, LANCE_UNENFORCED_PRIMARY_KEY_POSITION, e
+                    ))
+                })?)
+            } else {
                 self.metadata
                     .get(LANCE_UNENFORCED_PRIMARY_KEY)
                     .filter(|s| matches!(s.to_lowercase().as_str(), "true" | "1" | "yes"))
                     .map(|_| 0)
-            });
+            };
+        Ok(())
     }
 }
 
@@ -1153,7 +1158,7 @@ impl TryFrom<&ArrowField> for Field {
             dictionary: None,
             unenforced_primary_key_position: None,
         };
-        result.sync_embedded_metadata();
+        result.sync_embedded_metadata()?;
         Ok(result)
     }
 }
@@ -1840,4 +1845,25 @@ mod tests {
             .unwrap();
         assert_eq!(unloaded_projected, unloaded);
     }
+
+    #[test]
+    fn test_try_from_arrow_field_invalid_pk_position_returns_error() {
+        let arrow_field =
+            ArrowField::new("id", DataType::Int32, false).with_metadata(HashMap::from([(
+                LANCE_UNENFORCED_PRIMARY_KEY_POSITION.to_string(),
+                "not_a_number".to_string(),
+            )]));
+
+        let result = Field::try_from(&arrow_field);
+        assert!(
+            result.is_err(),
+            "Invalid pk position should fail in TryFrom"
+        );
+        let err_msg = result.unwrap_err().to_string();
+        assert!(
+            err_msg.contains("not_a_number"),
+            "Error should include the invalid value: {}",
+            err_msg
+        );
+    }
 }

rust/lance/src/dataset/metadata.rs

@@ -838,4 +838,61 @@ mod tests {
             ]
         );
     }
+
+    #[tokio::test]
+    async fn test_update_field_metadata_invalid_pk_position_returns_error() {
+        let mut dataset = test_dataset_for_pk().await;
+
+        let result = dataset
+            .update_field_metadata()
+            .update(
+                "id",
+                [("lance-schema:unenforced-primary-key:position", "abc")],
+            )
+            .unwrap()
+            .await;
+
+        assert!(result.is_err(), "Non-numeric position should return error");
+        assert!(matches!(result.unwrap_err(), Error::InvalidInput { .. }));
+    }
+
+    #[tokio::test]
+    async fn test_update_field_metadata_negative_pk_position_returns_error() {
+        let mut dataset = test_dataset_for_pk().await;
+
+        let result = dataset
+            .update_field_metadata()
+            .update(
+                "id",
+                [("lance-schema:unenforced-primary-key:position", "-1")],
+            )
+            .unwrap()
+            .await;
+
+        assert!(result.is_err(), "Negative position should return error");
+        assert!(matches!(result.unwrap_err(), Error::InvalidInput { .. }));
+    }
+
+    #[tokio::test]
+    async fn test_update_field_metadata_overflow_pk_position_returns_error() {
+        let mut dataset = test_dataset_for_pk().await;
+
+        let result = dataset
+            .update_field_metadata()
+            .update(
+                "id",
+                [(
+                    "lance-schema:unenforced-primary-key:position",
+                    "99999999999",
+                )],
+            )
+            .unwrap()
+            .await;
+
+        assert!(
+            result.is_err(),
+            "Overflowing u32 position should return error"
+        );
+        assert!(matches!(result.unwrap_err(), Error::InvalidInput { .. }));
+    }
 }

rust/lance/src/dataset/transaction.rs

@@ -2327,7 +2327,7 @@ impl Transaction {
                 for (field_id, field_metadata_update) in field_metadata_updates {
                     if let Some(field) = manifest.schema.field_by_id_mut(*field_id) {
                         apply_update_map(&mut field.metadata, field_metadata_update);
-                        field.sync_embedded_metadata();
+                        field.sync_embedded_metadata()?;
                     } else {
                         return Err(Error::invalid_input_source(
                             format!("Field with id {} does not exist", field_id).into(),