feat: support `LargeList` in `make_array` and `array_length` #8121

Weijun-H · 2023-11-10T13:06:15Z

Which issue does this PR close?

Rationale for this change

The current implementation of the make_array and array_length functions lack support for LargeList. Therefore, we need to enhance these functions by incorporating the OffsizeTrait to enable them to work seamlessly with LargeList. This will ensure that the functions can handle a large number of elements in the list, thus improving their overall efficiency.

What changes are included in this PR?

make_array can be List and LargeList depending on the array length
support LargeList in array_lenght
array_array and array! can consider OffsizeTrait

Are these changes tested?

Are there any user-facing changes?

Weijun-H · 2023-11-10T13:17:17Z

datafusion/physical-expr/src/array_expressions.rs

+    fn return_large_array() -> ArrayRef {
+        // Returns: [1, 2, 3, 4]
+        let capacity = i32::MAX as usize + 10;
+        let args = vec![Arc::new(Int64Array::from(vec![Some(1)])) as ArrayRef; capacity];
+
+        println!("args.len() = {}", args.len());
+
+        make_array(&args).expect("failed to initialize function array")
+    }
+


I don't know how to efficiently create a LargeList using make_array as it runs out of memory on my end.

maybe you should use below constructor to construct LargeList

let list_array = Arc::new(LargeListArray::from_iter_primitive::<Int32Type, _, _>( vec![ Some(vec![Some(0), Some(1), Some(2)]), None, Some(vec![Some(3), None, Some(5)]), ], )) as ArrayRef

Weijun-H · 2023-11-15T09:16:54Z

datafusion/physical-expr/src/array_expressions.rs

+    fn return_large_array() -> ArrayRef {
+        // Returns: [1, 2, 3, 4]
+        let args = [
+            Arc::new(Int64Array::from(vec![Some(1)])) as ArrayRef,
+            Arc::new(Int64Array::from(vec![Some(2)])) as ArrayRef,
+            Arc::new(Int64Array::from(vec![Some(3)])) as ArrayRef,
+            Arc::new(Int64Array::from(vec![Some(4)])) as ArrayRef,
+        ];
+        let data_type = DataType::Int64;
+        array_array::<i64>(&args, data_type).expect("failed to initialize function array")
+    }
+


Hack here to create a LargeList to avoid spending too much memory for testing.

What is the hack?

Weijun-H · 2023-11-18T10:00:38Z

@alamb @jayzhan211 @Veeupup
PTAL :-)

alamb

Thank you for the contribution @Weijun-H -- the basic structure of most of this PR looks good to me -- thank you. I left some comments, but I think it is pretty close.

Perhaps @jayzhan211 has some additional thoughts to share

alamb · 2023-11-18T11:05:50Z

datafusion/common/src/scalar.rs

            Self::iter_to_array(values.iter().cloned()).unwrap()
        };
