Logically compare objects, remove custom partial eq impl for metadata

Author: friendlymatthew

parquet-variant/src/variant/metadata.rs

@@ -15,8 +15,6 @@
 // specific language governing permissions and limitations
 // under the License.
 
-use std::collections::HashSet;
-
 use crate::decoder::{map_bytes_to_offsets, OffsetSizeBytes};
 use crate::utils::{first_byte_from_slice, overflow_error, slice_from_slice, string_from_slice};
 
@@ -127,7 +125,7 @@ impl VariantMetadataHeader {
 ///
 /// [`Variant`]: crate::Variant
 /// [Variant Spec]: https://github.com/apache/parquet-format/blob/master/VariantEncoding.md#metadata-encoding
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq)]
 pub struct VariantMetadata<'m> {
     pub(crate) bytes: &'m [u8],
     header: VariantMetadataHeader,
@@ -335,23 +333,6 @@ impl<'m> VariantMetadata<'m> {
     }
 }
 
-// Metadata dictionaries can be in any order per the spec to allow flexible Variant construction
-// This comparison checks for field name presence in both metadata instances rather than raw byte equality
-impl<'m> PartialEq for VariantMetadata<'m> {
-    fn eq(&self, other: &Self) -> bool {
-        self.is_empty() == other.is_empty()
-            && if self.is_sorted() && other.is_sorted() {
-                self.len() == other.len() && self.iter().eq(other.iter())
-            } else {
-                // can't guarantee neither field names are unique
-                let self_field_names: HashSet<&'m str> = HashSet::from_iter(self.iter());
-                let other_field_names: HashSet<&'m str> = HashSet::from_iter(other.iter());
-
-                self_field_names == other_field_names
-            }
-    }
-}
-
 /// Retrieves the ith dictionary entry, panicking if the index is out of bounds. Accessing
 /// [unvalidated] input could also panic if the underlying bytes are invalid.
 ///
@@ -586,42 +567,7 @@ mod tests {
         let m2 = VariantMetadata::try_new(&metadata_bytes).unwrap();
         assert!(!m2.is_sorted());
 
-        assert_eq!(m1, m2);
-    }
-
-    #[test]
-    fn test_compare_sorted_dictionary_with_unsorted_dictionary2() {
-        // create a sorted object
-        let mut b = VariantBuilder::new();
-        let mut o = b.new_object();
-
-        o.insert("a", false);
-        o.insert("b", false);
-
-        o.finish().unwrap();
-
-        let (m, _) = b.finish();
-
-        let m1 = VariantMetadata::new(&m);
-        assert!(m1.is_sorted());
-
-        // Create metadata with an unsorted dictionary (field names are "b", "b", "a")
-        // Since field names are not unique nor lexicographically ordered, it is considered not sorted.
-        let metadata_bytes = vec![
-            0b0000_0001,
-            3, // dictionary size
-            0, // "b"
-            1, // "b"
-            2, // "a"
-            3,
-            b'b',
-            b'b',
-            b'a',
-        ];
-        let m2 = VariantMetadata::try_new(&metadata_bytes).unwrap();
-        assert!(!m2.is_sorted());
-
-        assert_eq!(m1, m2);
+        assert_ne!(m1, m2);
     }
 
     #[test]
@@ -642,35 +588,4 @@ mod tests {
 
         assert_eq!(m1, m2);
     }
-
-    #[test]
-    fn test_compare_sorted_dictionary_with_sorted_dictionary2() {
-        // create a sorted object
-        let mut b = VariantBuilder::new();
-        let mut o = b.new_object();
-
-        o.insert("a", false);
-        o.insert("b", false);
-
-        o.finish().unwrap();
-
-        let (m, _) = b.finish();
-
-        let m1 = VariantMetadata::new(&m);
-
-        // create a sorted object with different field names
-        let mut b = VariantBuilder::new();
-        let mut o = b.new_object();
-
-        o.insert("c", false);
-        o.insert("d", false);
-
-        o.finish().unwrap();
-
-        let (m, _) = b.finish();
-
-        let m2 = VariantMetadata::new(&m);
-
-        assert_ne!(m1, m2);
-    }
 }

parquet-variant/src/variant/object.rs

@@ -413,13 +413,8 @@ impl<'m, 'v> VariantObject<'m, 'v> {
 // checks whether the field values are equal -- regardless of their order
 impl<'m, 'v> PartialEq for VariantObject<'m, 'v> {
     fn eq(&self, other: &Self) -> bool {
-        let mut is_equal = self.metadata == other.metadata
-            && self.header == other.header
-            && self.num_elements == other.num_elements
-            && self.first_field_offset_byte == other.first_field_offset_byte
-            && self.first_value_byte == other.first_value_byte;
+        let mut is_equal = self.num_elements == other.num_elements;
 
-        // value validation
         let other_fields: HashMap<&str, Variant> = HashMap::from_iter(other.iter());
 
         for (field_name, variant) in self.iter() {
@@ -938,27 +933,30 @@ mod tests {
 
         o.finish().unwrap();
 
-        let (m, v) = b.finish();
+        let (meta1, value1) = b.finish();
 
-        let v1 = Variant::try_new(&m, &v).unwrap();
+        let v1 = Variant::try_new(&meta1, &value1).unwrap();
         // v1 is sorted
         assert!(v1.metadata().unwrap().is_sorted());
 
         // create a second object with different insertion order
-        let mut b = VariantBuilder::new();
+        let mut b = VariantBuilder::new().with_field_names(["d", "c", "b", "a"].into_iter());
         let mut o = b.new_object();
 
         o.insert("b", 4.3);
         o.insert("a", ());
 
         o.finish().unwrap();
 
-        let (m, v) = b.finish();
+        let (meta2, value2) = b.finish();
 
-        let v2 = Variant::try_new(&m, &v).unwrap();
+        let v2 = Variant::try_new(&meta2, &value2).unwrap();
         // v2 is not sorted
         assert!(!v2.metadata().unwrap().is_sorted());
 
+        // object metadata are not the same
+        assert_ne!(v1.metadata(), v2.metadata());
+
         // objects are still logically equal
         assert_eq!(v1, v2);
     }
@@ -976,10 +974,6 @@ mod tests {
 
         let (m, v) = b.finish();
 
-        let variant_metadata = VariantMetadata::new(&m);
-
-        dbg!(variant_metadata.iter().collect::<Vec<_>>());
-
         let v1 = Variant::try_new(&m, &v).unwrap();
 
         // Create metadata with an unsorted dictionary (field names are "a", "a", "b")
@@ -998,12 +992,7 @@ mod tests {
         let m = VariantMetadata::try_new(&metadata_bytes).unwrap();
         assert!(!m.is_sorted());
 
-        dbg!(m.iter().collect::<Vec<_>>());
         let v2 = Variant::new_with_metadata(m, &v);
-
-        dbg!(v1.as_object().unwrap().iter().collect::<Vec<_>>());
-        dbg!(v2.as_object().unwrap().iter().collect::<Vec<_>>());
-
         assert_eq!(v1, v2);
     }
 }