Convert list array and non-list array to scalars #7862

jayzhan211 · 2023-10-19T01:43:17Z

Which issue does this PR close?

Closes #.

Rationale for this change

convert_array_to_scalar_vec can convert well for list array, but we will get Vec<Vec<ScalarValue>> for non-list array (primitive array).
It would be nice to have another conversion that returns nicely Vec<ScalarValue> for the non-list array. I would need this for #7835 too.

What changes are included in this PR?

convert_list_array_to_scalar_vec for ListArray
convert_non_list_array_to_scalar_vec for non-ListArray

The nested array for DistinctArrayAggAccumulator (update_batch) is removed so as to test.

Are these changes tested?

Doc test

Are there any user-facing changes?

Weijun-H

LGTM! Thanks @jayzhan211 👍

alamb

Thanks @jayzhan211

datafusion/common/src/scalar.rs

datafusion/physical-expr/src/aggregate/array_agg_distinct.rs

jayzhan211 · 2023-10-25T12:24:43Z

Summary of changes

Only accept ListArray for convert_list_array, only accept non-ListArray for convert_non_list
Add nested list test for convert_list_array
extend wrap_into_list for multiple arrays
keep the nested list test for distinct array agg removed, since update_batch only convert-non-list cases.

alamb · 2023-10-30T20:14:13Z

I think this is not waiting on any more feedback

datafusion/common/src/scalar.rs

jayzhan211 · 2023-11-28T13:20:17Z

wait on #8253

jayzhan211 · 2023-12-09T12:57:11Z

wait on #8439

Signed-off-by: jayzhan211 <[email protected]>

datafusion/physical-expr/src/aggregate/array_agg_distinct.rs

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 · 2023-12-16T01:00:24Z

@alamb I think this is ready to go, thanks

alamb

Thank you @jayzhan211 -- this is looking quite close I think

alamb · 2023-12-16T12:48:42Z

datafusion/common/src/scalar.rs

-    /// Retrieve ScalarValue for each row in `array`
+    /// Retrieve `ScalarValue` for each row in `array`
+    ///
+    /// Convert `ListArray` to `Vec<Vec<ScalarValue>>`, first `Vec` is for rows, second `Vec` is for elements in the list


👍 It might also help to explain why we need two different signatures. It took me a while to grok things too

Suggested change

/// Convert `ListArray` to `Vec<Vec<ScalarValue>>`, first `Vec` is for rows, second `Vec` is for elements in the list

/// Convert `ListArray` into a 2 dimensional to `Vec<Vec<ScalarValue>>`, first `Vec` is for rows,

/// second `Vec` is for elements in the list.

///

/// See [`Self::convert_non_list_array_to_scalars`] for converting non Lists

///

/// This method is an optimization to unwrap nested ListArrays to nested Rust structures without

/// converting them twice

alamb · 2023-12-16T12:50:41Z

datafusion/common/src/scalar.rs

+    /// Retrieve `ScalarValue` for each row in `array`
+    ///


This seems left over

Suggested change

/// Retrieve `ScalarValue` for each row in `array`

///

alamb · 2023-12-16T12:58:31Z

datafusion/common/src/scalar.rs