-        Arc::new(array_into_list_array(values))
+        if values.len() <= i32::MAX as usize {


I think the ScalarValue type needs to match the underyling array type -- so with this change, a ScalarValue::List might return LargeListArray or ListArray. This mismatch will likely cause issues downstream

I think the standard pattern would be to add a new function ScalarValue::new_large_list and ScalarValue::LargeList if they don't already exist

alamb · 2023-11-18T11:06:52Z

datafusion/physical-expr/src/array_expressions.rs

+    fn return_large_array() -> ArrayRef {
+        // Returns: [1, 2, 3, 4]
+        let args = [
+            Arc::new(Int64Array::from(vec![Some(1)])) as ArrayRef,
+            Arc::new(Int64Array::from(vec![Some(2)])) as ArrayRef,
+            Arc::new(Int64Array::from(vec![Some(3)])) as ArrayRef,
+            Arc::new(Int64Array::from(vec![Some(4)])) as ArrayRef,
+        ];
+        let data_type = DataType::Int64;
+        array_array::<i64>(&args, data_type).expect("failed to initialize function array")
+    }
+


What is the hack?

alamb · 2023-11-18T11:09:29Z

datafusion/physical-expr/src/array_expressions.rs

            as_uint64_array(&arr).expect("failed to initialize function array_length");

        assert_eq!(result, &UInt64Array::from(vec![None]));
+


Rather than extending this module with more rust tests, can you perhaps add SQL level tests using sqllogictest?

That will ensure the functions are usable end to end and that all the connections are in place

Perhaps https://github.com/apache/arrow-datafusion/blob/main/datafusion/sqllogictest/test_files/array.slt

I'm not sure how to create an large list in sql file. I think it's why he done the test here? @Weijun-H

Yes, I do not know how we could construct the LargeList in sqllogictest, instead of an array with more i32:MAX by make_array.

The current idea is to use arrow_cast() to create a LargeList instead of make_array. This pr is stalled by #8290 @alamb @jayzhan211

jayzhan211 · 2023-11-18T12:13:04Z

datafusion/physical-expr/src/array_expressions.rs

-    offsets.push(0);
+
+    let mut offsets: Vec<O> = Vec::with_capacity(total_len);
+    offsets.push(O::from_usize(0).unwrap());


jayzhan211 · 2023-11-18T12:13:50Z

datafusion/physical-expr/src/array_expressions.rs

+    Ok(Arc::new(GenericListArray::<O>::try_new(
        Arc::new(Field::new("item", data_type, true)),
-        OffsetBuffer::new(offsets.into()),
+        OffsetBuffer::new(ScalarBuffer::from(offsets)),


no need this for usize_as

jayzhan211 · 2023-11-18T12:16:27Z

datafusion/physical-expr/src/array_expressions.rs

+    match &args[0].data_type() {
+        DataType::List(_) => _array_length_list::<i32>(args),
+        DataType::LargeList(_) => _array_length_list::<i64>(args),
+        _ => Err(DataFusionError::Internal(format!(


jayzhan211 · 2023-11-18T12:17:45Z

datafusion/physical-expr/src/array_expressions.rs

+}
+
+/// array_length for List and LargeList
+fn _array_length_list<O: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> {


Any other name without _? In rust _xxx usually means unused.

Weijun-H · 2023-11-22T14:14:25Z

datafusion/sqllogictest/test_files/array.slt

 3 2

+query II
+select array_length(arrow_cast(array_repeat(array_repeat(array_repeat(3, 5), 2), 3), 'LargeList(List(List(Int64)))'), 1), array_length(arrow_cast(array_repeat(array_repeat(array_repeat(3, 5), 2), 3), 'LargeList(List(List(Int64)))'), 2);


Can not test LargeList(LargeList) because arrow-rs does not support it yet. #8305

Could it be possible to add a test that tries to cast LargeList(List) as a follow on PR? It would, of course, error initially, but then when we upgraded arrow to a version that did support that cast, we would have the test coverage

jayzhan211 · 2023-11-23T00:03:04Z

datafusion/physical-expr/src/array_expressions.rs

            }
        }
-        offsets.push(mutable.len() as i32);
+        offsets.push(O::from_usize(mutable.len()).unwrap());


I think we dont need unwrap if casting via usize_as

jayzhan211

Overall LGTM

Weijun-H · 2023-11-23T08:29:53Z

Overall LGTM

Thank you for reviewing 👍 @jayzhan211

jayzhan211

LGTM!

alamb

Thank you for this PR @Weijun-H -- I think it is very close

alamb · 2023-12-01T12:01:16Z

datafusion/physical-expr/src/array_expressions.rs

        DataType::Null => {
            let array = new_null_array(&DataType::Null, arrays.len());
-            Ok(Arc::new(array_into_list_array(array)))
+            if len <= i32::MAX as usize {


I still don't think that make_array should ever return NullArray (aka DataType::Null) and instead it should always return ListArray (possibly with a null value)

However, this PR doesn't make this situation worse, so 👍

datafusion/physical-expr/src/array_expressions.rs

array_length

alamb

Looks good to me -- thank you @Weijun-H

alamb · 2023-12-03T12:34:47Z

datafusion/sqllogictest/test_files/array.slt

 3 2

+query II
+select array_length(arrow_cast(array_repeat(array_repeat(array_repeat(3, 5), 2), 3), 'LargeList(List(List(Int64)))'), 1), array_length(arrow_cast(array_repeat(array_repeat(array_repeat(3, 5), 2), 3), 'LargeList(List(List(Int64)))'), 2);


Could it be possible to add a test that tries to cast LargeList(List) as a follow on PR? It would, of course, error initially, but then when we upgraded arrow to a version that did support that cast, we would have the test coverage

alamb · 2023-12-03T12:36:04Z

THanks for sticking with these PRs -- I know it has taken time, but now that we have the patterns down I feel like the code is really improving at a nice rate. This is what I think successful software development and incremental improvement looks like!

…8121) * feat: support LargeList in make_array and array_length * chore: add tests * fix: update tests for nested array * use usise_as * add new_large_list * refactor array_length * add comment * update test in sqllogictest * fix ci * fix macro * use usize_as * update comment * return based on data_type in make_array

github-actions bot added the physical-expr Changes to the physical-expr crates label Nov 10, 2023

Weijun-H commented Nov 10, 2023

View reviewed changes

Weijun-H force-pushed the feat-support-largelist branch 5 times, most recently from 91612a4 to b5f9e68 Compare November 15, 2023 09:14

Weijun-H commented Nov 15, 2023

View reviewed changes

Weijun-H marked this pull request as ready for review November 15, 2023 10:59

Weijun-H force-pushed the feat-support-largelist branch 3 times, most recently from f7bdb8f to a06fbf8 Compare November 16, 2023 10:17

Weijun-H mentioned this pull request Nov 17, 2023

Support LargeList for array #8185

Closed

19 tasks

Weijun-H force-pushed the feat-support-largelist branch from a06fbf8 to d340ff7 Compare November 17, 2023 21:29

alamb reviewed Nov 18, 2023

View reviewed changes

jayzhan211 reviewed Nov 18, 2023

View reviewed changes

Weijun-H mentioned this pull request Nov 21, 2023

support LargeList for arrow_cast, support ScalarValue::LargeList #8290

Merged

Weijun-H force-pushed the feat-support-largelist branch from 818685c to 6839aaa Compare November 22, 2023 14:11

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Nov 22, 2023

Weijun-H commented Nov 22, 2023

View reviewed changes

Weijun-H force-pushed the feat-support-largelist branch from 6839aaa to 5d5d0da Compare November 22, 2023 14:23

Weijun-H requested review from alamb and jayzhan211 November 22, 2023 15:53

jayzhan211 reviewed Nov 23, 2023

View reviewed changes

jayzhan211 requested changes Nov 23, 2023

View reviewed changes

Weijun-H requested a review from jayzhan211 November 23, 2023 08:41

jayzhan211 approved these changes Nov 23, 2023

View reviewed changes

Weijun-H force-pushed the feat-support-largelist branch 2 times, most recently from 0e58ca8 to 5d678a0 Compare December 1, 2023 08:50

alamb reviewed Dec 1, 2023

View reviewed changes

Weijun-H added 12 commits December 2, 2023 12:34

feat: support LargeList in make_array and

c7f41e9

array_length

chore: add tests

7429fd9

fix: update tests for nested array

ce16e9a

use usise_as

abb31d6

add new_large_list

b696a46

refactor array_length

3d9a0e6

add comment

19feb9b

update test in sqllogictest

7599f66

fix ci

d1f9bd0

fix macro

4861538

use usize_as

328ad3c

update comment

05e365a

Weijun-H force-pushed the feat-support-largelist branch from 95a8396 to 05e365a Compare December 2, 2023 13:32

return based on data_type in make_array

6caf58f

Weijun-H force-pushed the feat-support-largelist branch from 0b0e475 to 6caf58f Compare December 2, 2023 14:04

Weijun-H requested a review from alamb December 2, 2023 14:04

alamb approved these changes Dec 3, 2023

View reviewed changes

alamb merged commit f6af014 into apache:main Dec 3, 2023

matthewgapp mentioned this pull request Jan 11, 2024

matt/feat/recursive ctes/config flag matthewgapp/arrow-datafusion#3

Closed

		as_uint64_array(&arr).expect("failed to initialize function array_length");

		assert_eq!(result, &UInt64Array::from(vec![None]));

Uh oh!

feat: support LargeList in make_array and array_length #8121

feat: support LargeList in make_array and array_length #8121

Uh oh!

Conversation

Weijun-H commented Nov 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Weijun-H commented Nov 18, 2023

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Weijun-H Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayzhan211 left a comment

Choose a reason for hiding this comment

Uh oh!

Weijun-H commented Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayzhan211 left a comment

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Dec 3, 2023

Uh oh!

Reviewers

Assignees

feat: support `LargeList` in `make_array` and `array_length` #8121

feat: support `LargeList` in `make_array` and `array_length` #8121

Weijun-H commented Nov 10, 2023 •

edited

Loading

Weijun-H Nov 22, 2023 •

edited

Loading

Weijun-H commented Nov 23, 2023 •

edited

Loading