Change `ScalarValue::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` #8562

rspears74 · 2023-12-16T00:13:54Z

Which issue does this PR close?

Rationale for this change

It's much preferable to constrain ScalarValue::List to accept only an Arc<ListArray> rather than any ArrayRef and fail randomly. Similar for ScalarValue::LargeList and ScalarValue::FixedSizeList.

What changes are included in this PR?

Changed the type signature of ScalarValue::List, ScalarValue::LargeList, and ScalarValue::FixedSizedList and any cascading changes that follow from this. One test compared a ListArray passed to ScalarValue::List to a LargeListArray passed to ScalarValue::List - because ScalarValue::List only accepts ListArray now, this test was changed to compare two ListArrays instead.

Are these changes tested?

Tests have been updated where needed.

Are there any user-facing changes?

Users are now only allowed to pass a ListArray to ScalarValue::List, a LargeListArray to ScalarValue::LargeList, and a FixedSizeListArray to ScalarValue::FixedSizeList.

Also ScalarValue::LargeList and ScalarValue::FixedSizeList

jayzhan211 · 2023-12-16T00:36:01Z

datafusion/common/src/scalar.rs

@@ -3300,33 +3409,6 @@ mod tests {
        assert_eq!(result, &expected);
    }

-    #[test]


Is ScalarValue::eq_array test for List elsewhere? Otherwise, we can add the test for it.

I removed it because it was comparing a ListArray with a LargeListArray, and thus was no longer compatible with ScalarValue::List. But I can add this test back and just do two ListArrays?

datafusion/proto/src/logical_plan/to_proto.rs

jayzhan211

LGTM

jayzhan211 · 2024-01-04T00:40:11Z

@alamb It seems you forget to trace this PR in #8704?

Weijun-H

LGTM! Thanks @rspears74 👍

Weijun-H · 2024-01-04T03:14:28Z

datafusion/common/src/scalar.rs

