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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
