alamb commented on code in PR #7961:
URL: https://github.com/apache/arrow-rs/pull/7961#discussion_r2219508486
##########
parquet-variant/src/variant/metadata.rs:
##########
@@ -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)]
Review Comment:
👍
##########
parquet-variant/src/variant/object.rs:
##########
@@ -412,14 +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
- && self.validated == other.validated;
-
- // value validation
+ let mut is_equal = self.num_elements == other.num_elements;
Review Comment:
nit: you can return immediately here to short circut the comparsion if the
number of elements are not the same
Something like
```suggestion
if self.num_elements != other.num_elements {
return false;
}
```
##########
parquet-variant/src/variant/object.rs:
##########
@@ -938,28 +933,66 @@ 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
Review Comment:
👍
##########
parquet-variant/src/variant/object.rs:
##########
@@ -412,14 +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
- && self.validated == other.validated;
-
- // value validation
+ let mut is_equal = self.num_elements == other.num_elements;
Review Comment:
Also, the same comment applies below -- basically as soon as you discover
there is a field that doesn't match you can immediately return false rather
than continue to check the other fields
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