@@ -2067,30 +2071,78 @@ impl ScalarValue {
    ///
    /// assert_eq!(scalar_vec, expected);
    /// ```
-    pub fn convert_array_to_scalar_vec(array: &dyn Array) -> Result<Vec<Vec<Self>>> {
-        let mut scalars = Vec::with_capacity(array.len());
+    pub fn convert_list_array_to_scalar_vec<O: OffsetSizeTrait>(


I think this would be easier to use if it didn't mix generic and non generic code

Maybe something like

Suggested change

pub fn convert_list_array_to_scalar_vec<O: OffsetSizeTrait>(

pub fn convert_list_array_to_scalar_vec(

array: &dyn Array,

) -> Result<Vec<Vec<Self>>> {

if let Some(arr) = array.as_list_opt::<i32> {

Self::convert_list_array_to_scalar_vec_internal(arr)

} else if let Some(arr) = array.as_list_opt::<64> {

Self::convert_list_array_to_scalar_vec_internal(arr)

} else {

_internal_err!("Expected GenericListArray but found: {array:?}")

}

}

And then internally pass in the cast Array by changing

fn convert_list_array_to_scalar_vec_internal<O: OffsetSizeTrait>( array: &dyn Array, ) -> Result<Vec<Vec<Self>>> {

to

fn convert_list_array_to_scalar_vec_internal<O: OffsetSizeTrait>( array: &GenericListArray<O>, ) -> Result<Vec<Vec<Self>>> {

Signed-off-by: jayzhan211 <[email protected]>

alamb · 2023-12-17T11:32:53Z

(BTW sorry for being so nit picky on this PR -- I think ScalarValue is a key and important API of DataFusion so avoiding API churn (continually changing the same API over and over again) is important

jayzhan211 · 2023-12-17T23:40:00Z

(BTW sorry for being so nit picky on this PR -- I think ScalarValue is a key and important API of DataFusion so avoiding API churn (continually changing the same API over and over again) is important

This comment is a bit too late...

And, this API is actually not breaking the previous one convert_array_to_scalar_vec, we can have three versions of them. convert_array_to_scalar_vec for the place that we can't ensure the type for array.

Anyway, this change is just improvement, not fixing a critical error, so I will close it.

alamb · 2023-12-18T18:56:15Z

I am sorry -- I feel really bad that we can't find more review bandwidth to match your contributions @jayzhan211 -- your efforts so far have been great and are really appreciated.

github-actions bot added physical-expr Physical Expressions core Core DataFusion crate labels Oct 19, 2023

jayzhan211 marked this pull request as ready for review October 19, 2023 14:32

Weijun-H approved these changes Oct 20, 2023

View reviewed changes

alamb reviewed Oct 23, 2023

View reviewed changes

datafusion/common/src/scalar.rs Outdated Show resolved Hide resolved

datafusion/common/src/scalar.rs Outdated Show resolved Hide resolved

datafusion/common/src/scalar.rs Show resolved Hide resolved

datafusion/physical-expr/src/aggregate/array_agg_distinct.rs Outdated Show resolved Hide resolved

jayzhan211 changed the title ~~Minor: Convert list array and non-list array to scalars~~ Convert list array and non-list array to scalars Oct 25, 2023

jayzhan211 marked this pull request as draft October 25, 2023 00:58

jayzhan211 force-pushed the convert-general-arr-to-scalar branch from e7fd4ca to c9f4ea1 Compare October 25, 2023 01:42

github-actions bot added sqllogictest SQL Logic Tests (.slt) and removed sqllogictest SQL Logic Tests (.slt) labels Oct 25, 2023

jayzhan211 marked this pull request as ready for review October 25, 2023 12:44

jayzhan211 requested review from alamb and Weijun-H October 25, 2023 12:44

alamb marked this pull request as draft October 30, 2023 20:13

jayzhan211 mentioned this pull request Oct 30, 2023

Minor: Extend wrap_into_list_array to accept multiple args #7993

Merged

jayzhan211 force-pushed the convert-general-arr-to-scalar branch from 4b780ea to 49a671f Compare November 11, 2023 06:09

jayzhan211 commented Nov 11, 2023

View reviewed changes

datafusion/common/src/scalar.rs Outdated Show resolved Hide resolved

jayzhan211 commented Nov 11, 2023

View reviewed changes

datafusion/common/src/scalar.rs Outdated Show resolved Hide resolved

jayzhan211 marked this pull request as ready for review November 11, 2023 08:01

jayzhan211 marked this pull request as draft November 17, 2023 13:37

jayzhan211 force-pushed the convert-general-arr-to-scalar branch from 542226d to 6b56441 Compare November 17, 2023 13:43

jayzhan211 mentioned this pull request Nov 28, 2023

Change ScalarValue::Struct to ArrayRef #7893

Merged

jayzhan211 force-pushed the convert-general-arr-to-scalar branch from d17a682 to 29ac936 Compare December 9, 2023 12:04

jayzhan211 added 3 commits December 13, 2023 22:14

list/non-list conversion

3472b51

Signed-off-by: jayzhan211 <[email protected]>

fix offset trait

d704c83

Signed-off-by: jayzhan211 <[email protected]>

fmt

aee4eff

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 force-pushed the convert-general-arr-to-scalar branch from 29ac936 to aee4eff Compare December 13, 2023 14:22

fmt

b92d535

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 marked this pull request as ready for review December 13, 2023 14:26

ci fix

3b6b53e

Signed-off-by: jayzhan211 <[email protected]>

Weijun-H reviewed Dec 13, 2023

View reviewed changes

datafusion/physical-expr/src/aggregate/array_agg_distinct.rs Show resolved Hide resolved

add largetlist

c69f874

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 requested a review from Weijun-H December 13, 2023 15:08

alamb reviewed Dec 16, 2023

View reviewed changes

alamb mentioned this pull request Dec 16, 2023

DataFusion weekly project plan (Andrew Lamb) - Dec 11, 2023 #8490

Closed

8 tasks

improve doc

f7b6ac0

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 closed this Dec 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert list array and non-list array to scalars #7862

Convert list array and non-list array to scalars #7862

jayzhan211 commented Oct 19, 2023 •

edited

Loading

Weijun-H left a comment •

edited

Loading

alamb left a comment

jayzhan211 commented Oct 25, 2023 •

edited

Loading

alamb commented Oct 30, 2023

jayzhan211 commented Nov 28, 2023

jayzhan211 commented Dec 9, 2023

jayzhan211 commented Dec 16, 2023

alamb left a comment

alamb Dec 16, 2023

alamb Dec 16, 2023

alamb Dec 16, 2023

alamb commented Dec 17, 2023 •

edited

Loading

jayzhan211 commented Dec 17, 2023 •

edited

Loading

alamb commented Dec 18, 2023

-    /// Convert `ListArray` to `Vec<Vec<ScalarValue>>`, first `Vec` is for rows, second `Vec` is for elements in the list
+    /// Convert `ListArray` into a 2 dimensional to `Vec<Vec<ScalarValue>>`, first `Vec` is for rows,
+    /// second `Vec` is for elements in the list.
+    ///
+    /// See [`Self::convert_non_list_array_to_scalars`] for converting non Lists
+    ///
+    /// This method is an optimization to unwrap nested ListArrays to nested Rust structures without
+    /// converting them twice

-    pub fn convert_list_array_to_scalar_vec<O: OffsetSizeTrait>(
+    pub fn convert_list_array_to_scalar_vec(
+      array: &dyn Array,
+    ) -> Result<Vec<Vec<Self>>> {
+        if let Some(arr) = array.as_list_opt::<i32> {
+            Self::convert_list_array_to_scalar_vec_internal(arr)
+        } else if let Some(arr) = array.as_list_opt::<64> {
+            Self::convert_list_array_to_scalar_vec_internal(arr)
+        } else {
+            _internal_err!("Expected GenericListArray but found: {array:?}")
+        }
+    }

Convert list array and non-list array to scalars #7862

Convert list array and non-list array to scalars #7862

Conversation

jayzhan211 commented Oct 19, 2023 • edited Loading

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?

Weijun-H left a comment • edited Loading

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

jayzhan211 commented Oct 25, 2023 • edited Loading

alamb commented Oct 30, 2023

jayzhan211 commented Nov 28, 2023

jayzhan211 commented Dec 9, 2023

jayzhan211 commented Dec 16, 2023

alamb left a comment

Choose a reason for hiding this comment

alamb Dec 16, 2023

Choose a reason for hiding this comment

alamb Dec 16, 2023

Choose a reason for hiding this comment

alamb Dec 16, 2023

Choose a reason for hiding this comment

alamb commented Dec 17, 2023 • edited Loading

jayzhan211 commented Dec 17, 2023 • edited Loading

alamb commented Dec 18, 2023

jayzhan211 commented Oct 19, 2023 •

edited

Loading

Weijun-H left a comment •

edited

Loading

jayzhan211 commented Oct 25, 2023 •

edited

Loading

alamb commented Dec 17, 2023 •

edited

Loading

jayzhan211 commented Dec 17, 2023 •

edited

Loading