[GitHub] [arrow] drusso commented on a change in pull request #8556: ARROW-10378: [Rust] Update take() kernel with support for LargeList.

[GitBox <gi...@apache.org>]

drusso commented on a change in pull request #8556:
URL: https://github.com/apache/arrow/pull/8556#discussion_r518923411



##########
File path: rust/arrow/src/compute/kernels/take.rs
##########
@@ -195,15 +217,21 @@ where
 
     let null_slice = null_buf.data_mut();
 
-    let new_values: Vec<T::Native> = (0..data_len)
-        .map(|i| {
-            let index = indices.value(i) as usize;
-            if array.is_null(index) {
-                bit_util::unset_bit(null_slice, i);
-            }
-            array.value(index)
-        })
-        .collect();
+    let mut new_values: Vec<T::Native> = Vec::with_capacity(data_len);
+    let mut i = 0;
+    while i < data_len {

Review comment:
       I initially implemented this just as you wrote, but I opted for the `while` since it makes a non-negligible difference in the benchmarks. 
   
   I believe the while loop and the original `::collect::Vec<T::Native>()` version show equivalent performance – it's the while loop vs. `::collect::<Result<Vec<_>>()?` where there's difference. 
   
   




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org