Change ScalarValue::List to store ArrayRef #7629

jayzhan211 · 2023-09-23T05:24:45Z

Which issue does this PR close?

Closes #7352
Closes #996

Rationale for this change

What changes are included in this PR?

Old ScalarValue::List is replaced with ListArray. convert_array_to_scalar_vec and scalars_to_list_array are two core utilities function that transform between Vec and ListArray.

Are these changes tested?

Are there any user-facing changes?

jayzhan211 · 2023-10-04T15:22:37Z

Ready for review! Although this PR is quite large, I hope it is easy to read. @tustvold @alamb

alamb · 2023-10-04T16:58:09Z

I plan to give this a look later today or tomorrow. Thanks @jayzhan211

alamb

Thank you @jayzhan211 -- this is an epic PR. I am sorry for the delay in reviewing. It is hard for me to find enough contiguous time to review large PRs like this.

I left some comments. Other than the todo!() in PartialOrd (which would result in a panic) I think this PR is basically ready to go.

datafusion/common/src/utils.rs

datafusion/common/src/scalar.rs

alamb · 2023-10-10T16:27:22Z

datafusion/common/src/scalar.rs

@@ -1653,42 +1619,66 @@ impl ScalarValue {
        Ok(array)
    }

-    fn iter_to_array_list(
-        scalars: impl IntoIterator<Item = ScalarValue>,
+    // This function does not contains nulls but empty array instead.


what does "function not contain nulls" mean?

NullBuffer for ListArray is None in this function, you can't get NULL in the array from this function, only an empty array is possible.

I tried to have one function for iter_to_array_list_without_nulls and iter_to_array_list but failed since one needs nulls, and another needs an empty array.

datafusion/common/src/scalar.rs

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

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

alamb · 2023-10-10T16:46:45Z

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

-                in_data.data_type()
-            )
+        array_agg: Vec<Vec<ScalarValue>>,
+    ) -> Result<Vec<Vec<Vec<ScalarValue>>>> {


Would it be possible to document what the three levels of Vec mean semantically ?

Actually, I don't understand either 😢 Let me take a chance to learn more about aggregate.

I left the comment from merge_ordered_arrays for now. I think there might be some space to improve OrderSensitiveArrayAggAccumulator, but it is better to do it in another PR.

alamb · 2023-10-10T16:48:46Z

datafusion/proto/tests/cases/roundtrip_logical_plan.rs

@@ -424,59 +427,6 @@ fn scalar_values_error_serialization() {
            Some(vec![]),
            vec![Field::new("item", DataType::Int16, true)].into(),
        ),
-        // Should fail due to inconsistent types in the list


why were these tests removed? Do they pass now? If so, perhaps we can move them to the positive cases?

Because it is not possible to build inconsistent types with ListArray, therefore I just removed the whole test.

datafusion/proto/tests/cases/roundtrip_logical_plan.rs

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

alamb · 2023-10-12T11:28:55Z

@jayzhan211 please mark this PR as ready for review when you are ready for me to give it another look.

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

jayzhan211 · 2023-10-13T01:29:36Z

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

+        // in_data is Vec<ScalarValue> where ScalarValue does not include ScalarValue::List
+        for in_data in array_agg.into_iter() {
+            let ordering = in_data.into_iter().map(|struct_vals| {
+                    if let ScalarValue::Struct(Some(orderings), _) = struct_vals {


The reason we have 3 levels of Vec in orderings is because we got Vec\<ScalarValue> in Struct, but orderings seems to have only one ScalarValue, orderings.len() == 1 based on the test.

Is it reasonable to have >1 ScalarValue for orderings?

Maybe @mustafasrepo or @metegenez can offer some guidance here

alamb

Thank you @jayzhan211 -- this is an epic PR and reflects a lot of work. Thank you for the contribution

alamb · 2023-10-13T17:28:26Z

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

+        // in_data is Vec<ScalarValue> where ScalarValue does not include ScalarValue::List
+        for in_data in array_agg.into_iter() {
+            let ordering = in_data.into_iter().map(|struct_vals| {
+                    if let ScalarValue::Struct(Some(orderings), _) = struct_vals {


Maybe @mustafasrepo or @metegenez can offer some guidance here

jayzhan211 · 2023-10-13T23:41:29Z

Thank you @jayzhan211 -- this is an epic PR and reflects a lot of work. Thank you for the contribution

Thanks for your review!

…luelist

alamb · 2023-10-16T18:40:07Z

I merged this PR up from main and plan to merge it when it passes CI.

I will also make a follow on PR to clean up the to/from proto serialization.

alamb · 2023-10-16T18:42:28Z

Thanks again @jayzhan211 -- epic work.

Dandandan · 2023-10-17T11:46:54Z

Awesome, thanks @jayzhan211

alamb · 2023-10-26T15:54:59Z

I believe we found another bug here: #7938

github-actions bot added sql SQL Planner physical-expr Physical Expressions optimizer Optimizer rules core Core DataFusion crate labels Sep 23, 2023

jayzhan211 changed the title ~~Arrayref for scalarvaluelist~~ Make ScalarValue an ArrayRef Wrapper Sep 23, 2023

jayzhan211 force-pushed the arrayref-for-scalarvaluelist branch from dc280bf to 73e78c0 Compare September 25, 2023 14:42

jayzhan211 mentioned this pull request Sep 28, 2023

Minor: Move hash utils to common #7684

Merged

jayzhan211 changed the title ~~Make ScalarValue an ArrayRef Wrapper~~ Change ScalarValue::List to store ArrayRef Sep 29, 2023

jayzhan211 force-pushed the arrayref-for-scalarvaluelist branch 3 times, most recently from a87d75b to 8ce6e93 Compare October 4, 2023 00:58

jayzhan211 marked this pull request as ready for review October 4, 2023 14:35

alamb reviewed Oct 10, 2023

View reviewed changes

jayzhan211 marked this pull request as draft October 11, 2023 13:44

jayzhan211 added 14 commits October 11, 2023 21:48

first draft

30c84d3

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

revert subquery

0d1c79a

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

Decimal128 in list_to_array

fd3f050

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

refactor null in list_to_array

bcccbed

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

cleanup iter to array v3

c3a06a0

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

remove comment

e81ec13

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

cleanup

3e23a13

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

cleanup struct array

1043107

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

remove new_list in array

2a4a6d2

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

cleanup

27363e6

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

cleanup

5bd23f6

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

cleanup list to array

1d853b3

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

cleanup list to array

2af1aba

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

remove try from array v2

d0186f4

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

jayzhan211 added 3 commits October 11, 2023 21:48

rename and address comment

0e43051

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

address comment

76a0a73

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

rewrite roundtrip scalars

577612b

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

jayzhan211 force-pushed the arrayref-for-scalarvaluelist branch from 96f6f7d to 577612b Compare October 11, 2023 14:23

jayzhan211 added 4 commits October 12, 2023 08:40

fix flatten

82c8802

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

cleanup

b7daf86

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

add partial cmp

1924682

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

fmt

a11d5fd

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

jayzhan211 added 2 commits October 13, 2023 08:32

add comment

226d110

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

cleanup

116397a

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

jayzhan211 marked this pull request as ready for review October 13, 2023 00:43

jayzhan211 commented Oct 13, 2023

View reviewed changes

jayzhan211 requested a review from alamb October 13, 2023 12:16

alamb approved these changes Oct 13, 2023

View reviewed changes

alamb mentioned this pull request Oct 13, 2023

Change ScalarValue::List to store ArrayRef #7352

Closed

Merge remote-tracking branch 'apache/main' into arrayref-for-scalarva…

c59ec00

…luelist

alamb merged commit cb2d03c into apache:main Oct 16, 2023
22 checks passed

alamb mentioned this pull request Oct 16, 2023

Avoid panics on error while encoding/decoding ListValue::Array as protobuf #7837

Merged

alamb mentioned this pull request Oct 26, 2023

Panic with queries with multiple COUNT DISTINCT aggregates on dictionary values, and a group by #7938

Closed

alamb mentioned this pull request Oct 27, 2023

Support array aggregate sum function #7242

Closed

jayzhan211 mentioned this pull request Nov 17, 2023

Fix PartialOrd for ScalarValue::List/FixSizeList/LargeList #8253

Merged

alamb mentioned this pull request Nov 17, 2023

Explain on a plan containing a list literal is very verbose in v33 #8259

Closed

jonahgao mentioned this pull request Jan 7, 2024

fix: failed to create ValuesExec with non-nullable schema #8776

Merged

matthewgapp mentioned this pull request Jan 11, 2024

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change ScalarValue::List to store ArrayRef #7629

Change ScalarValue::List to store ArrayRef #7629

jayzhan211 commented Sep 23, 2023 •

edited by alamb

Loading

jayzhan211 commented Oct 4, 2023 •

edited

Loading

alamb commented Oct 4, 2023

alamb left a comment

alamb Oct 10, 2023

jayzhan211 Oct 11, 2023 •

edited

Loading

alamb Oct 10, 2023

jayzhan211 Oct 11, 2023

jayzhan211 Oct 13, 2023

alamb Oct 10, 2023

jayzhan211 Oct 11, 2023

alamb commented Oct 12, 2023

jayzhan211 Oct 13, 2023 •

edited

Loading

alamb Oct 13, 2023

alamb left a comment

alamb Oct 13, 2023

jayzhan211 commented Oct 13, 2023

alamb commented Oct 16, 2023

alamb commented Oct 16, 2023

Dandandan commented Oct 17, 2023

alamb commented Oct 26, 2023

Change ScalarValue::List to store ArrayRef #7629

Change ScalarValue::List to store ArrayRef #7629

Conversation

jayzhan211 commented Sep 23, 2023 • edited by alamb 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?

jayzhan211 commented Oct 4, 2023 • edited Loading

alamb commented Oct 4, 2023

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Oct 12, 2023

jayzhan211 Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 commented Oct 13, 2023

alamb commented Oct 16, 2023

alamb commented Oct 16, 2023

Dandandan commented Oct 17, 2023

alamb commented Oct 26, 2023

jayzhan211 commented Sep 23, 2023 •

edited by alamb

Loading

jayzhan211 commented Oct 4, 2023 •

edited

Loading

jayzhan211 Oct 11, 2023 •

edited

Loading

jayzhan211 Oct 13, 2023 •

edited

Loading