Unnest with single expression #9069

jayzhan211 · 2024-01-30T12:53:55Z

Which issue does this PR close?

Revisit from #6796, only single expression included in this PR.

Closes #.

Rationale for this change

Ref #6555

What changes are included in this PR?

Two types of unnest expressions

select unnest([1,2,3])
select unnest(column)

Are these changes tested?

Are there any user-facing changes?

comphead

that is really nice initiative, thanks @jayzhan211, I'll review it this week if no one else beats me

alamb

Thank you @jayzhan211 -- this PR looks awesome -- thank you.

I think it needs a few more tests and avoiding a panic, but otherise 👨‍🍳 👌

Thanks again.

alamb · 2024-01-31T22:04:16Z

datafusion/sql/src/select.rs

+                .build()
+        } else {
+            // Only support single unnest expression for now
+            assert_eq!(array_exprs_to_unnest.len(), 1);


Could we please make this return NotYetImplemented rather than panic?

alamb · 2024-01-31T22:04:55Z

datafusion/sqllogictest/test_files/unnest.slt

+6
+
+statement ok
+drop table unnest_table;


Awesome -- can you please also add tests for:

to unnest two columns (it is fine to error, but I think that behavior should be tested)

trying to unnest a scalar value 1 rather than [1]

to unnest zero columns (again should be an error)

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

jayzhan211 · 2024-02-01T01:26:51Z

datafusion/sql/src/expr/function.rs

+                    }) = exprs[0]
+                    {
+                        // valid
+                    } else if let Expr::Column(_) = exprs[0] {


arguments handling in Column seems not possible to handle anywhere before transforming to LogicalPlan. We should validate arguments inside UnnestExec or elsewhere downstream.

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

comphead · 2024-02-01T16:06:10Z

datafusion/core/src/physical_planner.rs

@@ -124,6 +124,9 @@ fn physical_name(e: &Expr) -> Result<String> {

 fn create_physical_name(e: &Expr, is_first_expr: bool) -> Result<String> {
    match e {
+        Expr::Unnest(_) => {
+            internal_err!("Expr::Unnest should have been converted to LogicalPlan::Unest")


Suggested change

internal_err!("Expr::Unnest should have been converted to LogicalPlan::Unest")

internal_err!("Expr::Unnest should have been converted to LogicalPlan::Unnest")

comphead · 2024-02-01T16:09:30Z

datafusion/expr/src/expr_schema.rs

@@ -82,6 +82,13 @@ impl ExprSchemable for Expr {
            Expr::Case(case) => case.when_then_expr[0].1.get_type(schema),
            Expr::Cast(Cast { data_type, .. })
            | Expr::TryCast(TryCast { data_type, .. }) => Ok(data_type.clone()),
+            Expr::Unnest(Unnest { exprs }) => {
+                let arg_data_types = exprs


do we need iter here as we refer for the first element only?

I think we can keep this, since there will be multiple exprs in the future

comphead · 2024-02-01T16:12:35Z

datafusion/sql/src/expr/function.rs

+                        return not_impl_err!("unnest() does not support struct yet");
+                    } else {
+                        return plan_err!(
+                            "unnest() can only be applied to array and structs and null"


this line says unnest supports struct but line above says the opposite

first line is struct nyi error.
second line is to tell you that only those 3 are valid.

thats the confusing part. not_impl_err!("unnest() does not support struct yet"); - here the user can get the struct is not supported by unnest.
plan_err!("unnest() can only be applied to array and structs and null") here the message says that unnest support ONLY array, structs and nulls which confronts with first err message

Maybe I'm missing something?

comphead · 2024-02-01T16:14:20Z

datafusion/sqllogictest/test_files/unnest.slt

+    ([6], [11,12], 3)
+;
+
+query I


please add some test description

comphead · 2024-02-01T16:14:37Z

datafusion/sqllogictest/test_files/unnest.slt

+----
+
+query ?
+select column1 from unnest_table;


do we need this test?

comphead · 2024-02-01T16:15:02Z

datafusion/sqllogictest/test_files/unnest.slt

+statement ok
+CREATE TABLE unnest_table
+AS VALUES
+    ([1,2,3], [7], 1),


please include nulls, empty arrays into the table

I added nulls in 79eb32d to keep this PR moving

comphead

Thanks @jayzhan211 I left some comments, nice job
Back in the day I tried to implement unnest but went to wrong direction through builtin function and eventually gave up. Your approach with expression is good

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

alamb

Thanks @jayzhan211 -- I think this PR is now ready to go. It looks like @comphead has a few more test suggestions in https://github.com/apache/arrow-datafusion/pull/9069/files#r1474726470 but I think we could also potentially add those tests as a follow on PR given this PR does not seem have special handling for them

alamb · 2024-02-04T12:16:42Z

datafusion/sqllogictest/test_files/unnest.slt

+statement ok
+CREATE TABLE unnest_table
+AS VALUES
+    ([1,2,3], [7], 1),


I added nulls in 79eb32d to keep this PR moving

alamb · 2024-02-04T13:24:56Z

Thanks again @jayzhan211 -- this is really nice 👌

github-actions bot added sql SQL Planner logical-expr Logical plan and expressions optimizer Optimizer rules core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Jan 30, 2024

jayzhan211 marked this pull request as ready for review January 30, 2024 14:13

comphead reviewed Jan 30, 2024

View reviewed changes

alamb mentioned this pull request Jan 30, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 29, 2024 #9030

Closed

6 tasks

alamb reviewed Jan 31, 2024

View reviewed changes

jayzhan211 added 10 commits February 1, 2024 09:22

introduce basic unnest

e09fe0f

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

proto

ad0689f

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

cleanup

a92d22e

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

cleanup

c382ebe

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

cleanup

800ecc5

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

rename

c5569e2

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

change panic to internal error

e61e5b6

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

cleanup

ae82257

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

typo

3d79b48

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

more err handling

eaf69fa

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

jayzhan211 force-pushed the unnest-basic branch from ee3dd80 to eaf69fa Compare February 1, 2024 01:22

jayzhan211 commented Feb 1, 2024

View reviewed changes

jayzhan211 requested review from comphead and alamb February 1, 2024 01:28

fix reimported

d2a86bc

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

comphead reviewed Feb 1, 2024

View reviewed changes

datafusion/sqllogictest/test_files/unnest.slt

([6], [11,12], 3)

;

query I

Copy link

Contributor

comphead Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add some test description

comphead reviewed Feb 1, 2024

View reviewed changes

datafusion/sqllogictest/test_files/unnest.slt Outdated

----

query ?

select column1 from unnest_table;

Copy link

Contributor

comphead Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this test?

comphead reviewed Feb 1, 2024

View reviewed changes

comments and fix typo

13ab024

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

alamb approved these changes Feb 2, 2024

View reviewed changes

alamb added 2 commits February 4, 2024 07:16

Add tests for nulls

79eb32d

Merge remote-tracking branch 'apache/main' into unnest-basic

d79d010

alamb approved these changes Feb 4, 2024

View reviewed changes

alamb merged commit 24197d7 into apache:main Feb 4, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unnest with single expression #9069

Unnest with single expression #9069

jayzhan211 commented Jan 30, 2024

comphead left a comment

alamb left a comment

alamb Jan 31, 2024

alamb Jan 31, 2024

jayzhan211 Feb 1, 2024

comphead Feb 1, 2024

comphead Feb 1, 2024

jayzhan211 Feb 1, 2024 •

edited

Loading

comphead Feb 1, 2024

jayzhan211 Feb 1, 2024

comphead Feb 2, 2024

comphead Feb 1, 2024

comphead Feb 1, 2024

comphead Feb 1, 2024

alamb Feb 4, 2024

comphead left a comment

alamb left a comment

alamb Feb 4, 2024

alamb commented Feb 4, 2024

	internal_err!("Expr::Unnest should have been converted to LogicalPlan::Unest")
	internal_err!("Expr::Unnest should have been converted to LogicalPlan::Unnest")

Unnest with single expression #9069

Unnest with single expression #9069

Conversation

jayzhan211 commented Jan 30, 2024

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?

comphead 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 Feb 1, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comphead 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

alamb commented Feb 4, 2024

jayzhan211 Feb 1, 2024 •

edited

Loading