Re: [PR] [Variant] Speedup `ObjectBuilder` (62x faster) [arrow-rs]

[via GitHub]

friendlymatthew commented on code in PR #7808:
URL: https://github.com/apache/arrow-rs/pull/7808#discussion_r2176175096


##########
parquet-variant/src/builder.rs:
##########
@@ -643,16 +640,15 @@ impl<'a, 'b> ObjectBuilder<'a, 'b> {
         let num_fields = self.fields.len();
         let is_large = num_fields > u8::MAX as usize;
 
-        let field_ids_by_sorted_field_name = self
-            .metadata_builder
-            .field_name_to_id
-            .iter()
-            .filter_map(|(_, id)| self.fields.contains_key(id).then_some(*id))
-            .collect::<Vec<_>>();
+        self.fields.sort_by(|&field_a_id, _, &field_b_id, _| {
+            let key_a = &self.metadata_builder.field_name(field_a_id as usize);
+            let key_b = &self.metadata_builder.field_name(field_b_id as usize);
+            key_a.cmp(key_b)
+        });
 
-        let max_id = self.fields.keys().last().copied().unwrap_or(0) as usize;
+        let max_id = self.fields.iter().map(|(i, _)| *i).max().unwrap_or(0);

Review Comment:
   We have no guarantee that `fields` is sorted in order of `field_id`, only 
insertion order. You could imagine creating an object list where the first 
object is defined with field names `a`, `b`, but the second object is defined 
in the opposite order. 
   
   Here are some cases where `max_id` isn't necessarily the last element in 
`fields`. 
   
   ```rs
     #[test]
     fn nested() {
         let mut variant = VariantBuilder::new();
   
         let mut obj1 = variant.new_object();
   
         obj1.insert("c", false);
         obj1.insert("b", false);
   
         {
             let mut inner_obj = obj1.new_object("a");
             inner_obj.insert("a", false);
             inner_obj.insert("b", false);
   
             // Here, the nested object's `field` ids are laid out as [2, 1]
             dbg!(inner_obj.fields.iter().map(|(&i, _)| i).collect::<Vec<_>>());
             inner_obj.finish();
         }
   
         obj1.finish();
     }
   
     #[test]
     fn predefined_field_names() {
         let mut variant = VariantBuilder::new().with_field_names(&["a", "b", 
"c", "d", "e"]);
   
         let mut obj1 = variant.new_object();
   
         obj1.insert("e", false);
         obj1.insert("a", false);
   
         // [4, 0]
         dbg!(obj1.fields.iter().map(|(&i, _)| i).collect::<Vec<_>>());
   
         obj1.finish();
     }
   ```



-- 
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