[GitHub] [arrow-rs] alamb commented on a diff in pull request #2345: Clean the code in `field.rs` and add more tests

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

alamb commented on code in PR #2345:
URL: https://github.com/apache/arrow-rs/pull/2345#discussion_r941178038


##########
arrow/src/datatypes/field.rs:
##########
@@ -698,41 +684,25 @@ impl Field {
     /// * self.metadata is a superset of other.metadata
     /// * all other fields are equal

Review Comment:
   I am not sure either -- can you possibly add this as a test (so that at least the behavior is documented and if we change it in the future we will also have to change the test)?



##########
arrow/src/datatypes/field.rs:
##########
@@ -840,4 +810,55 @@ mod test {
         assert_ne!(dict1, dict2);
         assert_ne!(get_field_hash(&dict1), get_field_hash(&dict2));
     }
+
+    #[test]
+    fn test_contains_reflexivity() {
+        let mut field = Field::new("field1", DataType::Float16, false);
+        field.set_metadata(Some(BTreeMap::from([
+            (String::from("k0"), String::from("v0")),
+            (String::from("k1"), String::from("v1")),
+        ])));
+        assert!(field.contains(&field))
+    }
+
+    #[test]
+    fn test_contains_transitivity() {
+        let child_field = Field::new("child1", DataType::Float16, false);
+
+        let mut field1 = Field::new("field1", DataType::Struct(vec![child_field]), false);
+        field1.set_metadata(Some(BTreeMap::from([(
+            String::from("k1"),
+            String::from("v1"),
+        )])));
+
+        let mut field2 = Field::new("field1", DataType::Struct(vec![]), true);
+        field2.set_metadata(Some(BTreeMap::from([(
+            String::from("k2"),
+            String::from("v2"),
+        )])));
+        field2.try_merge(&field1).unwrap();
+
+        let mut field3 = Field::new("field1", DataType::Struct(vec![]), false);
+        field3.set_metadata(Some(BTreeMap::from([(
+            String::from("k3"),
+            String::from("v3"),
+        )])));
+        field3.try_merge(&field2).unwrap();
+
+        assert!(field2.contains(&field1));
+        assert!(field3.contains(&field2));
+        assert!(field3.contains(&field1));
+
+        assert!(!field1.contains(&field2));
+        assert!(!field1.contains(&field3));
+        assert!(!field2.contains(&field3));
+    }
+
+    #[test]
+    fn test_contains_nullable() {

Review Comment:
   Good check



##########
arrow/src/datatypes/field.rs:
##########
@@ -572,31 +575,16 @@ impl Field {
             }
             _ => {}
         }
-        if from.dict_id != self.dict_id {
-            return Err(ArrowError::SchemaError(
-                "Fail to merge schema Field due to conflicting dict_id".to_string(),
-            ));
-        }
-        if from.dict_is_ordered != self.dict_is_ordered {
-            return Err(ArrowError::SchemaError(
-                "Fail to merge schema Field due to conflicting dict_is_ordered"
-                    .to_string(),
-            ));
-        }
         match &mut self.data_type {
             DataType::Struct(nested_fields) => match &from.data_type {
                 DataType::Struct(from_nested_fields) => {
                     for from_field in from_nested_fields {
-                        let mut is_new_field = true;
-                        for self_field in nested_fields.iter_mut() {
-                            if self_field.name != from_field.name {
-                                continue;
-                            }
-                            is_new_field = false;
-                            self_field.try_merge(from_field)?;
-                        }
-                        if is_new_field {
-                            nested_fields.push(from_field.clone());
+                        match nested_fields

Review Comment:
   I agree this is easier to read 👍 



-- 
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: github-unsubscribe@arrow.apache.org

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