Move Coercion for MakeArray to `coerce_arguments_for_signature` and introduce another one for ArrayAppend #8317

jayzhan211 · 2023-11-25T06:26:45Z

Which issue does this PR close?

Ref #7142
Close #7995

Rationale for this change

Fix Null Handing for array function. ArrayAppend first, others later.

What changes are included in this PR?

Move coercion for MakeArray to different place.
Add signature for ArrayAppend

Are these changes tested?

MakeArray - existing test
ArrayAppend - new test

Are there any user-facing changes?

jayzhan211 · 2023-11-25T06:31:13Z

datafusion/expr/src/type_coercion/functions.rs

        TypeSignature::Exact(valid_types) => vec![valid_types.clone()],
+        TypeSignature::ArrayAppendLikeSignature => {


Have a specialized checking for array append at the end. And I think we will need specialized check for other pattern too

jayzhan211 · 2023-11-25T06:32:03Z

datafusion/expr/src/type_coercion/functions.rs

@@ -181,7 +239,7 @@ fn coerced_from<'a>(
        Int64
            if matches!(
                type_from,
-                Null | Int8 | Int16 | Int32 | Int64 | UInt8 | UInt16 | UInt32
+                Null | Int8 | Int16 | Int32 | Int64 | UInt8 | UInt16 | UInt32 | Boolean


Only i64 is fixed for passing existing test in array.slt. More types should be fixed in #8302

jayzhan211 · 2023-11-28T13:22:06Z

wait on #8318 and #8331

jayzhan211 · 2023-12-04T01:24:47Z

datafusion/expr/src/type_coercion/functions.rs

+                    if let Some(corced_type) = corced_type {
+                        Ok(corced_type)
+                    } else {
+                        internal_err!("Coercion from {acc:?} to {x:?} failed.")


We call unwrap_or previously so select [1, true, null] unexpectedly correct since true is castable to 1 in arrow-rs but not in datafusion. select [true, 1, null] failed. It is better that we just return error if not coercible.

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

jayzhan211 · 2023-12-07T23:31:35Z

@alamb Ready for review! I hope this is a better solution for dealing with nulls. I had tried Trait SignatureComputation, but it is somewhat overly complicated.

alamb

Thank you @jayzhan211 -- this is very much in the right direction I think. I had some suggestions / comments about some of the specific cases but it is looking very close

alamb · 2023-12-08T12:13:46Z

datafusion/expr/src/signature.rs

@@ -95,6 +95,8 @@ pub enum TypeSignature {
    VariadicEqual,
    /// One or more arguments with arbitrary types
    VariadicAny,
+    /// A function such as `make_array` should be coerced to the same type
+    VariadicCoerced,


Can you explain how this is different than VariadicEqual? It seems from the comments here they are quite similar 🤔

I just noticed that VariadicEqual is not used in any function. Maybe we can just keep one.

I thought Equal is the one that don't care about coercion. All the type should be equal like (i32, i32, i32).
Coerced is the one that ensuring the final coerced type is the same (all of the type coercible to the same one), like (i32, i64) -> i64.

I think current VariadicCoerced includes VariadicEqual use case as well. We can just have one signature

alamb · 2023-12-08T12:20:44Z

datafusion/expr/src/signature.rs

@@ -113,6 +115,8 @@ pub enum TypeSignature {
    /// Function `make_array` takes 0 or more arguments with arbitrary types, its `TypeSignature`
    /// is `OneOf(vec![Any(0), VariadicAny])`.
    OneOf(Vec<TypeSignature>),
+    /// Specialized Signature for ArrayAppend and similar functions


What do you think about using a more generic name. Perhaps something like

/// The first argument is an array type ([`DataType::List`], or [`DataType::LargeList`] /// and the subsequent arguments are coerced to the List's element type /// /// For example a call to `func(a: List(int64), b: int32, c: utf8)` would attempt to coerce /// all the arguments to `int64`: /// ``` /// func(a: List(int64), cast(b as int64): int64, cast(c as int64): int64) /// ``` ArrayAndElements

There may be more general ways of expressing the array function types too 🤔

alamb · 2023-12-08T12:25:05Z

datafusion/optimizer/src/analyzer/type_coercion.rs

@@ -590,26 +590,6 @@ fn coerce_arguments_for_fun(
            .collect::<Result<Vec<_>>>()?;
    }

-    if *fun == BuiltinScalarFunction::MakeArray {


alamb · 2023-12-08T12:27:51Z

datafusion/sqllogictest/test_files/array.slt

@@ -265,10 +265,8 @@ AS VALUES
  (make_array([28, 29, 30], [31, 32, 33], [34, 35, 36], [28, 29, 30], [31, 32, 33], [34, 35, 36], [28, 29, 30], [31, 32, 33], [34, 35, 36], [28, 29, 30]), [28, 29, 30], [37, 38, 39], 10)
 ;

-query ?
+query error
 select [1, true, null]


this is an error because true can't be coerced to an integer, right? FWIW I think that is fine and is consistent with the postgres rues:

postgres=# select array[1, true, null]; ERROR: ARRAY types integer and boolean cannot be matched LINE 1: select array[1, true, null];

alamb · 2023-12-08T12:29:57Z

datafusion/sqllogictest/test_files/array.slt

-# [[]] [[4]]
+query ???????
+select
+  array_append(null, 1),


Do we want to support array_append(null, ...)?

Postgres does not allow this:

postgres=# select array_append(null, array[2,3]); ERROR: could not find array type for data type integer[]

It also does't try to find a fancy type with an empty list:

postgres=# select array_append(array[], array[2,3]); ERROR: cannot determine type of empty array LINE 1: select array_append(array[], array[2,3]); ^ HINT: Explicitly cast to the desired type, for example ARRAY[]::integer[].

🤔

Summary of other sql behavior

Duckdb: [[2, 3]], [[2, 3]]
ClickHouse: null, [[2, 3]]
Postgres: error, error

I think for array_append([], [2,3]), it is fine to not follow postgres and return [[2, 3]] like clickhouse and duckdb.
For array_append(null, [2, 3]), I think we can follow postrgres.

Clickhouse and Duckdb has the same output for array_append(make_array(null, null), 1) too.

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

alamb

Looks good to me -- thank you @jayzhan211 .

alamb · 2023-12-18T19:16:00Z

I merged this branch up from main to ensure there are no logical conflicts, and if all tests pass I intend to merge it

github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Nov 25, 2023

jayzhan211 changed the title ~~Move Coercion for MakeArray while verifying signature and introduce signature verification for ArrayAppend~~ Move Coercion for MakeArray to coerce_arguments_for_signature and introduce another one for ArrayAppend Nov 25, 2023

jayzhan211 commented Nov 25, 2023

View reviewed changes

jayzhan211 marked this pull request as draft November 25, 2023 06:56

This was referenced Nov 25, 2023

array_ndims doesn't correctly handle List(Null) #8318

Closed

fix array_append with null array #8333

Closed

jayzhan211 mentioned this pull request Dec 2, 2023

Improve type coercion and casting #8302

Open

jayzhan211 force-pushed the signature-for-array branch 3 times, most recently from 0dbfaab to 8483b76 Compare December 4, 2023 01:18

jayzhan211 marked this pull request as ready for review December 4, 2023 01:18

jayzhan211 commented Dec 4, 2023

View reviewed changes

jayzhan211 marked this pull request as draft December 4, 2023 05:54

jayzhan211 marked this pull request as ready for review December 4, 2023 12:17

Signature for array_append and make_array

3ba90b4

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

jayzhan211 force-pushed the signature-for-array branch from 1467fda to 3ba90b4 Compare December 7, 2023 23:26

alamb reviewed Dec 8, 2023

View reviewed changes

alamb mentioned this pull request Dec 8, 2023

DataFusion weekly project plan (Andrew Lamb) - Dec 4, 2023 #8420

Closed

7 tasks

jayzhan211 added 2 commits December 9, 2023 08:54

combine variadicequal and coerced to equal

32c9931

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

follow postgres style on array_append(null, T)

f760de5

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

jayzhan211 requested a review from alamb December 9, 2023 02:00

jayzhan211 added 3 commits December 9, 2023 11:17

update comment for ArrayAndElement

6384a05

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

remove test

3686a2f

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

add more test

3226add

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

alamb mentioned this pull request Dec 11, 2023

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

Closed

8 tasks

alamb mentioned this pull request Dec 18, 2023

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

Closed

7 tasks

alamb approved these changes Dec 18, 2023

View reviewed changes

Merge remote-tracking branch 'apache/main' into signature-for-array

1045066

alamb merged commit 1935c58 into apache:main Dec 18, 2023
22 checks passed

jayzhan211 mentioned this pull request Dec 21, 2023

All array functions should represent NULL as an element #7142

Open

matthewgapp mentioned this pull request Jan 11, 2024

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

Closed

Weijun-H mentioned this pull request Jan 18, 2024

Minor: remove null in array-append and array-prepend #8901

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Coercion for MakeArray to `coerce_arguments_for_signature` and introduce another one for ArrayAppend #8317

Move Coercion for MakeArray to `coerce_arguments_for_signature` and introduce another one for ArrayAppend #8317

jayzhan211 commented Nov 25, 2023 •

edited

Loading

jayzhan211 Nov 25, 2023

jayzhan211 Nov 25, 2023 •

edited

Loading

jayzhan211 commented Nov 28, 2023

jayzhan211 Dec 4, 2023

jayzhan211 commented Dec 7, 2023 •

edited

Loading

alamb left a comment

alamb Dec 8, 2023

jayzhan211 Dec 9, 2023

jayzhan211 Dec 9, 2023

alamb Dec 8, 2023

alamb Dec 8, 2023

alamb Dec 8, 2023

jayzhan211 Dec 13, 2023

alamb Dec 8, 2023

jayzhan211 Dec 9, 2023

jayzhan211 Dec 9, 2023

jayzhan211 Dec 9, 2023

alamb left a comment

alamb commented Dec 18, 2023

		TypeSignature::Exact(valid_types) => vec![valid_types.clone()],
		TypeSignature::ArrayAppendLikeSignature => {

Move Coercion for MakeArray to coerce_arguments_for_signature and introduce another one for ArrayAppend #8317

Move Coercion for MakeArray to coerce_arguments_for_signature and introduce another one for ArrayAppend #8317

Conversation

jayzhan211 commented Nov 25, 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

jayzhan211 Nov 25, 2023 • edited Loading

Choose a reason for hiding this comment

jayzhan211 commented Nov 28, 2023

Choose a reason for hiding this comment

jayzhan211 commented Dec 7, 2023 • edited Loading

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

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

Choose a reason for hiding this comment

alamb commented Dec 18, 2023

Move Coercion for MakeArray to `coerce_arguments_for_signature` and introduce another one for ArrayAppend #8317

Move Coercion for MakeArray to `coerce_arguments_for_signature` and introduce another one for ArrayAppend #8317

jayzhan211 commented Nov 25, 2023 •

edited

Loading

jayzhan211 Nov 25, 2023 •

edited

Loading

jayzhan211 commented Dec 7, 2023 •

edited

Loading