+                write!(f, "{value_formatter}")?
+            }
+            ScalarValue::LargeList(arr) => {
+                // ScalarValue List should always have a single element


Suggested change

// ScalarValue List should always have a single element

// ScalarValue LargeList should always have a single element

Weijun-H · 2024-01-04T03:14:47Z

datafusion/common/src/scalar.rs

+                let value_formatter = formatter.value(0);
+                write!(f, "{value_formatter}")?
+            }
+            ScalarValue::FixedSizeList(arr) => {
                // ScalarValue List should always have a single element


Suggested change

// ScalarValue List should always have a single element

// ScalarValue FixedSizeList should always have a single element

Weijun-H · 2024-01-04T03:16:49Z

datafusion/proto/src/logical_plan/to_proto.rs

+                    arr.to_owned() as ArrayRef,
+                )])
+                .map_err(|e| {
+                   Error::General( format!("Error creating temporary batch while encoding ScalarValue::List: {e}"))


Suggested change

Error::General( format!("Error creating temporary batch while encoding ScalarValue::List: {e}"))

Error::General( format!("Error creating temporary batch while encoding ScalarValue::LargeList: {e}"))

Weijun-H · 2024-01-04T03:17:13Z

datafusion/proto/src/logical_plan/to_proto.rs

+                    arr.to_owned() as ArrayRef,
+                )])
+                .map_err(|e| {
+                   Error::General( format!("Error creating temporary batch while encoding ScalarValue::List: {e}"))


Suggested change

Error::General( format!("Error creating temporary batch while encoding ScalarValue::List: {e}"))

Error::General( format!("Error creating temporary batch while encoding ScalarValue::FixedSizeList: {e}"))

alamb · 2024-01-04T22:22:02Z

I will try and find time to review this PR later this week . Sorry for the delay

alamb

Thank you for this PR @rspears74 - I think it is looking quite close. There are a few places where code gets repeated 3 times that I think we can avoid by refactoring code into specific functions. I left some more specific suggestions below.

Also thank you @jayzhan211 and @Weijun-H for the reviews

alamb · 2024-01-05T22:01:59Z

datafusion/proto/src/logical_plan/to_proto.rs

+                    )),
+                })
+            }
+            ScalarValue::LargeList(arr) => {


Would it be possible to avoid the copy/paste in this method by making a function? Something like the following?

fn encode_scalar_list_value(array: &Array) -> Result<protobuf::ScalarListValue> { ... }

This one was easy enough to implement.

alamb · 2024-01-05T22:12:39Z

datafusion/common/src/scalar.rs

-                        arr.value(0)
-                    } else {
-                        unreachable!("Since only List / LargeList / FixedSizeList are supported, this should never happen")
+                fn first_array_for_list(arr: &Arc<ListArray>) -> ArrayRef {


It would be really nice here to avoid the redundant copy/paste code, perhaps by making a function with signature like this

fn cmp_list_array(arr1: &dyn Array, arr2: &dyn Array) -> Option<Ordering> { .. }

Which then you can call with Arc<ListArray> and Arc<LargeListArray> types

This one is a little tougher, because if the args to the function are "generic" (i.e. ArrayRef instead of a specific Arc<ListArray> type), the arr1.value(0) method is not available. The first_array_for_list function needs to be able to convert the ArrayRef into the specific type to then get the value out. So I'm struggling a bit coming up with a way here.

Here is a proposal of how to do it: rspears74#1 (that targets this PR)

rspears74 · 2024-01-06T00:40:32Z

Welp, I had a merge conflict, and upon fixing it, there's some new code that's incompatible with these changes (not compiling), so I'm gonna have to do a little more involved work to fix those. Will try to make that happen ASAP.

rspears74 · 2024-01-06T06:43:55Z

Welp, I had a merge conflict, and upon fixing it, there's some new code that's incompatible with these changes (not compiling), so I'm gonna have to do a little more involved work to fix those. Will try to make that happen ASAP.

Alright that wasn't so bad. Fixed and passing tests.

jayzhan211

LGTM

alamb

Thank you @rspears74 -- I think this is looking good.

cc @wjones127 since I think you are interested in FixedSizedList in general

I think there is still some repetition that could be avoided (I left my suggestions on how to do so) but we could potentially do that as a follow on PR as well

alamb · 2024-01-06T13:48:01Z

datafusion/common/src/scalar.rs

-                        arr.value(0)
-                    } else {
-                        unreachable!("Since only List / LargeList / FixedSizeList are supported, this should never happen")
+                fn first_array_for_list(arr: &Arc<ListArray>) -> ArrayRef {


Here is a proposal of how to do it: rspears74#1 (that targets this PR)

alamb · 2024-01-06T13:50:58Z

datafusion/common/src/scalar.rs

                // ScalarValue List should always have a single element
                assert_eq!(arr.len(), 1);
                let options = FormatOptions::default().with_display_error(true);
-                let formatter = ArrayFormatter::try_new(arr, &options).unwrap();
+                let formatter =


The same pattern could be applied here as in rspears74#1 (use first_array_for_list)

I'll take care of this one too. I went through my changes looking for any larger duplication sections but missed this one.

Remove redundant partial_cmp implementation

wjones127

This seems acceptable. Like Andrew said I think we could avoid some repetition.

I do think it's worth quickly considering whether wrapping in Scalar might be useful, as right now there isn't any strong enforcement that the array values in these scalars is length 1.

datafusion/common/src/scalar.rs

wjones127 · 2024-01-06T21:10:12Z

datafusion/common/src/scalar.rs

@@ -142,13 +143,13 @@ pub enum ScalarValue {
    /// Fixed size list scalar.
    ///
    /// The array must be a FixedSizeListArray with length 1.
-    FixedSizeList(ArrayRef),
+    FixedSizeList(Arc<FixedSizeListArray>),


I know I'm coming in a little late, but have we considered instead making the type Scalar<Arc<FixedSizeListArray>>? (Scalar being the one in arrow-rs) This would enforce that the length is 1, which might be nice. I could also understand if that makes it too awkward to work with.

Personally I like the enforcement of length 1 (I mentioned this problem initially in the issue I opened), but it does seem to be a bit cumbersome to deal with.

I think it is a good idea.

Let's file a follow on ticket for continuing improving the representation of list constants

rspears74 · 2024-01-06T23:37:03Z

Not sure why the MSRV check is failing?

alamb · 2024-01-07T12:47:24Z

Not sure why the MSRV check is failing?

You may need to merge up from main -- I think this was fixed on main recently

alamb · 2024-01-09T20:35:35Z

I merged up from main to resolve a merge conflict. Thank you everyone

rspears74 · 2024-01-09T21:35:27Z

Thanks @alamb @jayzhan211 @wjones127 for working through this with me!

Spears Randall added 2 commits December 15, 2023 16:38

Change ScalarValue::List type signature

4bc822b

Also ScalarValue::LargeList and ScalarValue::FixedSizeList

Formatting/cleanup

6938bd6

github-actions bot added physical-expr Physical Expressions optimizer Optimizer rules labels Dec 16, 2023

jayzhan211 requested changes Dec 16, 2023

View reviewed changes

alamb changed the title ~~ScalarValue::List type narrowing~~ Change ScalarValue::{List, LargeList, FixedSizedList} to take specific types rather than ArrayRef Dec 17, 2023

alamb added the api change Changes the API exposed to users of the crate label Dec 17, 2023

This was referenced Dec 17, 2023

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

Closed

DataFusion weekly project plan (Andrew Lamb) - Dec 18, 2023 #8577

Closed

Spears Randall added 3 commits December 18, 2023 09:19

Remove duplicate match statements

a8585b6

Add back scalar eq_array test for List

da1bf38

Formatting

48bfc92

jayzhan211 approved these changes Dec 22, 2023

View reviewed changes

alamb mentioned this pull request Dec 26, 2023

DataFusion weekly project plan (Andrew Lamb) - Dec 25, 2023 #8655

Closed

7 tasks

Weijun-H approved these changes Jan 4, 2024

View reviewed changes

alamb mentioned this pull request Jan 4, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 1, 2024 #8704

Closed

9 tasks

alamb reviewed Jan 5, 2024

View reviewed changes

Spears Randall and others added 2 commits January 5, 2024 17:31

Reduce code duplication

bf1adaf

Merge branch 'main' into scalarvalue-list-sig

5fe9329

Spears Randall added 2 commits January 5, 2024 17:40

Fix merge conflict

73d6776

Fix post-merge compile errors

41f0d36

jayzhan211 approved these changes Jan 6, 2024

View reviewed changes

Remove redundant partial_cmp implementation

c1ec85e

alamb mentioned this pull request Jan 6, 2024

Remove redundant partial_cmp implementation rspears74/arrow-datafusion#1

Merged

improve

2fd67d9

alamb approved these changes Jan 6, 2024

View reviewed changes

Spears Randall and others added 4 commits January 6, 2024 09:41

Cargo fmt fix

84f3fed

Merge pull request #1 from alamb/alamb/less_copy3

7cf1511

Remove redundant partial_cmp implementation

Reduce duplication in formatter

38488af

Reduce more duplication

c212877

wjones127 reviewed Jan 6, 2024

View reviewed changes

Fix test error

588671c

rspears74 and others added 2 commits January 7, 2024 08:52

Merge branch 'apache:main' into scalarvalue-list-sig

2579749

Merge remote-tracking branch 'apache/main' into scalarvalue-list-sig

713374c

alamb merged commit f8d8603 into apache:main Jan 9, 2024
22 checks passed

jayzhan211 mentioned this pull request Jan 13, 2024

Add Scalar to ScalarValue Nested Type #8846

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change `ScalarValue::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` #8562

Change `ScalarValue::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` #8562

rspears74 commented Dec 16, 2023 •

edited

Loading

jayzhan211 Dec 16, 2023

rspears74 Dec 18, 2023

jayzhan211 Dec 19, 2023

jayzhan211 left a comment

jayzhan211 commented Jan 4, 2024

Weijun-H left a comment

Weijun-H Jan 4, 2024

Weijun-H Jan 4, 2024

Weijun-H Jan 4, 2024

Weijun-H Jan 4, 2024

alamb commented Jan 4, 2024

alamb left a comment

alamb Jan 5, 2024

rspears74 Jan 5, 2024

alamb Jan 5, 2024

rspears74 Jan 5, 2024

alamb Jan 6, 2024

rspears74 commented Jan 6, 2024 •

edited

Loading

rspears74 commented Jan 6, 2024

jayzhan211 left a comment

alamb left a comment

alamb Jan 6, 2024

alamb Jan 6, 2024

rspears74 Jan 6, 2024

wjones127 left a comment

wjones127 Jan 6, 2024

rspears74 Jan 6, 2024

jayzhan211 Jan 7, 2024

alamb Jan 9, 2024

rspears74 commented Jan 6, 2024

alamb commented Jan 7, 2024

alamb commented Jan 9, 2024

rspears74 commented Jan 9, 2024

	// ScalarValue List should always have a single element
	// ScalarValue LargeList should always have a single element

	// ScalarValue List should always have a single element
	// ScalarValue FixedSizeList should always have a single element

	Error::General( format!("Error creating temporary batch while encoding ScalarValue::List: {e}"))
	Error::General( format!("Error creating temporary batch while encoding ScalarValue::LargeList: {e}"))

Change ScalarValue::{List, LargeList, FixedSizedList} to take specific types rather than ArrayRef #8562

Change ScalarValue::{List, LargeList, FixedSizedList} to take specific types rather than ArrayRef #8562

Conversation

rspears74 commented Dec 16, 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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 left a comment

Choose a reason for hiding this comment

jayzhan211 commented Jan 4, 2024

Weijun-H left a 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 Jan 4, 2024

alamb left a 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

Choose a reason for hiding this comment

rspears74 commented Jan 6, 2024 • edited Loading

rspears74 commented Jan 6, 2024

jayzhan211 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjones127 left a 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

rspears74 commented Jan 6, 2024

alamb commented Jan 7, 2024

alamb commented Jan 9, 2024

rspears74 commented Jan 9, 2024

Change `ScalarValue::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` #8562

Change `ScalarValue::{List, LargeList, FixedSizedList}` to take specific types rather than `ArrayRef` #8562

rspears74 commented Dec 16, 2023 •

edited

Loading

rspears74 commented Jan 6, 2024 •

edited

Loading