[GitHub] [arrow] nevi-me commented on a change in pull request #9025: ARROW-10259: [Rust] Add custom metadata to Field

[GitBox <gi...@apache.org>]

nevi-me commented on a change in pull request #9025:
URL: https://github.com/apache/arrow/pull/9025#discussion_r552525224



##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -1296,9 +1301,31 @@ impl Field {
             nullable,
             dict_id,
             dict_is_ordered,
+            metadata: None,
         }
     }
 
+    /// Sets the `Field`'s optional custom metadata.
+    /// The metadata is set as `None` for empty map.
+    /// Return cloned `Self`.
+    #[inline]
+    pub fn with_metadata(&mut self, metadata: Option<BTreeMap<String, String>>) -> Self {

Review comment:
       I think we shouldn't pass a mutable self, then return a new self, because this is going to mutate the incoming `Field` and return a clone of it, i.e.:
   
   ```rust
   let mut field = Field::new("my_field", DataType::Int32, false);
   let field2 = field.with_metadata(Some(metadata));
   assert_eq!(field, field2); // this will be true, so we're creating 2 fields
   ```
   
   It would be better to return nothing from the function, as we've already mutated `&mut self`.

##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -1903,9 +1929,20 @@ mod tests {
 
     #[test]
     fn serde_struct_type() {
+        let kv_array = [("k".to_string(), "v".to_string())];

Review comment:
       Sorry, forgot to reply earlier. Yeah, there's no harm in keeping the behaviour as is in this PR

##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -1500,6 +1590,7 @@ impl Field {
     }
 
     /// Merge field into self if it is compatible. Struct will be merged recursively.
+    /// NOTE: `self` may be updated to unexpected state in case of merge failure.

Review comment:
       I think this is fine, I'm happy with someone who encounters an issue here in future, providing an alternative implementation that doesn't partially mutate `&mut self` if there's a failure.
   
   @houqp any opinion here, as you contributed this function?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org