Change ScalarValue::List to store ArrayRef by jayzhan211 · Pull Request #7629 · apache/datafusion

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


-    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

            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 <[email protected]>

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 <[email protected]>

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 Changes to the physical-expr crates 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 <[email protected]>

revert subquery

0d1c79a

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

Decimal128 in list_to_array

fd3f050

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

refactor null in list_to_array

bcccbed

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

cleanup iter to array v3

c3a06a0

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

remove comment

e81ec13

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

cleanup

3e23a13

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

cleanup struct array

1043107

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

remove new_list in array

2a4a6d2

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

cleanup

27363e6

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

cleanup

5bd23f6

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

cleanup list to array

1d853b3

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

cleanup list to array

2af1aba

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

remove try from array v2

d0186f4

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

jayzhan211 added 3 commits October 11, 2023 21:48

rename and address comment

0e43051

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

address comment

76a0a73

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

rewrite roundtrip scalars

577612b

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

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 <[email protected]>

cleanup

b7daf86

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

add partial cmp

1924682

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

fmt

a11d5fd

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

jayzhan211 added 2 commits October 13, 2023 08:32

add comment

226d110

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

cleanup

116397a

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

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

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

Comments

Conversation

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

jayzhan211 commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb commented Oct 4, 2023

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayzhan211 Oct 11, 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!

Uh oh!

Uh oh!

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!

Uh oh!

alamb commented Oct 12, 2023

Uh oh!

jayzhan211 Oct 13, 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!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayzhan211 commented Oct 13, 2023

Uh oh!

alamb commented Oct 16, 2023

Uh oh!

alamb commented Oct 16, 2023

Uh oh!

Dandandan commented Oct 17, 2023

Uh oh!

alamb commented Oct 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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