wjones127 commented on code in PR #5170: URL: https://github.com/apache/arrow-rs/pull/5170#discussion_r1416160007
########## arrow-select/src/take.rs: ########## Review Comment: It would also be nice to pre-compute the `capacity` of the `values` vector here: https://github.com/apache/arrow-rs/blob/932e0df1e7c888e56bae3eab3419ac33c87aa127/arrow-select/src/take.rs#L692 ########## arrow-select/src/take.rs: ########## @@ -1985,6 +1987,23 @@ mod tests { assert_eq!(&values, &[Some(23), Some(4), None, None]) } + #[test] + fn test_take_fixed_size_list_null_indices() { + let indices = Int32Array::new(vec![0, 1].into(), Some(NullBuffer::from(vec![true, false]))); + let values = Arc::new(Int32Array::from(vec![0, 1, 2, 3])); + let arr_field = Arc::new(Field::new("item", values.data_type().clone(), true)); + let values = FixedSizeListArray::try_new(arr_field, 2, values, None).unwrap(); + + let r = take(&values, &indices, None).unwrap(); + let values = r + .as_fixed_size_list() + .values() + .as_primitive::<Int32Type>() + .into_iter() + .collect::<Vec<_>>(); + assert_eq!(&values, &[Some(0), Some(1), None, None]) Review Comment: nit: You can directly compare `&ScalarBuffer` and a slice. ```suggestion let values = r .as_fixed_size_list() .values() .as_primitive::<Int32Type>() .values(); assert_eq!(values, &[Some(0), Some(1), None, None]) ``` ########## arrow-select/src/take.rs: ########## @@ -1985,6 +1987,23 @@ mod tests { assert_eq!(&values, &[Some(23), Some(4), None, None]) } + #[test] + fn test_take_fixed_size_list_null_indices() { + let indices = Int32Array::new(vec![0, 1].into(), Some(NullBuffer::from(vec![true, false]))); Review Comment: nit: maybe more straight forward to not separate values and nullability: ```suggestion let indices = Int32Array::from_iter([Some(1), None]); ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
