scovich commented on code in PR #7961:
URL: https://github.com/apache/arrow-rs/pull/7961#discussion_r2220158563
##########
parquet-variant/src/variant/object.rs:
##########
@@ -263,7 +264,7 @@ impl<'m, 'v> VariantObject<'m, 'v> {
let next_field_name = self.metadata.get(field_id)?;
if let Some(current_name) = current_field_name {
- if next_field_name <= current_name {
+ if next_field_name < current_name {
Review Comment:
Bug fix, right?
##########
parquet-variant/src/variant/object.rs:
##########
@@ -412,26 +413,24 @@ 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
+ if self.num_elements != other.num_elements {
+ return false;
+ }
+
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) => {
Review Comment:
We don't need a hash map at all -- _IFF_ the two are valid and logically
equal, they will have the same field names in the same order, because the spec
requires the object fields to be sorted lexicographically. It should be enough
to co-iterate over the two sets of field+value pairs after verifying the field
counts match, e.g.
```rust
for ((name_a, value_a), (name_b, value_b)) in self.iter().zip(other.iter()) {
if name_a != name_b || value_a != value_b {
return false;
}
}
```
--
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
