You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "rluvaton (via GitHub)" <gi...@apache.org> on 2024/12/26 17:45:35 UTC
[PR] test(arrow-ord): remove duplicate logic and make it easier to add tests [arrow-rs]
rluvaton opened a new pull request, #6913:
URL: https://github.com/apache/arrow-rs/pull/6913
# Rationale for this change
While working on #6911 and I started to write tests, I found out I just duplicated the logic, and it was messier, so cleaning up to make it easier to create new tests.
**Background:** all sort arrays test helper were given 2 vectors with native rust types (e.g. `Vec<Option<bool>>`), one for the input and the other for the expected output.
and the sort test helper convert it to the input and expected arrays to Arrow arrays (e.g. `BooleanArray`), and then call `sort` (or `sort_limit` depend on the input) and assert that the expected match the sorted.
this is done to help make the test simpler by just providing what the actual arrow array represent without messing with converting to arrow type.
the problem is that some Rust types can have multiple Arrow types, for example `Vec<Option<Vec<u8>>>` can be `BinaryArray` or `LargeBinaryArray` or `FixedSizeBinaryArray` or `BinaryViewArray` (same for lists and string).
in that case each sort helper function convert the input data to some of the arrow type (like `FixedSizeBinaryArray`) or to multiple arrow type (`BinaryArray` and `LargeBinaryArray`)
and then do the same as described above.
The problem with this is that there are a lot of duplication and adding tests to new type is just adding a lot of boilerplate instead of just starting to write code
# What changes are included in this PR?
So instead of this approach I created a single `test_sort_array` function that is given the `input`, `expected` and a function to convert that rust input type (e.g. `Vec<Option<Vec<u8>>>` like explained above) to all variants from that type (e.g. `BinaryArray` and `LargeBinaryArray` and `FixedSizeBinaryArray` and `BinaryViewArray`)
then created for each test type a new helper to convert that rust type to arrow type
and moved all the build array helpers to sub module for more organize code.
But why do I pass a function to build the array and not using Rust trait system? this will surely be cleaner as you can have the actual function implementation hidden behind the input type and it would make the test cleaner.
You (basically my other self) are right. The problem is that I tried and it can't work without implementing for each primitive type manually (I think), because let's see the following example:
Let's say I have a trait called `ConvertToArray`
```rust
trait ConvertToArrays {
fn convert_to_array(input: Vec<Option<Self>>) -> Vec<ArrayRef>;
}
```
and then I implement it for primitive type to create primitive array:
```rust
impl<T> ConvertToArrays for T::Native
where T: ArrowPrimitiveType,
PrimitiveArray<T>: From<Vec<Option<T::Native>>>
{
fn convert_to_array(input: Vec<Option<Self>>) -> Vec<ArrayRef> {
vec![Arc::new(PrimitiveArray::<T>::from(data)) as ArrayRef]
}
}
```
all good and it works. the problem is I can't implement it for `bool` when I want to test sorting `BooleanArray` because `ArrowPrimitiveType` can be implemented in the future for `bool` and this would cause duplicate implementation. and for that reason Rust disallow that
# Are there any user-facing changes?
nope, only tests
--
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] test(arrow-ord): remove duplicate logic and make it easier to add tests [arrow-rs]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on code in PR #6913:
URL: https://github.com/apache/arrow-rs/pull/6913#discussion_r1901662382
##########
arrow-ord/src/sort.rs:
##########
@@ -801,19 +801,186 @@ mod tests {
use half::f16;
use rand::rngs::StdRng;
use rand::{Rng, RngCore, SeedableRng};
+ use std::ops::Deref;
+
+ mod build_arrays_helper {
+ use crate::sort::tests::get_same_lists_length;
+ use arrow_array::{
+ ArrayRef, ArrowPrimitiveType, Decimal128Array, Decimal256Array, FixedSizeBinaryArray,
+ FixedSizeListArray, GenericBinaryArray, GenericListArray, LargeStringArray,
+ OffsetSizeTrait, PrimitiveArray, StringArray, StringViewArray,
+ };
+ use arrow_buffer::i256;
+ use std::sync::Arc;
+
+ pub fn decimal128_array(data: Vec<Option<i128>>) -> Vec<ArrayRef> {
+ vec![Arc::new(
+ data.into_iter()
+ .collect::<Decimal128Array>()
+ .with_precision_and_scale(23, 6)
+ .unwrap(),
+ )]
+ }
- fn create_decimal128_array(data: Vec<Option<i128>>) -> Decimal128Array {
- data.into_iter()
- .collect::<Decimal128Array>()
- .with_precision_and_scale(23, 6)
- .unwrap()
+ pub fn decimal256_array(data: Vec<Option<i256>>) -> Vec<ArrayRef> {
+ vec![Arc::new(
+ data.into_iter()
+ .collect::<Decimal256Array>()
+ .with_precision_and_scale(53, 6)
+ .unwrap(),
+ )]
+ }
+
+ pub fn primitive_arrays<T>(data: Vec<Option<T::Native>>) -> Vec<ArrayRef>
+ where
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ vec![Arc::new(PrimitiveArray::<T>::from(data)) as ArrayRef]
+ }
+
+ pub fn string_arrays(data: Vec<Option<&str>>) -> Vec<ArrayRef> {
+ vec![
+ Arc::new(StringArray::from(data.clone())) as ArrayRef,
+ Arc::new(LargeStringArray::from(data.clone())) as ArrayRef,
+ Arc::new(StringViewArray::from(data)) as ArrayRef,
+ ]
+ }
+
+ pub fn binary_arrays(data: Vec<Option<Vec<u8>>>) -> Vec<ArrayRef> {
+ // Generic size binary array
+ fn generic_binary_array<S: OffsetSizeTrait>(
+ data: &[Option<Vec<u8>>],
+ ) -> Arc<GenericBinaryArray<S>> {
+ Arc::new(GenericBinaryArray::<S>::from_opt_vec(
+ data.iter()
+ .map(|binary| binary.as_ref().map(Vec::as_slice))
+ .collect(),
+ ))
+ }
+
+ let mut arrays = vec![
+ generic_binary_array::<i32>(&data) as ArrayRef,
+ generic_binary_array::<i64>(&data) as ArrayRef,
+ ];
+
+ if let Some(first_length) = get_same_lists_length(&data) {
+ arrays.push(Arc::new(
+ FixedSizeBinaryArray::try_from_sparse_iter_with_size(
+ data.iter().cloned(),
+ first_length as i32,
+ )
+ .unwrap(),
+ ));
+ }
+
+ arrays
+ }
+
+ pub fn primitive_generic_list_array<OffsetSize, T>(
+ data: &[Option<Vec<Option<T::Native>>>],
+ ) -> ArrayRef
+ where
+ OffsetSize: OffsetSizeTrait,
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ Arc::new(GenericListArray::<OffsetSize>::from_iter_primitive::<T, _, _>(data.to_vec()))
+ }
+
+ pub fn primitive_fixed_list_array<T>(
+ data: &[Option<Vec<Option<T::Native>>>],
+ length: i32,
+ ) -> ArrayRef
+ where
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ Arc::new(FixedSizeListArray::from_iter_primitive::<T, _, _>(
+ data.to_vec(),
+ length,
+ ))
+ }
+
+ pub fn primitive_list_arrays<T>(data: Vec<Option<Vec<Option<T::Native>>>>) -> Vec<ArrayRef>
+ where
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ let mut arrays = vec![
+ primitive_generic_list_array::<i32, T>(&data),
+ primitive_generic_list_array::<i64, T>(&data),
+ ];
+
+ if let Some(first_length) = get_same_lists_length(&data) {
+ arrays.push(primitive_fixed_list_array::<T>(&data, first_length as i32));
+ }
+
+ arrays
+ }
}
- fn create_decimal256_array(data: Vec<Option<i256>>) -> Decimal256Array {
- data.into_iter()
- .collect::<Decimal256Array>()
- .with_precision_and_scale(53, 6)
- .unwrap()
+ /// Return Some(length) if all lists have the same length, None otherwise
+ fn get_same_lists_length<T>(data: &[Option<Vec<T>>]) -> Option<usize> {
+ let first_length = data
+ .iter()
+ .find_map(|x| x.as_ref().map(|x| x.len()))
+ .unwrap_or(0);
+ let first_non_match_length = data
+ .iter()
+ .map(|x| x.as_ref().map(|x| x.len()).unwrap_or(first_length))
+ .any(|x| x != first_length);
+
+ if first_non_match_length {
+ None
+ } else {
+ Some(first_length)
+ }
+ }
+
+ /// Test sorting arrays
+ ///
+ /// the `into_lists_fn` function is used to convert the data into the list variants wanted to be tested, (e.g. ListArray, LargeListArray, FixedSizeListArray or any other list variant)
+ /// The ordering of the returned vector must be consistent as it will be called with the data and the expected data
+ fn test_sort_arrays<
+ ListItemType: Clone,
+ IntoListFn: Fn(Vec<Option<ListItemType>>) -> Vec<ArrayRef>,
+ >(
+ into_lists_fn: IntoListFn,
+ data: Vec<Option<ListItemType>>,
+ options: Option<SortOptions>,
+ limit: Option<usize>,
+ expected_data: Vec<Option<ListItemType>>,
+ ) {
+ let input = into_lists_fn(data);
+ let mut expected = into_lists_fn(expected_data);
+
+ // Filter out mismatched data types
+ // This can happen for getting some type of Fixed array in the expected (due to limit or something), but not in the input
+ if input.len() != expected.len() {
+ let all_input_specific_class = input
+ .iter()
+ .map(|array| array.data_type())
+ .collect::<Vec<_>>();
+ expected = expected
+ .into_iter()
+ .filter(|array| all_input_specific_class.contains(&array.data_type()))
+ .collect::<Vec<_>>()
+ }
+
+ assert_eq!(
+ input.len(),
+ expected.len(),
+ "The input and expected data must have the same number of array variants"
+ );
+
+ for (input, expected) in input.iter().zip(expected.iter()) {
+ let sorted = match limit {
+ Some(_) => sort_limit(input, options, limit).unwrap(),
+ _ => sort(input, options).unwrap(),
+ };
Review Comment:
I prefer to keep it as is as the `sort_limit` does not call `sort` when `limit` is `None`. and this is how it was in the original tests
##########
arrow-ord/src/sort.rs:
##########
@@ -801,19 +801,186 @@ mod tests {
use half::f16;
use rand::rngs::StdRng;
use rand::{Rng, RngCore, SeedableRng};
+ use std::ops::Deref;
+
+ mod build_arrays_helper {
+ use crate::sort::tests::get_same_lists_length;
+ use arrow_array::{
+ ArrayRef, ArrowPrimitiveType, Decimal128Array, Decimal256Array, FixedSizeBinaryArray,
+ FixedSizeListArray, GenericBinaryArray, GenericListArray, LargeStringArray,
+ OffsetSizeTrait, PrimitiveArray, StringArray, StringViewArray,
+ };
+ use arrow_buffer::i256;
+ use std::sync::Arc;
+
+ pub fn decimal128_array(data: Vec<Option<i128>>) -> Vec<ArrayRef> {
+ vec![Arc::new(
+ data.into_iter()
+ .collect::<Decimal128Array>()
+ .with_precision_and_scale(23, 6)
+ .unwrap(),
+ )]
+ }
- fn create_decimal128_array(data: Vec<Option<i128>>) -> Decimal128Array {
- data.into_iter()
- .collect::<Decimal128Array>()
- .with_precision_and_scale(23, 6)
- .unwrap()
+ pub fn decimal256_array(data: Vec<Option<i256>>) -> Vec<ArrayRef> {
+ vec![Arc::new(
+ data.into_iter()
+ .collect::<Decimal256Array>()
+ .with_precision_and_scale(53, 6)
+ .unwrap(),
+ )]
+ }
+
+ pub fn primitive_arrays<T>(data: Vec<Option<T::Native>>) -> Vec<ArrayRef>
+ where
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ vec![Arc::new(PrimitiveArray::<T>::from(data)) as ArrayRef]
+ }
+
+ pub fn string_arrays(data: Vec<Option<&str>>) -> Vec<ArrayRef> {
+ vec![
+ Arc::new(StringArray::from(data.clone())) as ArrayRef,
+ Arc::new(LargeStringArray::from(data.clone())) as ArrayRef,
+ Arc::new(StringViewArray::from(data)) as ArrayRef,
+ ]
+ }
+
+ pub fn binary_arrays(data: Vec<Option<Vec<u8>>>) -> Vec<ArrayRef> {
+ // Generic size binary array
+ fn generic_binary_array<S: OffsetSizeTrait>(
+ data: &[Option<Vec<u8>>],
+ ) -> Arc<GenericBinaryArray<S>> {
+ Arc::new(GenericBinaryArray::<S>::from_opt_vec(
+ data.iter()
+ .map(|binary| binary.as_ref().map(Vec::as_slice))
+ .collect(),
+ ))
+ }
+
+ let mut arrays = vec![
+ generic_binary_array::<i32>(&data) as ArrayRef,
+ generic_binary_array::<i64>(&data) as ArrayRef,
+ ];
+
+ if let Some(first_length) = get_same_lists_length(&data) {
+ arrays.push(Arc::new(
+ FixedSizeBinaryArray::try_from_sparse_iter_with_size(
+ data.iter().cloned(),
+ first_length as i32,
+ )
+ .unwrap(),
+ ));
+ }
+
+ arrays
+ }
+
+ pub fn primitive_generic_list_array<OffsetSize, T>(
+ data: &[Option<Vec<Option<T::Native>>>],
+ ) -> ArrayRef
+ where
+ OffsetSize: OffsetSizeTrait,
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ Arc::new(GenericListArray::<OffsetSize>::from_iter_primitive::<T, _, _>(data.to_vec()))
+ }
+
+ pub fn primitive_fixed_list_array<T>(
+ data: &[Option<Vec<Option<T::Native>>>],
+ length: i32,
+ ) -> ArrayRef
+ where
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ Arc::new(FixedSizeListArray::from_iter_primitive::<T, _, _>(
+ data.to_vec(),
+ length,
+ ))
+ }
+
+ pub fn primitive_list_arrays<T>(data: Vec<Option<Vec<Option<T::Native>>>>) -> Vec<ArrayRef>
+ where
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ let mut arrays = vec![
+ primitive_generic_list_array::<i32, T>(&data),
+ primitive_generic_list_array::<i64, T>(&data),
+ ];
+
+ if let Some(first_length) = get_same_lists_length(&data) {
+ arrays.push(primitive_fixed_list_array::<T>(&data, first_length as i32));
+ }
+
+ arrays
+ }
}
- fn create_decimal256_array(data: Vec<Option<i256>>) -> Decimal256Array {
- data.into_iter()
- .collect::<Decimal256Array>()
- .with_precision_and_scale(53, 6)
- .unwrap()
+ /// Return Some(length) if all lists have the same length, None otherwise
+ fn get_same_lists_length<T>(data: &[Option<Vec<T>>]) -> Option<usize> {
+ let first_length = data
+ .iter()
+ .find_map(|x| x.as_ref().map(|x| x.len()))
+ .unwrap_or(0);
+ let first_non_match_length = data
+ .iter()
+ .map(|x| x.as_ref().map(|x| x.len()).unwrap_or(first_length))
+ .any(|x| x != first_length);
+
+ if first_non_match_length {
+ None
+ } else {
+ Some(first_length)
+ }
+ }
+
+ /// Test sorting arrays
+ ///
+ /// the `into_lists_fn` function is used to convert the data into the list variants wanted to be tested, (e.g. ListArray, LargeListArray, FixedSizeListArray or any other list variant)
+ /// The ordering of the returned vector must be consistent as it will be called with the data and the expected data
+ fn test_sort_arrays<
+ ListItemType: Clone,
+ IntoListFn: Fn(Vec<Option<ListItemType>>) -> Vec<ArrayRef>,
+ >(
+ into_lists_fn: IntoListFn,
+ data: Vec<Option<ListItemType>>,
+ options: Option<SortOptions>,
+ limit: Option<usize>,
+ expected_data: Vec<Option<ListItemType>>,
+ ) {
Review Comment:
thanks, applied
--
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] test(arrow-ord): remove duplicate logic and make it easier to add tests [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #6913:
URL: https://github.com/apache/arrow-rs/pull/6913#issuecomment-2570032937
There's some CI errors to look into.
Beyond that, I'm confused by some of the reasoning provided in the body:
> But why do I pass a function to build the array and not using Rust trait system? this will surely be cleaner as you can have the actual function implementation hidden behind the input type and it would make the test cleaner.
>
> You (basically my other self) are right. The problem is that I tried and it can't work without implementing for each primitive type manually (I think), because let's see the following example:
>
> ...
>
> all good and it works. the problem is I can't implement it for `bool` when I want to test sorting `BooleanArray` because `ArrowPrimitiveType` can be implemented in the future for `bool` and this would cause duplicate implementation. and for that reason Rust disallow that
I find it unlikely that `ArrowPrimitiveType` will be implemented for `bool` in the future, and even if it does I'm sure there'll be other problems such that this test code breaking is one of the smaller issues that would follow. But even disregarding that, none of the tests you've modified are for `BooleanArray`s, so I'm not sure it should be used as an example of why a trait based approach might not work.
Of course if a trait based approach is more verbose/complex than the current function based approach I can understand why this approach was chosen.
--
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] test(arrow-ord): remove duplicate logic and make it easier to add tests [arrow-rs]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on code in PR #6913:
URL: https://github.com/apache/arrow-rs/pull/6913#discussion_r1901661568
##########
arrow-ord/src/sort.rs:
##########
@@ -801,19 +801,186 @@ mod tests {
use half::f16;
use rand::rngs::StdRng;
use rand::{Rng, RngCore, SeedableRng};
+ use std::ops::Deref;
+
+ mod build_arrays_helper {
+ use crate::sort::tests::get_same_lists_length;
+ use arrow_array::{
+ ArrayRef, ArrowPrimitiveType, Decimal128Array, Decimal256Array, FixedSizeBinaryArray,
+ FixedSizeListArray, GenericBinaryArray, GenericListArray, LargeStringArray,
+ OffsetSizeTrait, PrimitiveArray, StringArray, StringViewArray,
+ };
+ use arrow_buffer::i256;
+ use std::sync::Arc;
+
+ pub fn decimal128_array(data: Vec<Option<i128>>) -> Vec<ArrayRef> {
+ vec![Arc::new(
+ data.into_iter()
+ .collect::<Decimal128Array>()
+ .with_precision_and_scale(23, 6)
+ .unwrap(),
+ )]
+ }
- fn create_decimal128_array(data: Vec<Option<i128>>) -> Decimal128Array {
- data.into_iter()
- .collect::<Decimal128Array>()
- .with_precision_and_scale(23, 6)
- .unwrap()
+ pub fn decimal256_array(data: Vec<Option<i256>>) -> Vec<ArrayRef> {
+ vec![Arc::new(
+ data.into_iter()
+ .collect::<Decimal256Array>()
+ .with_precision_and_scale(53, 6)
+ .unwrap(),
+ )]
+ }
+
+ pub fn primitive_arrays<T>(data: Vec<Option<T::Native>>) -> Vec<ArrayRef>
+ where
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ vec![Arc::new(PrimitiveArray::<T>::from(data)) as ArrayRef]
+ }
+
+ pub fn string_arrays(data: Vec<Option<&str>>) -> Vec<ArrayRef> {
+ vec![
+ Arc::new(StringArray::from(data.clone())) as ArrayRef,
+ Arc::new(LargeStringArray::from(data.clone())) as ArrayRef,
+ Arc::new(StringViewArray::from(data)) as ArrayRef,
+ ]
+ }
+
+ pub fn binary_arrays(data: Vec<Option<Vec<u8>>>) -> Vec<ArrayRef> {
+ // Generic size binary array
+ fn generic_binary_array<S: OffsetSizeTrait>(
+ data: &[Option<Vec<u8>>],
+ ) -> Arc<GenericBinaryArray<S>> {
+ Arc::new(GenericBinaryArray::<S>::from_opt_vec(
+ data.iter()
+ .map(|binary| binary.as_ref().map(Vec::as_slice))
+ .collect(),
+ ))
+ }
+
+ let mut arrays = vec![
+ generic_binary_array::<i32>(&data) as ArrayRef,
+ generic_binary_array::<i64>(&data) as ArrayRef,
+ ];
+
+ if let Some(first_length) = get_same_lists_length(&data) {
+ arrays.push(Arc::new(
+ FixedSizeBinaryArray::try_from_sparse_iter_with_size(
+ data.iter().cloned(),
+ first_length as i32,
+ )
+ .unwrap(),
+ ));
+ }
+
+ arrays
+ }
+
+ pub fn primitive_generic_list_array<OffsetSize, T>(
+ data: &[Option<Vec<Option<T::Native>>>],
+ ) -> ArrayRef
+ where
+ OffsetSize: OffsetSizeTrait,
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ Arc::new(GenericListArray::<OffsetSize>::from_iter_primitive::<T, _, _>(data.to_vec()))
+ }
+
+ pub fn primitive_fixed_list_array<T>(
+ data: &[Option<Vec<Option<T::Native>>>],
+ length: i32,
+ ) -> ArrayRef
+ where
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ Arc::new(FixedSizeListArray::from_iter_primitive::<T, _, _>(
+ data.to_vec(),
+ length,
+ ))
+ }
+
+ pub fn primitive_list_arrays<T>(data: Vec<Option<Vec<Option<T::Native>>>>) -> Vec<ArrayRef>
+ where
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
Review Comment:
Thanks, removed
##########
arrow-ord/src/sort.rs:
##########
@@ -801,19 +801,186 @@ mod tests {
use half::f16;
use rand::rngs::StdRng;
use rand::{Rng, RngCore, SeedableRng};
+ use std::ops::Deref;
+
+ mod build_arrays_helper {
+ use crate::sort::tests::get_same_lists_length;
+ use arrow_array::{
+ ArrayRef, ArrowPrimitiveType, Decimal128Array, Decimal256Array, FixedSizeBinaryArray,
+ FixedSizeListArray, GenericBinaryArray, GenericListArray, LargeStringArray,
+ OffsetSizeTrait, PrimitiveArray, StringArray, StringViewArray,
+ };
+ use arrow_buffer::i256;
+ use std::sync::Arc;
+
+ pub fn decimal128_array(data: Vec<Option<i128>>) -> Vec<ArrayRef> {
+ vec![Arc::new(
+ data.into_iter()
+ .collect::<Decimal128Array>()
+ .with_precision_and_scale(23, 6)
+ .unwrap(),
+ )]
+ }
- fn create_decimal128_array(data: Vec<Option<i128>>) -> Decimal128Array {
- data.into_iter()
- .collect::<Decimal128Array>()
- .with_precision_and_scale(23, 6)
- .unwrap()
+ pub fn decimal256_array(data: Vec<Option<i256>>) -> Vec<ArrayRef> {
+ vec![Arc::new(
+ data.into_iter()
+ .collect::<Decimal256Array>()
+ .with_precision_and_scale(53, 6)
+ .unwrap(),
+ )]
+ }
+
+ pub fn primitive_arrays<T>(data: Vec<Option<T::Native>>) -> Vec<ArrayRef>
+ where
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ vec![Arc::new(PrimitiveArray::<T>::from(data)) as ArrayRef]
+ }
+
+ pub fn string_arrays(data: Vec<Option<&str>>) -> Vec<ArrayRef> {
+ vec![
+ Arc::new(StringArray::from(data.clone())) as ArrayRef,
+ Arc::new(LargeStringArray::from(data.clone())) as ArrayRef,
+ Arc::new(StringViewArray::from(data)) as ArrayRef,
+ ]
+ }
+
+ pub fn binary_arrays(data: Vec<Option<Vec<u8>>>) -> Vec<ArrayRef> {
+ // Generic size binary array
+ fn generic_binary_array<S: OffsetSizeTrait>(
+ data: &[Option<Vec<u8>>],
+ ) -> Arc<GenericBinaryArray<S>> {
+ Arc::new(GenericBinaryArray::<S>::from_opt_vec(
+ data.iter()
+ .map(|binary| binary.as_ref().map(Vec::as_slice))
+ .collect(),
+ ))
+ }
+
+ let mut arrays = vec![
+ generic_binary_array::<i32>(&data) as ArrayRef,
+ generic_binary_array::<i64>(&data) as ArrayRef,
+ ];
Review Comment:
Thanks, updated
--
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] test(arrow-ord): remove duplicate logic and make it easier to add tests [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #6913:
URL: https://github.com/apache/arrow-rs/pull/6913#discussion_r1901575341
##########
arrow-ord/src/sort.rs:
##########
@@ -801,19 +801,186 @@ mod tests {
use half::f16;
use rand::rngs::StdRng;
use rand::{Rng, RngCore, SeedableRng};
+ use std::ops::Deref;
+
+ mod build_arrays_helper {
+ use crate::sort::tests::get_same_lists_length;
+ use arrow_array::{
+ ArrayRef, ArrowPrimitiveType, Decimal128Array, Decimal256Array, FixedSizeBinaryArray,
+ FixedSizeListArray, GenericBinaryArray, GenericListArray, LargeStringArray,
+ OffsetSizeTrait, PrimitiveArray, StringArray, StringViewArray,
+ };
+ use arrow_buffer::i256;
+ use std::sync::Arc;
+
+ pub fn decimal128_array(data: Vec<Option<i128>>) -> Vec<ArrayRef> {
+ vec![Arc::new(
+ data.into_iter()
+ .collect::<Decimal128Array>()
+ .with_precision_and_scale(23, 6)
+ .unwrap(),
+ )]
+ }
- fn create_decimal128_array(data: Vec<Option<i128>>) -> Decimal128Array {
- data.into_iter()
- .collect::<Decimal128Array>()
- .with_precision_and_scale(23, 6)
- .unwrap()
+ pub fn decimal256_array(data: Vec<Option<i256>>) -> Vec<ArrayRef> {
+ vec![Arc::new(
+ data.into_iter()
+ .collect::<Decimal256Array>()
+ .with_precision_and_scale(53, 6)
+ .unwrap(),
+ )]
+ }
+
+ pub fn primitive_arrays<T>(data: Vec<Option<T::Native>>) -> Vec<ArrayRef>
+ where
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ vec![Arc::new(PrimitiveArray::<T>::from(data)) as ArrayRef]
+ }
+
+ pub fn string_arrays(data: Vec<Option<&str>>) -> Vec<ArrayRef> {
+ vec![
+ Arc::new(StringArray::from(data.clone())) as ArrayRef,
+ Arc::new(LargeStringArray::from(data.clone())) as ArrayRef,
+ Arc::new(StringViewArray::from(data)) as ArrayRef,
+ ]
+ }
+
+ pub fn binary_arrays(data: Vec<Option<Vec<u8>>>) -> Vec<ArrayRef> {
+ // Generic size binary array
+ fn generic_binary_array<S: OffsetSizeTrait>(
+ data: &[Option<Vec<u8>>],
+ ) -> Arc<GenericBinaryArray<S>> {
+ Arc::new(GenericBinaryArray::<S>::from_opt_vec(
+ data.iter()
+ .map(|binary| binary.as_ref().map(Vec::as_slice))
+ .collect(),
+ ))
+ }
+
+ let mut arrays = vec![
+ generic_binary_array::<i32>(&data) as ArrayRef,
+ generic_binary_array::<i64>(&data) as ArrayRef,
+ ];
+
+ if let Some(first_length) = get_same_lists_length(&data) {
+ arrays.push(Arc::new(
+ FixedSizeBinaryArray::try_from_sparse_iter_with_size(
+ data.iter().cloned(),
+ first_length as i32,
+ )
+ .unwrap(),
+ ));
+ }
+
+ arrays
+ }
+
+ pub fn primitive_generic_list_array<OffsetSize, T>(
+ data: &[Option<Vec<Option<T::Native>>>],
+ ) -> ArrayRef
+ where
+ OffsetSize: OffsetSizeTrait,
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ Arc::new(GenericListArray::<OffsetSize>::from_iter_primitive::<T, _, _>(data.to_vec()))
+ }
+
+ pub fn primitive_fixed_list_array<T>(
+ data: &[Option<Vec<Option<T::Native>>>],
+ length: i32,
+ ) -> ArrayRef
+ where
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ Arc::new(FixedSizeListArray::from_iter_primitive::<T, _, _>(
+ data.to_vec(),
+ length,
+ ))
+ }
+
+ pub fn primitive_list_arrays<T>(data: Vec<Option<Vec<Option<T::Native>>>>) -> Vec<ArrayRef>
+ where
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ let mut arrays = vec![
+ primitive_generic_list_array::<i32, T>(&data),
+ primitive_generic_list_array::<i64, T>(&data),
+ ];
+
+ if let Some(first_length) = get_same_lists_length(&data) {
+ arrays.push(primitive_fixed_list_array::<T>(&data, first_length as i32));
+ }
+
+ arrays
+ }
}
- fn create_decimal256_array(data: Vec<Option<i256>>) -> Decimal256Array {
- data.into_iter()
- .collect::<Decimal256Array>()
- .with_precision_and_scale(53, 6)
- .unwrap()
+ /// Return Some(length) if all lists have the same length, None otherwise
+ fn get_same_lists_length<T>(data: &[Option<Vec<T>>]) -> Option<usize> {
+ let first_length = data
+ .iter()
+ .find_map(|x| x.as_ref().map(|x| x.len()))
+ .unwrap_or(0);
+ let first_non_match_length = data
+ .iter()
+ .map(|x| x.as_ref().map(|x| x.len()).unwrap_or(first_length))
+ .any(|x| x != first_length);
+
+ if first_non_match_length {
+ None
+ } else {
+ Some(first_length)
+ }
+ }
+
+ /// Test sorting arrays
+ ///
+ /// the `into_lists_fn` function is used to convert the data into the list variants wanted to be tested, (e.g. ListArray, LargeListArray, FixedSizeListArray or any other list variant)
+ /// The ordering of the returned vector must be consistent as it will be called with the data and the expected data
+ fn test_sort_arrays<
+ ListItemType: Clone,
+ IntoListFn: Fn(Vec<Option<ListItemType>>) -> Vec<ArrayRef>,
+ >(
+ into_lists_fn: IntoListFn,
+ data: Vec<Option<ListItemType>>,
+ options: Option<SortOptions>,
+ limit: Option<usize>,
+ expected_data: Vec<Option<ListItemType>>,
+ ) {
Review Comment:
```suggestion
fn test_sort_arrays<ListItemType, IntoListFn>(
into_lists_fn: IntoListFn,
data: Vec<Option<ListItemType>>,
options: Option<SortOptions>,
limit: Option<usize>,
expected_data: Vec<Option<ListItemType>>,
) where
ListItemType: Clone,
IntoListFn: Fn(Vec<Option<ListItemType>>) -> Vec<ArrayRef>,
{
```
Thoughts on using where bound here to make it a bit more readable?
##########
arrow-ord/src/sort.rs:
##########
@@ -801,19 +801,186 @@ mod tests {
use half::f16;
use rand::rngs::StdRng;
use rand::{Rng, RngCore, SeedableRng};
+ use std::ops::Deref;
+
+ mod build_arrays_helper {
+ use crate::sort::tests::get_same_lists_length;
+ use arrow_array::{
+ ArrayRef, ArrowPrimitiveType, Decimal128Array, Decimal256Array, FixedSizeBinaryArray,
+ FixedSizeListArray, GenericBinaryArray, GenericListArray, LargeStringArray,
+ OffsetSizeTrait, PrimitiveArray, StringArray, StringViewArray,
+ };
+ use arrow_buffer::i256;
+ use std::sync::Arc;
+
+ pub fn decimal128_array(data: Vec<Option<i128>>) -> Vec<ArrayRef> {
+ vec![Arc::new(
+ data.into_iter()
+ .collect::<Decimal128Array>()
+ .with_precision_and_scale(23, 6)
+ .unwrap(),
+ )]
+ }
- fn create_decimal128_array(data: Vec<Option<i128>>) -> Decimal128Array {
- data.into_iter()
- .collect::<Decimal128Array>()
- .with_precision_and_scale(23, 6)
- .unwrap()
+ pub fn decimal256_array(data: Vec<Option<i256>>) -> Vec<ArrayRef> {
+ vec![Arc::new(
+ data.into_iter()
+ .collect::<Decimal256Array>()
+ .with_precision_and_scale(53, 6)
+ .unwrap(),
+ )]
+ }
+
+ pub fn primitive_arrays<T>(data: Vec<Option<T::Native>>) -> Vec<ArrayRef>
+ where
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ vec![Arc::new(PrimitiveArray::<T>::from(data)) as ArrayRef]
+ }
+
+ pub fn string_arrays(data: Vec<Option<&str>>) -> Vec<ArrayRef> {
+ vec![
+ Arc::new(StringArray::from(data.clone())) as ArrayRef,
+ Arc::new(LargeStringArray::from(data.clone())) as ArrayRef,
+ Arc::new(StringViewArray::from(data)) as ArrayRef,
+ ]
+ }
+
+ pub fn binary_arrays(data: Vec<Option<Vec<u8>>>) -> Vec<ArrayRef> {
+ // Generic size binary array
+ fn generic_binary_array<S: OffsetSizeTrait>(
+ data: &[Option<Vec<u8>>],
+ ) -> Arc<GenericBinaryArray<S>> {
+ Arc::new(GenericBinaryArray::<S>::from_opt_vec(
+ data.iter()
+ .map(|binary| binary.as_ref().map(Vec::as_slice))
+ .collect(),
+ ))
+ }
+
+ let mut arrays = vec![
+ generic_binary_array::<i32>(&data) as ArrayRef,
+ generic_binary_array::<i64>(&data) as ArrayRef,
+ ];
+
+ if let Some(first_length) = get_same_lists_length(&data) {
+ arrays.push(Arc::new(
+ FixedSizeBinaryArray::try_from_sparse_iter_with_size(
+ data.iter().cloned(),
+ first_length as i32,
+ )
+ .unwrap(),
+ ));
+ }
+
+ arrays
+ }
+
+ pub fn primitive_generic_list_array<OffsetSize, T>(
+ data: &[Option<Vec<Option<T::Native>>>],
+ ) -> ArrayRef
+ where
+ OffsetSize: OffsetSizeTrait,
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ Arc::new(GenericListArray::<OffsetSize>::from_iter_primitive::<T, _, _>(data.to_vec()))
+ }
+
+ pub fn primitive_fixed_list_array<T>(
+ data: &[Option<Vec<Option<T::Native>>>],
+ length: i32,
+ ) -> ArrayRef
+ where
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ Arc::new(FixedSizeListArray::from_iter_primitive::<T, _, _>(
+ data.to_vec(),
+ length,
+ ))
+ }
+
+ pub fn primitive_list_arrays<T>(data: Vec<Option<Vec<Option<T::Native>>>>) -> Vec<ArrayRef>
+ where
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
Review Comment:
```suggestion
```
I'm not sure this bound is strictly necessary. Same for the other two list array functions above
##########
arrow-ord/src/sort.rs:
##########
@@ -801,19 +801,186 @@ mod tests {
use half::f16;
use rand::rngs::StdRng;
use rand::{Rng, RngCore, SeedableRng};
+ use std::ops::Deref;
+
+ mod build_arrays_helper {
+ use crate::sort::tests::get_same_lists_length;
+ use arrow_array::{
+ ArrayRef, ArrowPrimitiveType, Decimal128Array, Decimal256Array, FixedSizeBinaryArray,
+ FixedSizeListArray, GenericBinaryArray, GenericListArray, LargeStringArray,
+ OffsetSizeTrait, PrimitiveArray, StringArray, StringViewArray,
+ };
+ use arrow_buffer::i256;
+ use std::sync::Arc;
+
+ pub fn decimal128_array(data: Vec<Option<i128>>) -> Vec<ArrayRef> {
+ vec![Arc::new(
+ data.into_iter()
+ .collect::<Decimal128Array>()
+ .with_precision_and_scale(23, 6)
+ .unwrap(),
+ )]
+ }
- fn create_decimal128_array(data: Vec<Option<i128>>) -> Decimal128Array {
- data.into_iter()
- .collect::<Decimal128Array>()
- .with_precision_and_scale(23, 6)
- .unwrap()
+ pub fn decimal256_array(data: Vec<Option<i256>>) -> Vec<ArrayRef> {
+ vec![Arc::new(
+ data.into_iter()
+ .collect::<Decimal256Array>()
+ .with_precision_and_scale(53, 6)
+ .unwrap(),
+ )]
+ }
+
+ pub fn primitive_arrays<T>(data: Vec<Option<T::Native>>) -> Vec<ArrayRef>
+ where
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ vec![Arc::new(PrimitiveArray::<T>::from(data)) as ArrayRef]
+ }
+
+ pub fn string_arrays(data: Vec<Option<&str>>) -> Vec<ArrayRef> {
+ vec![
+ Arc::new(StringArray::from(data.clone())) as ArrayRef,
+ Arc::new(LargeStringArray::from(data.clone())) as ArrayRef,
+ Arc::new(StringViewArray::from(data)) as ArrayRef,
+ ]
+ }
+
+ pub fn binary_arrays(data: Vec<Option<Vec<u8>>>) -> Vec<ArrayRef> {
+ // Generic size binary array
+ fn generic_binary_array<S: OffsetSizeTrait>(
+ data: &[Option<Vec<u8>>],
+ ) -> Arc<GenericBinaryArray<S>> {
+ Arc::new(GenericBinaryArray::<S>::from_opt_vec(
+ data.iter()
+ .map(|binary| binary.as_ref().map(Vec::as_slice))
+ .collect(),
+ ))
+ }
+
+ let mut arrays = vec![
+ generic_binary_array::<i32>(&data) as ArrayRef,
+ generic_binary_array::<i64>(&data) as ArrayRef,
+ ];
Review Comment:
```suggestion
let binary_data = data
.iter()
.map(|binary| binary.as_ref().map(Vec::as_slice))
.collect::<Vec<_>>();
let mut arrays = vec![
Arc::new(BinaryArray::from_opt_vec(binary_data.clone())) as ArrayRef,
Arc::new(LargeBinaryArray::from_opt_vec(binary_data)) as ArrayRef,
];
```
I found it a bit confusing that binary arrays have a specific helper function generic across their offset sizes whereas string right above doesn't; thoughts on simplifying to this to align with string (somewhat)?
##########
arrow-ord/src/sort.rs:
##########
@@ -801,19 +801,186 @@ mod tests {
use half::f16;
use rand::rngs::StdRng;
use rand::{Rng, RngCore, SeedableRng};
+ use std::ops::Deref;
+
+ mod build_arrays_helper {
+ use crate::sort::tests::get_same_lists_length;
+ use arrow_array::{
+ ArrayRef, ArrowPrimitiveType, Decimal128Array, Decimal256Array, FixedSizeBinaryArray,
+ FixedSizeListArray, GenericBinaryArray, GenericListArray, LargeStringArray,
+ OffsetSizeTrait, PrimitiveArray, StringArray, StringViewArray,
+ };
+ use arrow_buffer::i256;
+ use std::sync::Arc;
+
+ pub fn decimal128_array(data: Vec<Option<i128>>) -> Vec<ArrayRef> {
+ vec![Arc::new(
+ data.into_iter()
+ .collect::<Decimal128Array>()
+ .with_precision_and_scale(23, 6)
+ .unwrap(),
+ )]
+ }
- fn create_decimal128_array(data: Vec<Option<i128>>) -> Decimal128Array {
- data.into_iter()
- .collect::<Decimal128Array>()
- .with_precision_and_scale(23, 6)
- .unwrap()
+ pub fn decimal256_array(data: Vec<Option<i256>>) -> Vec<ArrayRef> {
+ vec![Arc::new(
+ data.into_iter()
+ .collect::<Decimal256Array>()
+ .with_precision_and_scale(53, 6)
+ .unwrap(),
+ )]
+ }
+
+ pub fn primitive_arrays<T>(data: Vec<Option<T::Native>>) -> Vec<ArrayRef>
+ where
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ vec![Arc::new(PrimitiveArray::<T>::from(data)) as ArrayRef]
+ }
+
+ pub fn string_arrays(data: Vec<Option<&str>>) -> Vec<ArrayRef> {
+ vec![
+ Arc::new(StringArray::from(data.clone())) as ArrayRef,
+ Arc::new(LargeStringArray::from(data.clone())) as ArrayRef,
+ Arc::new(StringViewArray::from(data)) as ArrayRef,
+ ]
+ }
+
+ pub fn binary_arrays(data: Vec<Option<Vec<u8>>>) -> Vec<ArrayRef> {
+ // Generic size binary array
+ fn generic_binary_array<S: OffsetSizeTrait>(
+ data: &[Option<Vec<u8>>],
+ ) -> Arc<GenericBinaryArray<S>> {
+ Arc::new(GenericBinaryArray::<S>::from_opt_vec(
+ data.iter()
+ .map(|binary| binary.as_ref().map(Vec::as_slice))
+ .collect(),
+ ))
+ }
+
+ let mut arrays = vec![
+ generic_binary_array::<i32>(&data) as ArrayRef,
+ generic_binary_array::<i64>(&data) as ArrayRef,
+ ];
+
+ if let Some(first_length) = get_same_lists_length(&data) {
+ arrays.push(Arc::new(
+ FixedSizeBinaryArray::try_from_sparse_iter_with_size(
+ data.iter().cloned(),
+ first_length as i32,
+ )
+ .unwrap(),
+ ));
+ }
+
+ arrays
+ }
+
+ pub fn primitive_generic_list_array<OffsetSize, T>(
+ data: &[Option<Vec<Option<T::Native>>>],
+ ) -> ArrayRef
+ where
+ OffsetSize: OffsetSizeTrait,
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ Arc::new(GenericListArray::<OffsetSize>::from_iter_primitive::<T, _, _>(data.to_vec()))
+ }
+
+ pub fn primitive_fixed_list_array<T>(
+ data: &[Option<Vec<Option<T::Native>>>],
+ length: i32,
+ ) -> ArrayRef
+ where
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ Arc::new(FixedSizeListArray::from_iter_primitive::<T, _, _>(
+ data.to_vec(),
+ length,
+ ))
+ }
+
+ pub fn primitive_list_arrays<T>(data: Vec<Option<Vec<Option<T::Native>>>>) -> Vec<ArrayRef>
+ where
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ let mut arrays = vec![
+ primitive_generic_list_array::<i32, T>(&data),
+ primitive_generic_list_array::<i64, T>(&data),
+ ];
+
+ if let Some(first_length) = get_same_lists_length(&data) {
+ arrays.push(primitive_fixed_list_array::<T>(&data, first_length as i32));
+ }
+
+ arrays
+ }
}
- fn create_decimal256_array(data: Vec<Option<i256>>) -> Decimal256Array {
- data.into_iter()
- .collect::<Decimal256Array>()
- .with_precision_and_scale(53, 6)
- .unwrap()
+ /// Return Some(length) if all lists have the same length, None otherwise
+ fn get_same_lists_length<T>(data: &[Option<Vec<T>>]) -> Option<usize> {
+ let first_length = data
+ .iter()
+ .find_map(|x| x.as_ref().map(|x| x.len()))
+ .unwrap_or(0);
+ let first_non_match_length = data
+ .iter()
+ .map(|x| x.as_ref().map(|x| x.len()).unwrap_or(first_length))
+ .any(|x| x != first_length);
+
+ if first_non_match_length {
+ None
+ } else {
+ Some(first_length)
+ }
+ }
+
+ /// Test sorting arrays
+ ///
+ /// the `into_lists_fn` function is used to convert the data into the list variants wanted to be tested, (e.g. ListArray, LargeListArray, FixedSizeListArray or any other list variant)
+ /// The ordering of the returned vector must be consistent as it will be called with the data and the expected data
+ fn test_sort_arrays<
+ ListItemType: Clone,
+ IntoListFn: Fn(Vec<Option<ListItemType>>) -> Vec<ArrayRef>,
+ >(
+ into_lists_fn: IntoListFn,
+ data: Vec<Option<ListItemType>>,
+ options: Option<SortOptions>,
+ limit: Option<usize>,
+ expected_data: Vec<Option<ListItemType>>,
+ ) {
+ let input = into_lists_fn(data);
+ let mut expected = into_lists_fn(expected_data);
+
+ // Filter out mismatched data types
+ // This can happen for getting some type of Fixed array in the expected (due to limit or something), but not in the input
+ if input.len() != expected.len() {
+ let all_input_specific_class = input
+ .iter()
+ .map(|array| array.data_type())
+ .collect::<Vec<_>>();
+ expected = expected
+ .into_iter()
+ .filter(|array| all_input_specific_class.contains(&array.data_type()))
+ .collect::<Vec<_>>()
+ }
+
+ assert_eq!(
+ input.len(),
+ expected.len(),
+ "The input and expected data must have the same number of array variants"
+ );
+
+ for (input, expected) in input.iter().zip(expected.iter()) {
+ let sorted = match limit {
+ Some(_) => sort_limit(input, options, limit).unwrap(),
+ _ => sort(input, options).unwrap(),
+ };
Review Comment:
```suggestion
let sorted = sort_limit(input, options, limit).unwrap();
```
Looks like can pass `None` as limit into `sort_limit` so I think this can be simplified like so
--
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] test(arrow-ord): remove duplicate logic and make it easier to add tests [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6913:
URL: https://github.com/apache/arrow-rs/pull/6913#issuecomment-2564746235
> I'm willing to help, although currently my codebase knowledge is not the greatest but I think it's getting better
Thank you 🙏 -- the most helpful thing would be to review outstanding PRs and ensure the description is understandable, the code is commented and covered with tests, and you think the code does what the description says
If you find such PRs, feel free to @ mention me and I'll try and give it a look asap
--
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] test(arrow-ord): remove duplicate logic and make it easier to add tests [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #6913:
URL: https://github.com/apache/arrow-rs/pull/6913#discussion_r1902277707
##########
arrow-ord/src/sort.rs:
##########
@@ -801,19 +801,186 @@ mod tests {
use half::f16;
use rand::rngs::StdRng;
use rand::{Rng, RngCore, SeedableRng};
+ use std::ops::Deref;
+
+ mod build_arrays_helper {
+ use crate::sort::tests::get_same_lists_length;
+ use arrow_array::{
+ ArrayRef, ArrowPrimitiveType, Decimal128Array, Decimal256Array, FixedSizeBinaryArray,
+ FixedSizeListArray, GenericBinaryArray, GenericListArray, LargeStringArray,
+ OffsetSizeTrait, PrimitiveArray, StringArray, StringViewArray,
+ };
+ use arrow_buffer::i256;
+ use std::sync::Arc;
+
+ pub fn decimal128_array(data: Vec<Option<i128>>) -> Vec<ArrayRef> {
+ vec![Arc::new(
+ data.into_iter()
+ .collect::<Decimal128Array>()
+ .with_precision_and_scale(23, 6)
+ .unwrap(),
+ )]
+ }
- fn create_decimal128_array(data: Vec<Option<i128>>) -> Decimal128Array {
- data.into_iter()
- .collect::<Decimal128Array>()
- .with_precision_and_scale(23, 6)
- .unwrap()
+ pub fn decimal256_array(data: Vec<Option<i256>>) -> Vec<ArrayRef> {
+ vec![Arc::new(
+ data.into_iter()
+ .collect::<Decimal256Array>()
+ .with_precision_and_scale(53, 6)
+ .unwrap(),
+ )]
+ }
+
+ pub fn primitive_arrays<T>(data: Vec<Option<T::Native>>) -> Vec<ArrayRef>
+ where
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ vec![Arc::new(PrimitiveArray::<T>::from(data)) as ArrayRef]
+ }
+
+ pub fn string_arrays(data: Vec<Option<&str>>) -> Vec<ArrayRef> {
+ vec![
+ Arc::new(StringArray::from(data.clone())) as ArrayRef,
+ Arc::new(LargeStringArray::from(data.clone())) as ArrayRef,
+ Arc::new(StringViewArray::from(data)) as ArrayRef,
+ ]
+ }
+
+ pub fn binary_arrays(data: Vec<Option<Vec<u8>>>) -> Vec<ArrayRef> {
+ // Generic size binary array
+ fn generic_binary_array<S: OffsetSizeTrait>(
+ data: &[Option<Vec<u8>>],
+ ) -> Arc<GenericBinaryArray<S>> {
+ Arc::new(GenericBinaryArray::<S>::from_opt_vec(
+ data.iter()
+ .map(|binary| binary.as_ref().map(Vec::as_slice))
+ .collect(),
+ ))
+ }
+
+ let mut arrays = vec![
+ generic_binary_array::<i32>(&data) as ArrayRef,
+ generic_binary_array::<i64>(&data) as ArrayRef,
+ ];
+
+ if let Some(first_length) = get_same_lists_length(&data) {
+ arrays.push(Arc::new(
+ FixedSizeBinaryArray::try_from_sparse_iter_with_size(
+ data.iter().cloned(),
+ first_length as i32,
+ )
+ .unwrap(),
+ ));
+ }
+
+ arrays
+ }
+
+ pub fn primitive_generic_list_array<OffsetSize, T>(
+ data: &[Option<Vec<Option<T::Native>>>],
+ ) -> ArrayRef
+ where
+ OffsetSize: OffsetSizeTrait,
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ Arc::new(GenericListArray::<OffsetSize>::from_iter_primitive::<T, _, _>(data.to_vec()))
+ }
+
+ pub fn primitive_fixed_list_array<T>(
+ data: &[Option<Vec<Option<T::Native>>>],
+ length: i32,
+ ) -> ArrayRef
+ where
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ Arc::new(FixedSizeListArray::from_iter_primitive::<T, _, _>(
+ data.to_vec(),
+ length,
+ ))
+ }
+
+ pub fn primitive_list_arrays<T>(data: Vec<Option<Vec<Option<T::Native>>>>) -> Vec<ArrayRef>
+ where
+ T: ArrowPrimitiveType,
+ PrimitiveArray<T>: From<Vec<Option<T::Native>>>,
+ {
+ let mut arrays = vec![
+ primitive_generic_list_array::<i32, T>(&data),
+ primitive_generic_list_array::<i64, T>(&data),
+ ];
+
+ if let Some(first_length) = get_same_lists_length(&data) {
+ arrays.push(primitive_fixed_list_array::<T>(&data, first_length as i32));
+ }
+
+ arrays
+ }
}
- fn create_decimal256_array(data: Vec<Option<i256>>) -> Decimal256Array {
- data.into_iter()
- .collect::<Decimal256Array>()
- .with_precision_and_scale(53, 6)
- .unwrap()
+ /// Return Some(length) if all lists have the same length, None otherwise
+ fn get_same_lists_length<T>(data: &[Option<Vec<T>>]) -> Option<usize> {
+ let first_length = data
+ .iter()
+ .find_map(|x| x.as_ref().map(|x| x.len()))
+ .unwrap_or(0);
+ let first_non_match_length = data
+ .iter()
+ .map(|x| x.as_ref().map(|x| x.len()).unwrap_or(first_length))
+ .any(|x| x != first_length);
+
+ if first_non_match_length {
+ None
+ } else {
+ Some(first_length)
+ }
+ }
+
+ /// Test sorting arrays
+ ///
+ /// the `into_lists_fn` function is used to convert the data into the list variants wanted to be tested, (e.g. ListArray, LargeListArray, FixedSizeListArray or any other list variant)
+ /// The ordering of the returned vector must be consistent as it will be called with the data and the expected data
+ fn test_sort_arrays<
+ ListItemType: Clone,
+ IntoListFn: Fn(Vec<Option<ListItemType>>) -> Vec<ArrayRef>,
+ >(
+ into_lists_fn: IntoListFn,
+ data: Vec<Option<ListItemType>>,
+ options: Option<SortOptions>,
+ limit: Option<usize>,
+ expected_data: Vec<Option<ListItemType>>,
+ ) {
Review Comment:
I think this was missed from the changes you applied
--
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] test(arrow-ord): remove duplicate logic and make it easier to add tests [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6913:
URL: https://github.com/apache/arrow-rs/pull/6913#issuecomment-2564740327
Thank you for these PRs @rluvaton -- I know we are seriously behind on PR review in arrow-rs. I hope that we will have more capacity after next week. We just need to find more people willing to help out with reviews
--
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] test(arrow-ord): remove duplicate logic and make it easier to add tests [arrow-rs]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on PR #6913:
URL: https://github.com/apache/arrow-rs/pull/6913#issuecomment-2564740838
I'm willing to help, although currently my codebase knowledge is not the greatest but I think it's getting better
--
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] test(arrow-ord): remove duplicate logic and make it easier to add tests [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6913:
URL: https://github.com/apache/arrow-rs/pull/6913#issuecomment-2583896326
Marking as draft as I think this PR is no longer waiting on feedback. Please mark it as ready for review when it is ready for another look
--
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] test(arrow-ord): remove duplicate logic and make it easier to add tests [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #6913:
URL: https://github.com/apache/arrow-rs/pull/6913#issuecomment-3691184218
Closing this PR as stale. Feel free to reopen if it becomes active again.
--
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] test(arrow-ord): remove duplicate logic and make it easier to add tests [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey closed pull request #6913: test(arrow-ord): remove duplicate logic and make it easier to add tests
URL: https://github.com/apache/arrow-rs/pull/6913
--
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