Change ScalarValue::Struct to ArrayRef by jayzhan211 · Pull Request #7893 · apache/datafusion

jayzhan211 · 2023-10-21T03:23:55Z

Which issue does this PR close?

Closes #7835

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

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

wait on #7862

jayzhan211 · 2024-01-06T07:03:18Z

datafusion/proto/tests/cases/roundtrip_logical_plan.rs

-    let should_fail_on_seralize: Vec<ScalarValue> = vec![
-        // Should fail due to empty values
-        ScalarValue::Struct(
-            Some(vec![]),


Move to round_trip_scalar_values, since it is able to serialized

ScalarValue::try_from(&DataType::Struct(Fields::from(vec![ Field::new("a", DataType::Int32, true), Field::new("a", DataType::Boolean, false), ]))) .unwrap(),

I agree that there is no need to test serializing an empty array as it isn't a valid input anyways

jayzhan211 · 2024-01-10T14:04:01Z

@alamb Ready for review!

jayzhan211 · 2024-01-10T14:11:54Z

datafusion/common/src/scalar.rs

                "|                           |",
-                "| {a: , b: }                |",
-                "| {a: , b: {ba: , bb: }}    |",
+                "| {a: 1, b: }               |",


I think there is no way to construct StructArray like the left-hand side.

jayzhan211 · 2024-01-10T14:15:12Z

datafusion/sqllogictest/test_files/explain.slt

+explain select struct(1, 2.3, 'abc');
+----
+logical_plan
+Projection: Struct({c0:Int64(1),c1:Float64(2.3),c2:Utf8("abc")}) AS struct(Int64(1),Float64(2.3),Utf8("abc"))


test format

jayzhan211 · 2024-01-10T14:16:38Z

datafusion/common/src/scalar.rs

-            .into_iter()
-            .map(|(name, scalar)| (Field::new(name, scalar.data_type(), false), scalar))
-            .unzip();
+// Wrapper for ScalarValue::Struct that checks the length of the arrays, without nulls


TODO: Remove these two wrappers, no longer needed after changing to Scalar<T>

Are these still TODO?

Yes, we haven't changed to Scalar yet.

jayzhan211 · 2024-01-10T14:23:03Z

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

-        let struct_type = DataType::Struct(Fields::from(fields));
+        let mut column_wise_ordering_values = vec![];
+        let num_columns = fields.len();
+        for i in 0..num_columns {


I think there might be a better design for StructArray (previous design is based on old ScalarValue::Struct). I avoid changing the logic or data structure in this PR.

May benefit #8558?

cc @mustafasrepo

I think there might be a better design for StructArray (previous design is based on old ScalarValue::Struct). I avoid changing the logic or data structure in this PR.

May benefit #8558?

I don't think it will benefit #8558 (won't harm either). However it will be a better change anyway.

Weijun-H · 2024-01-10T14:37:57Z

datafusion/core/src/physical_planner.rs

    }

-    /// Return a `null` literal representing a struct type like: `{ a: bool }`
+    // / Return a `null` literal representing a struct type like: `{ a: bool }`


nit:

Suggested change

// / Return a `null` literal representing a struct type like: `{ a: bool }`

/// Return a `null` literal representing a struct type like: `{ a: bool }`

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

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

+
                            Ok(ordering_columns_per_row)
                        } else {
                            exec_err!(


todo: internal_err

jayzhan211 · 2024-01-29T11:56:18Z

Rebase

alamb · 2024-02-02T21:49:45Z

@jayzhan211 -- is this PR ready for a review?

jayzhan211 · 2024-02-02T23:53:30Z

@jayzhan211 -- is this PR ready for a review?

Yes, it keeps getting conflicts, but I think you can take a first scan, unless the conflicts are critical

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

jayzhan211 · 2024-02-05T13:42:58Z

Rebase

alamb · 2024-02-05T15:40:36Z

Sorry -- starting to look now

alamb

This is looking really good @jayzhan211 -- thank you both for the PR as well as for sticking with it for so long

I had a few comments about how to improve the implementation by using arrow kernels, but I also think we could merge this as is and then implement those improvements as a follow on PR if you prefer.

Again, thank you for your patience.

datafusion/core/tests/user_defined/user_defined_aggregates.rs

alamb · 2024-02-05T15:49:24Z

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

@@ -323,20 +335,32 @@ impl Accumulator for OrderSensitiveArrayAggAccumulator {
 impl OrderSensitiveArrayAggAccumulator {
    fn evaluate_orderings(&self) -> Result<ScalarValue> {


Maybe we can file a follow on ticket to track this idea

datafusion/proto/src/logical_plan/to_proto.rs

alamb · 2024-02-05T15:53:54Z

datafusion/proto/tests/cases/roundtrip_logical_plan.rs

-    let should_fail_on_seralize: Vec<ScalarValue> = vec![
-        // Should fail due to empty values
-        ScalarValue::Struct(
-            Some(vec![]),


I agree that there is no need to test serializing an empty array as it isn't a valid input anyways

alamb · 2024-02-05T15:58:37Z

datafusion/common/src/scalar.rs

+                );
+            }
+
+            let mut valid = BooleanBufferBuilder::new(arrays.len());


What do you think about using https://docs.rs/arrow/latest/arrow/compute/kernels/concat/index.html here? I think you should be able to simply concat the arrays together without having to have special handling (and if concat doesn't support StructArray we can potentially file a ticket upstream in arrow-rs)

datafusion/common/src/scalar.rs

alamb · 2024-02-05T16:04:39Z

datafusion/common/src/scalar.rs

-            .into_iter()
-            .map(|(name, scalar)| (Field::new(name, scalar.data_type(), false), scalar))
-            .unzip();
+// Wrapper for ScalarValue::Struct that checks the length of the arrays, without nulls


Are these still TODO?

datafusion/common/src/scalar.rs

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

alamb · 2024-02-06T12:14:45Z

Thanks @jayzhan211 -- this is looking great. There are a few more outstanding suggestions, but I think we could do them as follow on PRs -- shall I merge this one?

jayzhan211 · 2024-02-06T13:46:44Z

Thanks @jayzhan211 -- this is looking great. There are a few more outstanding suggestions, but I think we could do them as follow on PRs -- shall I merge this one?

Sure!

alamb · 2024-02-07T12:58:21Z

Thanks again @jayzhan211

github-actions bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Oct 21, 2023

jayzhan211 force-pushed the arrayref-for-scalarvalue-struct branch from ac9ffc3 to e27d7f9 Compare November 12, 2023 02:27

github-actions bot removed logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) labels Nov 12, 2023

jayzhan211 force-pushed the arrayref-for-scalarvalue-struct branch from e27d7f9 to 5a5a88d Compare December 9, 2023 11:13

jayzhan211 mentioned this pull request Dec 13, 2023

ARRAY_AGG of column of type list ORDER BY column of type non-list #8512

Closed

jayzhan211 force-pushed the arrayref-for-scalarvalue-struct branch from 5a5a88d to a006450 Compare December 14, 2023 12:37

jayzhan211 mentioned this pull request Dec 15, 2023

Minor: Introduce utils::hash for StructArray #8552

Merged

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

jayzhan211 force-pushed the arrayref-for-scalarvalue-struct branch 2 times, most recently from e57eca5 to 1c82d51 Compare December 18, 2023 00:21

jayzhan211 force-pushed the arrayref-for-scalarvalue-struct branch 2 times, most recently from addf685 to bd98b9a Compare January 6, 2024 06:19

jayzhan211 commented Jan 6, 2024

View reviewed changes

jayzhan211 force-pushed the arrayref-for-scalarvalue-struct branch 2 times, most recently from c786df7 to 60f4d2a Compare January 10, 2024 13:27

jayzhan211 marked this pull request as ready for review January 10, 2024 13:48

jayzhan211 commented Jan 10, 2024

View reviewed changes

Weijun-H reviewed Jan 10, 2024

View reviewed changes

alamb mentioned this pull request Jan 10, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 8, 2024 #8786

Closed

7 tasks

jayzhan211 commented Jan 11, 2024

View reviewed changes

jayzhan211 requested a review from alamb January 13, 2024 02:10

jayzhan211 force-pushed the arrayref-for-scalarvalue-struct branch from 3f53ead to 98afb20 Compare January 29, 2024 11:56

jayzhan211 force-pushed the arrayref-for-scalarvalue-struct branch from e431707 to a756a8b Compare January 30, 2024 14:14

alamb mentioned this pull request Feb 4, 2024

DataFusion weekly project plan (Andrew Lamb) - Feb 5, 2024 #9121

Closed

6 tasks

jayzhan211 added 7 commits February 5, 2024 21:14

first draft

a6ebe74

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

cleanup

2c2503c

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

rebase

177e72b

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

todo

9efa792

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

revert back to_scalar

b18eb25

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

cleanup

608bdd7

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

fmt

4088750

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

jayzhan211 force-pushed the arrayref-for-scalarvalue-struct branch from a756a8b to 4088750 Compare February 5, 2024 13:15

alamb approved these changes Feb 5, 2024

View reviewed changes

jayzhan211 added 5 commits February 6, 2024 10:39

fix debug

8e8233f

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

fix display

fcce7f6

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

fix try from array

7e7e8bb

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

comment

6b6f6e2

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

fix state types

1a8a29c

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

alamb mentioned this pull request Feb 7, 2024

Use concat to simplify Struct scalar creation #9145

Closed

alamb merged commit 8413da8 into apache:main Feb 7, 2024

This was referenced Feb 9, 2024

Use concat to simplify Nested Scalar creation #9174

Merged

Easier way to create ScalarValue::Struct #9227

Closed

Improve documentation on how to build ScalarValue::Struct and add ScalarStructBuilder #9229

Merged

	// / Return a `null` literal representing a struct type like: `{ a: bool }`
	/// Return a `null` literal representing a struct type like: `{ a: bool }`

		@@ -323,20 +335,32 @@ impl Accumulator for OrderSensitiveArrayAggAccumulator {
		impl OrderSensitiveArrayAggAccumulator {
		fn evaluate_orderings(&self) -> Result<ScalarValue> {

Conversation

jayzhan211 commented Oct 21, 2023

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 Nov 28, 2023

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayzhan211 commented Jan 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayzhan211 Jan 10, 2024 • 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 Jan 10, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayzhan211 commented Jan 29, 2024

Uh oh!

alamb commented Feb 2, 2024

Uh oh!

jayzhan211 commented Feb 2, 2024

Uh oh!

jayzhan211 commented Feb 5, 2024

Uh oh!

alamb commented Feb 5, 2024

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alamb commented Feb 6, 2024

Uh oh!

jayzhan211 commented Feb 6, 2024

Uh oh!

alamb commented Feb 7, 2024

Uh oh!

Reviewers

Assignees

Labels

jayzhan211 Jan 10, 2024 •

edited

Loading

jayzhan211 Jan 10, 2024 •

edited

Loading