You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "friendlymatthew (via GitHub)" <gi...@apache.org> on 2025/07/16 20:34:35 UTC
[PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
friendlymatthew opened a new pull request, #7943:
URL: https://github.com/apache/arrow-rs/pull/7943
# Rationale for this change
This PR introduces a custom implementation of `PartialEq` for variant objects.
According to the spec, field values are not required to be in the same order as the field IDs, to enable flexibility when constructing Variant values.
Instead of comparing the raw bytes of 2 variant objects, this implementation recursively checks whether the field values are equal -- regardless of their order
--
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
Re: [PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7943:
URL: https://github.com/apache/arrow-rs/pull/7943#discussion_r2211598158
##########
parquet-variant/src/variant/object.rs:
##########
@@ -387,6 +389,31 @@ impl<'m, 'v> VariantObject<'m, 'v> {
}
}
+impl<'m, 'v> PartialEq for VariantObject<'m, 'v> {
Review Comment:
Can you please add some comments explaining the rationale for why this is a custom derivation for future readers?
Something ilke "Need custom partialEq implementation as field_ids may differ if metadata is different"
##########
parquet-variant/src/variant/object.rs:
##########
@@ -387,6 +389,31 @@ impl<'m, 'v> VariantObject<'m, 'v> {
}
}
+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
Review Comment:
👍
We can probably add a fast path for the case when the metadata is actually the same and then we can just compare the field ids
But that could also be done in a different 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #7943:
URL: https://github.com/apache/arrow-rs/pull/7943#discussion_r2216451926
##########
parquet-variant/src/variant/object.rs:
##########
@@ -387,6 +389,31 @@ impl<'m, 'v> VariantObject<'m, 'v> {
}
}
+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
Review Comment:
I'm not sure I understand why the above example is wrong tho? The metadata dictionary being the same says very little about any two objects that happen to use it?
--
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
Re: [PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on PR #7943:
URL: https://github.com/apache/arrow-rs/pull/7943#issuecomment-3083213674
Unrelated but these public methods on `VariantMetadata` are redundant:
https://github.com/apache/arrow-rs/blob/03a837e883323ef7e3294f0805c9e1cadd3963b8/parquet-variant/src/variant/metadata.rs#L210-L213
https://github.com/apache/arrow-rs/blob/03a837e883323ef7e3294f0805c9e1cadd3963b8/parquet-variant/src/variant/metadata.rs#L288-L291
--
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
Re: [PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on PR #7943:
URL: https://github.com/apache/arrow-rs/pull/7943#issuecomment-3080624452
I'm going to do a full pass and write some comments for the new methods I introduced tomorrow
--
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
Re: [PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on PR #7943:
URL: https://github.com/apache/arrow-rs/pull/7943#issuecomment-3083056825
> Like show creating two objects with the same fields but differnetly populated metadata are still equal
Hm, I see what you mean. I guess whether or not the metadata is sorted shouldn't matter when doing a logical comparison. i.e. field ids ordering do not matter
--
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
Re: [PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #7943:
URL: https://github.com/apache/arrow-rs/pull/7943#discussion_r2214222666
##########
parquet-variant/src/variant/metadata.rs:
##########
@@ -332,6 +334,30 @@ impl<'m> VariantMetadata<'m> {
}
}
+// According to the spec, metadata dictionaries are not required to be in a specific order,
+// to enable flexibility when constructing Variant values
+//
+// Instead of comparing the raw bytes of 2 variant metadata instances, this implementation
+// checks whether the dictionary entries are equal -- regardless of their sorting order
+impl<'m> PartialEq for VariantMetadata<'m> {
+ fn eq(&self, other: &Self) -> bool {
Review Comment:
Given the cost of constructing hash maps etc, is it worth adding the following quick-check in case the dictionaries are both sorted?
```rust
if self.is_ordered() && other.is_ordered() {
if self.len() != other.len() {
return false;
}
let self_value_bytes = ... all string value bytes ...;
let other_value_bytes = ... all string value bytes ...;
return self_value_bytes == other_value_bytes;
}
```
Before trusting a single long string compare tho, we would need to convince ourselves that there's no way two dictionaries with different offsets can have identical value bytes. Otherwise, we'd have to loop over the two sets of strings manually.
--
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
Re: [PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on code in PR #7943:
URL: https://github.com/apache/arrow-rs/pull/7943#discussion_r2215455919
##########
parquet-variant/src/variant/metadata.rs:
##########
@@ -332,6 +334,30 @@ impl<'m> VariantMetadata<'m> {
}
}
+// According to the spec, metadata dictionaries are not required to be in a specific order,
+// to enable flexibility when constructing Variant values
+//
+// Instead of comparing the raw bytes of 2 variant metadata instances, this implementation
+// checks whether the dictionary entries are equal -- regardless of their sorting order
+impl<'m> PartialEq for VariantMetadata<'m> {
+ fn eq(&self, other: &Self) -> bool {
Review Comment:
Very nice. I will think about this and push up a 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on code in PR #7943:
URL: https://github.com/apache/arrow-rs/pull/7943#discussion_r2211549323
##########
parquet-variant/src/builder.rs:
##########
@@ -252,7 +304,28 @@ impl ValueBuffer {
variant: Variant<'m, 'd>,
metadata_builder: &mut MetadataBuilder,
) {
- self.try_append_variant(variant, metadata_builder).unwrap();
+ match variant {
+ Variant::Null => self.append_null(),
+ Variant::BooleanTrue => self.append_bool(true),
+ Variant::BooleanFalse => self.append_bool(false),
+ Variant::Int8(v) => self.append_int8(v),
+ Variant::Int16(v) => self.append_int16(v),
+ Variant::Int32(v) => self.append_int32(v),
+ Variant::Int64(v) => self.append_int64(v),
+ Variant::Date(v) => self.append_date(v),
+ Variant::TimestampMicros(v) => self.append_timestamp_micros(v),
+ Variant::TimestampNtzMicros(v) => self.append_timestamp_ntz_micros(v),
+ Variant::Decimal4(decimal4) => self.append_decimal4(decimal4),
+ Variant::Decimal8(decimal8) => self.append_decimal8(decimal8),
+ Variant::Decimal16(decimal16) => self.append_decimal16(decimal16),
+ Variant::Float(v) => self.append_float(v),
+ Variant::Double(v) => self.append_double(v),
+ Variant::Binary(v) => self.append_binary(v),
+ Variant::String(s) => self.append_string(s),
+ Variant::ShortString(s) => self.append_short_string(s),
+ Variant::Object(obj) => self.append_object(metadata_builder, obj),
+ Variant::List(list) => self.append_list(metadata_builder, list),
+ }
Review Comment:
It is unfortunate to have to copy the same match statement as `try_append_variant`, but `ValueBuffer::append_object` and `ValueBuffer::try_append_object` call different iterators and append methods
--
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
Re: [PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7943:
URL: https://github.com/apache/arrow-rs/pull/7943#discussion_r2216008690
##########
parquet-variant/src/variant/metadata.rs:
##########
@@ -332,6 +334,30 @@ impl<'m> VariantMetadata<'m> {
}
}
+// According to the spec, metadata dictionaries are not required to be in a specific order,
+// to enable flexibility when constructing Variant values
+//
+// Instead of comparing the raw bytes of 2 variant metadata instances, this implementation
+// checks whether the dictionary entries are equal -- regardless of their sorting order
+impl<'m> PartialEq for VariantMetadata<'m> {
+ fn eq(&self, other: &Self) -> bool {
+ let is_equal = self.is_empty() == other.is_empty()
+ && self.is_fully_validated() == other.is_fully_validated()
+ && self.first_value_byte == other.first_value_byte
+ && self.validated == other.validated;
+
+ let other_field_names: HashSet<&'m str> = HashSet::from_iter(other.iter());
+
+ for field_name in self.iter() {
+ if !other_field_names.contains(field_name) {
+ return false;
Review Comment:
Yes. You are right. I think @friendlymatthew has a fix for it in
- https://github.com/apache/arrow-rs/pull/7961
##########
parquet-variant/src/variant/metadata.rs:
##########
@@ -332,6 +334,30 @@ impl<'m> VariantMetadata<'m> {
}
}
+// According to the spec, metadata dictionaries are not required to be in a specific order,
+// to enable flexibility when constructing Variant values
+//
+// Instead of comparing the raw bytes of 2 variant metadata instances, this implementation
+// checks whether the dictionary entries are equal -- regardless of their sorting order
+impl<'m> PartialEq for VariantMetadata<'m> {
+ fn eq(&self, other: &Self) -> bool {
Review Comment:
Follow up: https://github.com/apache/arrow-rs/pull/7961/files
--
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
Re: [PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7943:
URL: https://github.com/apache/arrow-rs/pull/7943#discussion_r2213275443
##########
parquet-variant/src/variant/metadata.rs:
##########
@@ -332,6 +334,30 @@ impl<'m> VariantMetadata<'m> {
}
}
+// According to the spec, metadata dictionaries are not required to be in a specific order,
+// to enable flexibility when constructing Variant values
+//
+// Instead of comparing the raw bytes of 2 variant metadata instances, this implementation
+// checks whether the dictionary entries are equal -- regardless of their sorting order
+impl<'m> PartialEq for VariantMetadata<'m> {
+ fn eq(&self, other: &Self) -> bool {
+ let is_equal = self.is_empty() == other.is_empty()
+ && self.is_fully_validated() == other.is_fully_validated()
+ && self.first_value_byte == other.first_value_byte
+ && self.validated == other.validated;
Review Comment:
You could break early here if is_equal is false and not check the fields
##########
parquet-variant/src/variant/object.rs:
##########
@@ -387,6 +389,38 @@ impl<'m, 'v> VariantObject<'m, 'v> {
}
}
+// Custom implementation of PartialEq for variant objects
+//
+// According to the spec, field values are not required to be in the same order as the field IDs,
+// to enable flexibility when constructing Variant values
+//
+// Instead of comparing the raw bytes of 2 variant objects, this implementation recursively
+// 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;
Review Comment:
same comment here about validation
##########
parquet-variant/src/builder.rs:
##########
@@ -252,7 +304,28 @@ impl ValueBuffer {
variant: Variant<'m, 'd>,
metadata_builder: &mut MetadataBuilder,
) {
- self.try_append_variant(variant, metadata_builder).unwrap();
+ match variant {
+ Variant::Null => self.append_null(),
+ Variant::BooleanTrue => self.append_bool(true),
+ Variant::BooleanFalse => self.append_bool(false),
+ Variant::Int8(v) => self.append_int8(v),
+ Variant::Int16(v) => self.append_int16(v),
+ Variant::Int32(v) => self.append_int32(v),
+ Variant::Int64(v) => self.append_int64(v),
+ Variant::Date(v) => self.append_date(v),
+ Variant::TimestampMicros(v) => self.append_timestamp_micros(v),
+ Variant::TimestampNtzMicros(v) => self.append_timestamp_ntz_micros(v),
+ Variant::Decimal4(decimal4) => self.append_decimal4(decimal4),
+ Variant::Decimal8(decimal8) => self.append_decimal8(decimal8),
+ Variant::Decimal16(decimal16) => self.append_decimal16(decimal16),
+ Variant::Float(v) => self.append_float(v),
+ Variant::Double(v) => self.append_double(v),
+ Variant::Binary(v) => self.append_binary(v),
+ Variant::String(s) => self.append_string(s),
+ Variant::ShortString(s) => self.append_short_string(s),
+ Variant::Object(obj) => self.append_object(metadata_builder, obj),
+ Variant::List(list) => self.append_list(metadata_builder, list),
+ }
Review Comment:
I see -- I think it is the unfortunate consequence of having the distinction between the "validated" and "non validated" variant APIs
Maybe we can add a few comments explaining the practical difference (performance vs extra error checking)
##########
parquet-variant/src/variant/metadata.rs:
##########
@@ -332,6 +334,22 @@ impl<'m> VariantMetadata<'m> {
}
}
+impl<'m> PartialEq for VariantMetadata<'m> {
+ fn eq(&self, other: &Self) -> bool {
+ let mut is_equal = self.is_empty() == other.is_empty()
+ && self.is_fully_validated() == other.is_fully_validated()
Review Comment:
I would expect the following to pass for all variants and metadata
```rust
let variant1 = Variant::new(metadata, buffers);
let variant2 = Variant::new(metadata, buffers).with_full_validation();
assert_eq!(variant1, variant2)
```
##########
parquet-variant/src/variant/object.rs:
##########
@@ -718,4 +752,187 @@ mod tests {
test_variant_object_with_large_data(16777216 + 1, OffsetSizeBytes::Four);
// 2^24
}
+
+ #[test]
+ fn test_objects_with_same_fields_are_equal() {
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("b", ());
+ o.insert("c", ());
+ o.insert("a", ());
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v1 = Variant::try_new(&m, &v).unwrap();
+ let v2 = Variant::try_new(&m, &v).unwrap();
+
+ assert_eq!(v1, v2);
+ }
+
+ #[test]
+ fn test_same_objects_with_different_builder_are_equal() {
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ o.insert("b", false);
+
+ o.finish().unwrap();
+ let (m, v) = b.finish();
+
+ let v1 = Variant::try_new(&m, &v).unwrap();
+
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ o.insert("b", false);
+
+ o.finish().unwrap();
+ let (m, v) = b.finish();
+
+ let v2 = Variant::try_new(&m, &v).unwrap();
+
+ assert_eq!(v1, v2);
+ }
+
+ #[test]
+ fn test_objects_with_different_values_are_not_equal() {
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ o.insert("b", 4.3);
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v1 = Variant::try_new(&m, &v).unwrap();
+
+ // second object, same field name but different values
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ let mut inner_o = o.new_object("b");
+ inner_o.insert("a", 3.3);
+ inner_o.finish().unwrap();
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v2 = Variant::try_new(&m, &v).unwrap();
+
+ let m1 = v1.metadata().unwrap();
+ let m2 = v2.metadata().unwrap();
+
+ // metadata would be equal since they contain the same keys
+ assert_eq!(m1, m2);
+
+ // but the objects are not equal
+ assert_ne!(v1, v2);
+ }
+
+ #[test]
+ fn test_objects_with_different_field_names_are_not_equal() {
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ o.insert("b", 4.3);
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v1 = Variant::try_new(&m, &v).unwrap();
+
+ // second object, same field name but different values
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("aardvark", ());
+ o.insert("barracuda", 3.3);
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+ let v2 = Variant::try_new(&m, &v).unwrap();
+
+ assert_ne!(v1, v2);
+ }
+
+ #[test]
+ fn test_objects_with_different_insertion_order_are_equal() {
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("b", false);
+ o.insert("a", ());
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v1 = Variant::try_new(&m, &v).unwrap();
+ assert!(!v1.metadata().unwrap().is_sorted());
+
+ // create another object pre-filled with field names, b and a
+ // but insert the fields in the order of a, b
+ let mut b = VariantBuilder::new().with_field_names(["b", "a"].into_iter());
Review Comment:
👍
##########
parquet-variant/src/builder.rs:
##########
@@ -216,6 +217,57 @@ impl ValueBuffer {
self.append_slice(value.as_bytes());
}
+ fn append_object(&mut self, metadata_builder: &mut MetadataBuilder, obj: VariantObject) {
+ let mut object_builder = self.new_object(metadata_builder);
+
+ for (field_name, value) in obj.iter() {
+ object_builder.insert(field_name, value);
+ }
+
+ object_builder.finish().unwrap();
+ }
+
+ fn try_append_object(
+ &mut self,
+ metadata_builder: &mut MetadataBuilder,
+ obj: VariantObject,
+ ) -> Result<(), ArrowError> {
+ let mut object_builder = self.new_object(metadata_builder);
+
+ for res in obj.iter_try() {
Review Comment:
You might be able to consolidate the apis if you checked the `is_validated` dynamically
For example, have a single try_append_object and internally try
```rust
/// if source variant is already validated, use faster APIs
if obj.is_validated() {
for (field_name, value) in obj.iter() {
object_builder.insert(field_name, value);
}
} else {
for res in obj.iter_try() {
let (field_name, value) = res?;
object_builder.try_insert(field_name, value)?;
}
}
```
##########
parquet-variant/src/variant/metadata.rs:
##########
@@ -332,6 +334,30 @@ impl<'m> VariantMetadata<'m> {
}
}
+// According to the spec, metadata dictionaries are not required to be in a specific order,
Review Comment:
Do we need a special is_equal check for VariantMetadata? It seems like now since VariantObject doesn't include a check for the metadata being equal we could avoid a special equality 🤔
##########
parquet-variant/src/builder.rs:
##########
@@ -252,7 +304,28 @@ impl ValueBuffer {
variant: Variant<'m, 'd>,
metadata_builder: &mut MetadataBuilder,
) {
- self.try_append_variant(variant, metadata_builder).unwrap();
+ match variant {
+ Variant::Null => self.append_null(),
+ Variant::BooleanTrue => self.append_bool(true),
+ Variant::BooleanFalse => self.append_bool(false),
+ Variant::Int8(v) => self.append_int8(v),
+ Variant::Int16(v) => self.append_int16(v),
+ Variant::Int32(v) => self.append_int32(v),
+ Variant::Int64(v) => self.append_int64(v),
+ Variant::Date(v) => self.append_date(v),
+ Variant::TimestampMicros(v) => self.append_timestamp_micros(v),
+ Variant::TimestampNtzMicros(v) => self.append_timestamp_ntz_micros(v),
+ Variant::Decimal4(decimal4) => self.append_decimal4(decimal4),
+ Variant::Decimal8(decimal8) => self.append_decimal8(decimal8),
+ Variant::Decimal16(decimal16) => self.append_decimal16(decimal16),
+ Variant::Float(v) => self.append_float(v),
+ Variant::Double(v) => self.append_double(v),
+ Variant::Binary(v) => self.append_binary(v),
+ Variant::String(s) => self.append_string(s),
+ Variant::ShortString(s) => self.append_short_string(s),
+ Variant::Object(obj) => self.append_object(metadata_builder, obj),
+ Variant::List(list) => self.append_list(metadata_builder, list),
+ }
Review Comment:
I also left a suggestion above about how to potentially unify the code paths
##########
parquet-variant/src/variant/object.rs:
##########
@@ -718,4 +752,187 @@ mod tests {
test_variant_object_with_large_data(16777216 + 1, OffsetSizeBytes::Four);
// 2^24
}
+
+ #[test]
+ fn test_objects_with_same_fields_are_equal() {
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("b", ());
+ o.insert("c", ());
+ o.insert("a", ());
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v1 = Variant::try_new(&m, &v).unwrap();
+ let v2 = Variant::try_new(&m, &v).unwrap();
+
+ assert_eq!(v1, v2);
+ }
+
+ #[test]
+ fn test_same_objects_with_different_builder_are_equal() {
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ o.insert("b", false);
+
+ o.finish().unwrap();
+ let (m, v) = b.finish();
+
+ let v1 = Variant::try_new(&m, &v).unwrap();
+
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ o.insert("b", false);
+
+ o.finish().unwrap();
+ let (m, v) = b.finish();
+
+ let v2 = Variant::try_new(&m, &v).unwrap();
+
+ assert_eq!(v1, v2);
+ }
+
+ #[test]
+ fn test_objects_with_different_values_are_not_equal() {
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ o.insert("b", 4.3);
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v1 = Variant::try_new(&m, &v).unwrap();
+
+ // second object, same field name but different values
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ let mut inner_o = o.new_object("b");
+ inner_o.insert("a", 3.3);
+ inner_o.finish().unwrap();
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v2 = Variant::try_new(&m, &v).unwrap();
+
+ let m1 = v1.metadata().unwrap();
+ let m2 = v2.metadata().unwrap();
+
+ // metadata would be equal since they contain the same keys
+ assert_eq!(m1, m2);
+
+ // but the objects are not equal
+ assert_ne!(v1, v2);
+ }
+
+ #[test]
+ fn test_objects_with_different_field_names_are_not_equal() {
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ o.insert("b", 4.3);
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v1 = Variant::try_new(&m, &v).unwrap();
+
+ // second object, same field name but different values
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("aardvark", ());
+ o.insert("barracuda", 3.3);
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+ let v2 = Variant::try_new(&m, &v).unwrap();
+
+ assert_ne!(v1, v2);
+ }
+
+ #[test]
+ fn test_objects_with_different_insertion_order_are_equal() {
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("b", false);
+ o.insert("a", ());
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v1 = Variant::try_new(&m, &v).unwrap();
+ assert!(!v1.metadata().unwrap().is_sorted());
+
+ // create another object pre-filled with field names, b and a
+ // but insert the fields in the order of a, b
+ let mut b = VariantBuilder::new().with_field_names(["b", "a"].into_iter());
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ o.insert("b", false);
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v2 = Variant::try_new(&m, &v).unwrap();
+
+ // v2 should also have a unsorted dictionary
+ assert!(!v2.metadata().unwrap().is_sorted());
+
+ assert_eq!(v1, v2);
+ }
+
+ #[test]
+ fn test_objects_with_differing_metadata_are_equal() {
Review Comment:
For a follow on work, I think we should
##########
parquet-variant/src/variant/object.rs:
##########
@@ -718,4 +752,187 @@ mod tests {
test_variant_object_with_large_data(16777216 + 1, OffsetSizeBytes::Four);
// 2^24
}
+
+ #[test]
+ fn test_objects_with_same_fields_are_equal() {
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("b", ());
+ o.insert("c", ());
+ o.insert("a", ());
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v1 = Variant::try_new(&m, &v).unwrap();
+ let v2 = Variant::try_new(&m, &v).unwrap();
+
+ assert_eq!(v1, v2);
+ }
+
+ #[test]
+ fn test_same_objects_with_different_builder_are_equal() {
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ o.insert("b", false);
+
+ o.finish().unwrap();
+ let (m, v) = b.finish();
+
+ let v1 = Variant::try_new(&m, &v).unwrap();
+
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ o.insert("b", false);
+
+ o.finish().unwrap();
+ let (m, v) = b.finish();
+
+ let v2 = Variant::try_new(&m, &v).unwrap();
+
+ assert_eq!(v1, v2);
+ }
+
+ #[test]
+ fn test_objects_with_different_values_are_not_equal() {
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ o.insert("b", 4.3);
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v1 = Variant::try_new(&m, &v).unwrap();
+
+ // second object, same field name but different values
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ let mut inner_o = o.new_object("b");
+ inner_o.insert("a", 3.3);
+ inner_o.finish().unwrap();
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v2 = Variant::try_new(&m, &v).unwrap();
+
+ let m1 = v1.metadata().unwrap();
+ let m2 = v2.metadata().unwrap();
+
+ // metadata would be equal since they contain the same keys
+ assert_eq!(m1, m2);
+
+ // but the objects are not equal
+ assert_ne!(v1, v2);
+ }
+
+ #[test]
+ fn test_objects_with_different_field_names_are_not_equal() {
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ o.insert("b", 4.3);
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v1 = Variant::try_new(&m, &v).unwrap();
+
+ // second object, same field name but different values
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("aardvark", ());
+ o.insert("barracuda", 3.3);
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+ let v2 = Variant::try_new(&m, &v).unwrap();
+
+ assert_ne!(v1, v2);
+ }
+
+ #[test]
+ fn test_objects_with_different_insertion_order_are_equal() {
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("b", false);
+ o.insert("a", ());
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v1 = Variant::try_new(&m, &v).unwrap();
+ assert!(!v1.metadata().unwrap().is_sorted());
+
+ // create another object pre-filled with field names, b and a
+ // but insert the fields in the order of a, b
+ let mut b = VariantBuilder::new().with_field_names(["b", "a"].into_iter());
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ o.insert("b", false);
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v2 = Variant::try_new(&m, &v).unwrap();
+
+ // v2 should also have a unsorted dictionary
+ assert!(!v2.metadata().unwrap().is_sorted());
+
+ assert_eq!(v1, v2);
+ }
+
+ #[test]
+ fn test_objects_with_differing_metadata_are_equal() {
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ o.insert("b", 4.3);
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v1 = Variant::try_new(&m, &v).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 o = b.new_object();
+
+ o.insert("b", 4.3);
+ o.insert("a", ());
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v2 = Variant::try_new(&m, &v).unwrap();
+ // v2 is not sorted
+ assert!(!v2.metadata().unwrap().is_sorted());
+
+ // objects are still logically equal
+ assert_eq!(v1, v2);
+ }
Review Comment:
I think we should also add some other tests eventually too -- like lists and primitives
##########
parquet-variant/src/variant/metadata.rs:
##########
@@ -332,6 +334,22 @@ impl<'m> VariantMetadata<'m> {
}
}
+impl<'m> PartialEq for VariantMetadata<'m> {
+ fn eq(&self, other: &Self) -> bool {
+ let mut is_equal = self.is_empty() == other.is_empty()
+ && self.is_fully_validated() == other.is_fully_validated()
Review Comment:
It seems like the `validated` and `is_fully_validated` flags doesn't need to be part of a logical type check? Like two variants can be equal by value even if one is fully validated and one is not
--
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
Re: [PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on code in PR #7943:
URL: https://github.com/apache/arrow-rs/pull/7943#discussion_r2212607373
##########
parquet-variant/src/variant/object.rs:
##########
@@ -387,6 +389,31 @@ impl<'m, 'v> VariantObject<'m, 'v> {
}
}
+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
Review Comment:
Hm, I am not sure if we can compare by field names.
Here's a test case where two variants have the same field names but differing values. The two objects will have the same metadata, but it will fail the logical comparison.
```rs
fn foo() {
let mut b = VariantBuilder::new();
let mut o = b.new_object();
o.insert("a", ());
o.insert("b", 4.3);
o.finish().unwrap();
let (m, v) = b.finish();
let v1 = Variant::try_new(&m, &v).unwrap();
// second object, same field name but different values
let mut b = VariantBuilder::new();
let mut o = b.new_object();
o.insert("a", ());
let mut inner_o = o.new_object("b");
inner_o.insert("a", 3.3);
inner_o.finish().unwrap();
o.finish().unwrap();
let (m, v) = b.finish();
let v2 = Variant::try_new(&m, &v).unwrap();
let m1 = v1.metadata().unwrap();
let m2 = v2.metadata().unwrap();
// metadata would be equal since they contain the same keys
assert_eq!(m1, m2);
// but the objects are not equal
assert_ne!(v1, v2);
}
```
--
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
Re: [PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #7943:
URL: https://github.com/apache/arrow-rs/pull/7943#discussion_r2214214548
##########
parquet-variant/src/variant/metadata.rs:
##########
@@ -332,6 +334,30 @@ impl<'m> VariantMetadata<'m> {
}
}
+// According to the spec, metadata dictionaries are not required to be in a specific order,
+// to enable flexibility when constructing Variant values
+//
+// Instead of comparing the raw bytes of 2 variant metadata instances, this implementation
+// checks whether the dictionary entries are equal -- regardless of their sorting order
+impl<'m> PartialEq for VariantMetadata<'m> {
+ fn eq(&self, other: &Self) -> bool {
+ let is_equal = self.is_empty() == other.is_empty()
+ && self.is_fully_validated() == other.is_fully_validated()
+ && self.first_value_byte == other.first_value_byte
+ && self.validated == other.validated;
+
+ let other_field_names: HashSet<&'m str> = HashSet::from_iter(other.iter());
+
+ for field_name in self.iter() {
+ if !other_field_names.contains(field_name) {
+ return false;
Review Comment:
This seems one-sided? Don't we need to prove the symmetric difference is empty?
--
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
Re: [PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #7943:
URL: https://github.com/apache/arrow-rs/pull/7943#discussion_r2216453780
##########
parquet-variant/src/variant/object.rs:
##########
@@ -387,6 +389,38 @@ impl<'m, 'v> VariantObject<'m, 'v> {
}
}
+// Custom implementation of PartialEq for variant objects
+//
+// According to the spec, field values are not required to be in the same order as the field IDs,
+// to enable flexibility when constructing Variant values
+//
+// Instead of comparing the raw bytes of 2 variant objects, this implementation recursively
+// 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 other_fields: HashMap<&str, Variant> = HashMap::from_iter(other.iter());
+
+ for (field_name, variant) in self.iter() {
+ match other_fields.get(field_name as &str) {
+ Some(other_variant) => {
+ is_equal = is_equal && variant == *other_variant;
+ }
+ None => return false,
+ }
Review Comment:
post-hoc nit: we should short circuit
```rust
if other_fields.get(field_name as &str).is_none_or(|other| variant != *other) {
return false;
}
```
... but the check is actually incomplete because it fails to prove the symmetric difference is empty.
--
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
Re: [PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #7943:
URL: https://github.com/apache/arrow-rs/pull/7943#discussion_r2216449041
##########
parquet-variant/src/variant/object.rs:
##########
@@ -387,6 +389,31 @@ impl<'m, 'v> VariantObject<'m, 'v> {
}
}
+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
Review Comment:
See https://github.com/apache/arrow-rs/pull/7961#issuecomment-3089972179
For example, depending on what we want to achieve with these logical comparisons, two logically equivalent objects need not have the same header byte.
--
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
Re: [PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on code in PR #7943:
URL: https://github.com/apache/arrow-rs/pull/7943#discussion_r2211549323
##########
parquet-variant/src/builder.rs:
##########
@@ -252,7 +304,28 @@ impl ValueBuffer {
variant: Variant<'m, 'd>,
metadata_builder: &mut MetadataBuilder,
) {
- self.try_append_variant(variant, metadata_builder).unwrap();
+ match variant {
+ Variant::Null => self.append_null(),
+ Variant::BooleanTrue => self.append_bool(true),
+ Variant::BooleanFalse => self.append_bool(false),
+ Variant::Int8(v) => self.append_int8(v),
+ Variant::Int16(v) => self.append_int16(v),
+ Variant::Int32(v) => self.append_int32(v),
+ Variant::Int64(v) => self.append_int64(v),
+ Variant::Date(v) => self.append_date(v),
+ Variant::TimestampMicros(v) => self.append_timestamp_micros(v),
+ Variant::TimestampNtzMicros(v) => self.append_timestamp_ntz_micros(v),
+ Variant::Decimal4(decimal4) => self.append_decimal4(decimal4),
+ Variant::Decimal8(decimal8) => self.append_decimal8(decimal8),
+ Variant::Decimal16(decimal16) => self.append_decimal16(decimal16),
+ Variant::Float(v) => self.append_float(v),
+ Variant::Double(v) => self.append_double(v),
+ Variant::Binary(v) => self.append_binary(v),
+ Variant::String(s) => self.append_string(s),
+ Variant::ShortString(s) => self.append_short_string(s),
+ Variant::Object(obj) => self.append_object(metadata_builder, obj),
+ Variant::List(list) => self.append_list(metadata_builder, list),
+ }
Review Comment:
It is unfortunate to have to copy the same match statement as `try_append_variant`, but `ValueBuffer::append_object` and `ValueBuffer::try_append_object` calls different iterators and append methods
--
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
Re: [PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7943:
URL: https://github.com/apache/arrow-rs/pull/7943#issuecomment-3084494146
What I plan to do is merge this PR and then write some more tests for a few more cases
--
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
Re: [PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #7943:
URL: https://github.com/apache/arrow-rs/pull/7943#discussion_r2214190973
##########
parquet-variant/src/builder.rs:
##########
@@ -216,6 +217,57 @@ impl ValueBuffer {
self.append_slice(value.as_bytes());
}
+ fn append_object(&mut self, metadata_builder: &mut MetadataBuilder, obj: VariantObject) {
+ let mut object_builder = self.new_object(metadata_builder);
+
+ for (field_name, value) in obj.iter() {
+ object_builder.insert(field_name, value);
+ }
+
+ object_builder.finish().unwrap();
+ }
+
+ fn try_append_object(
+ &mut self,
+ metadata_builder: &mut MetadataBuilder,
+ obj: VariantObject,
+ ) -> Result<(), ArrowError> {
+ let mut object_builder = self.new_object(metadata_builder);
+
+ for res in obj.iter_try() {
Review Comment:
Except `append_object` is infallible... so even if we "consolidate" by having `try_append_object` fall back to its infallible partner, both similar-but-different for loops will still exist.
--
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
Re: [PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7943:
URL: https://github.com/apache/arrow-rs/pull/7943#discussion_r2213281991
##########
parquet-variant/src/variant/object.rs:
##########
@@ -718,4 +752,187 @@ mod tests {
test_variant_object_with_large_data(16777216 + 1, OffsetSizeBytes::Four);
// 2^24
}
+
+ #[test]
+ fn test_objects_with_same_fields_are_equal() {
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("b", ());
+ o.insert("c", ());
+ o.insert("a", ());
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v1 = Variant::try_new(&m, &v).unwrap();
+ let v2 = Variant::try_new(&m, &v).unwrap();
+
+ assert_eq!(v1, v2);
+ }
+
+ #[test]
+ fn test_same_objects_with_different_builder_are_equal() {
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ o.insert("b", false);
+
+ o.finish().unwrap();
+ let (m, v) = b.finish();
+
+ let v1 = Variant::try_new(&m, &v).unwrap();
+
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ o.insert("b", false);
+
+ o.finish().unwrap();
+ let (m, v) = b.finish();
+
+ let v2 = Variant::try_new(&m, &v).unwrap();
+
+ assert_eq!(v1, v2);
+ }
+
+ #[test]
+ fn test_objects_with_different_values_are_not_equal() {
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ o.insert("b", 4.3);
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v1 = Variant::try_new(&m, &v).unwrap();
+
+ // second object, same field name but different values
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ let mut inner_o = o.new_object("b");
+ inner_o.insert("a", 3.3);
+ inner_o.finish().unwrap();
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v2 = Variant::try_new(&m, &v).unwrap();
+
+ let m1 = v1.metadata().unwrap();
+ let m2 = v2.metadata().unwrap();
+
+ // metadata would be equal since they contain the same keys
+ assert_eq!(m1, m2);
+
+ // but the objects are not equal
+ assert_ne!(v1, v2);
+ }
+
+ #[test]
+ fn test_objects_with_different_field_names_are_not_equal() {
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ o.insert("b", 4.3);
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v1 = Variant::try_new(&m, &v).unwrap();
+
+ // second object, same field name but different values
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("aardvark", ());
+ o.insert("barracuda", 3.3);
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+ let v2 = Variant::try_new(&m, &v).unwrap();
+
+ assert_ne!(v1, v2);
+ }
+
+ #[test]
+ fn test_objects_with_different_insertion_order_are_equal() {
+ let mut b = VariantBuilder::new();
+ let mut o = b.new_object();
+
+ o.insert("b", false);
+ o.insert("a", ());
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v1 = Variant::try_new(&m, &v).unwrap();
+ assert!(!v1.metadata().unwrap().is_sorted());
+
+ // create another object pre-filled with field names, b and a
+ // but insert the fields in the order of a, b
+ let mut b = VariantBuilder::new().with_field_names(["b", "a"].into_iter());
+ let mut o = b.new_object();
+
+ o.insert("a", ());
+ o.insert("b", false);
+
+ o.finish().unwrap();
+
+ let (m, v) = b.finish();
+
+ let v2 = Variant::try_new(&m, &v).unwrap();
+
+ // v2 should also have a unsorted dictionary
+ assert!(!v2.metadata().unwrap().is_sorted());
+
+ assert_eq!(v1, v2);
+ }
+
+ #[test]
+ fn test_objects_with_differing_metadata_are_equal() {
Review Comment:
For a follow on work, I think we should
--
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
Re: [PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7943:
URL: https://github.com/apache/arrow-rs/pull/7943#issuecomment-3083734490
> Unrelated but these public methods on `VariantMetadata` are redundant:
- Filed an issue to track: https://github.com/apache/arrow-rs/issues/7947
--
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
Re: [PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7943:
URL: https://github.com/apache/arrow-rs/pull/7943#discussion_r2213695642
##########
parquet-variant/src/variant/metadata.rs:
##########
@@ -332,6 +334,22 @@ impl<'m> VariantMetadata<'m> {
}
}
+impl<'m> PartialEq for VariantMetadata<'m> {
+ fn eq(&self, other: &Self) -> bool {
+ let mut is_equal = self.is_empty() == other.is_empty()
+ && self.is_fully_validated() == other.is_fully_validated()
Review Comment:
- Filed https://github.com/apache/arrow-rs/issues/7952 to track
--
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
Re: [PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #7943:
URL: https://github.com/apache/arrow-rs/pull/7943
--
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
Re: [PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7943:
URL: https://github.com/apache/arrow-rs/pull/7943#issuecomment-3084649948
Here is a follow on PR:
- https://github.com/apache/arrow-rs/pull/7953
--
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
Re: [PR] [Variant] Impl `PartialEq` for VariantObject [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on code in PR #7943:
URL: https://github.com/apache/arrow-rs/pull/7943#discussion_r2215509595
##########
parquet-variant/src/variant/object.rs:
##########
@@ -387,6 +389,31 @@ impl<'m, 'v> VariantObject<'m, 'v> {
}
}
+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
Review Comment:
cc @scovich, I'm curious if this looks reasonable to you? I may be misunderstanding something..
--
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