[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_r549593402



##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -1594,7 +1620,7 @@ impl Field {
 
 impl fmt::Display for Field {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(f, "{}: {:?}", self.name, self.data_type)
+        write!(f, "{:?}", self)

Review comment:
       This defaults us to `Display` which we could `#[derive(Display)]` instead. I think we could remove this function in that case.

##########
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:
       We should serialise the metadata to a JSON struct in the form of:
   
   ```json
   {
     "metadata": [
       {"key": "k", "value": "v"}
     ]
   }
   ```
   
   There are integration tests that we need to enable, I'll find the relevant file in the repository.

##########
File path: rust/arrow/src/datatypes.rs
##########
@@ -22,6 +22,7 @@
 //!  * [`Field`](crate::datatypes::Field) to describe one field within a schema.
 //!  * [`DataType`](crate::datatypes::DataType) to describe the type of a field.
 
+use std::collections::BTreeMap;

Review comment:
       Is there value in using `BTreeMap` for both metadata types?

##########
File path: rust/arrow/src/util/integration_util.rs
##########
@@ -60,13 +60,22 @@ pub struct ArrowJsonField {
 
 impl From<&Field> for ArrowJsonField {
     fn from(field: &Field) -> Self {
+        let mut metadata_value = None;
+        if let Some(kv_list) = field.metadata() {
+            let mut json_map = VMap::new();
+            for (k, v) in kv_list {
+                json_map.insert(k.clone(), Value::String(v.clone()));
+            }
+            metadata_value = Some(Value::Object(json_map));
+        }
+
         Self {
             name: field.name().to_string(),
             field_type: field.data_type().to_json(),
             nullable: field.is_nullable(),
             children: vec![],
-            dictionary: None, // TODO: not enough info
-            metadata: None,   // TODO(ARROW-10259)
+            dictionary: None,         // TODO: not enough info
+            metadata: metadata_value, // TODO(ARROW-10259): metadata is not used.

Review comment:
       This is now safe to remove, as you're addressing it in this PR




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