fix: `array_concat` with arrays with different dimensions, add `_list` aliases for `_array` functions #7008

izveigor · 2023-07-18T09:20:52Z

Which issue does this PR close?

Closes #6992
Closes #7028
Follow on to #6879

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Yes

Are there any user-facing changes?

Yes

… properly

jayzhan211 · 2023-07-19T10:25:03Z

datafusion/core/tests/sqllogictests/test_files/array.slt

@@ -149,7 +149,7 @@ select column1, column2, column3 from arrays_values_without_nulls;
 ### Array function tests


-## make_array
+## make_array (aliases: `make_list`)


Any reason to have the alias make_list, may it be confused with creating ListArray?

@jayzhan211 I have identified several reasons:

The idea was taken from popular database: DuckDB. So, in my opinion, users of this database can more easily adapt to our array functions. (Documentation: https://duckdb.org/docs/sql/functions/nested).

array and list prefix or postfix can separate mutable or immutable objects. (See discussion: The concept of practical implementation of the array #6855 (About aliases)).

Some people prefer to use one prefix/postfix over another.

izveigor · 2023-07-19T11:14:20Z

datafusion/expr/src/built_in_function.rs

@@ -430,7 +430,22 @@ impl BuiltinScalarFunction {
        )
    }

-    /// Returns the output [`DataType` of this function
+    /// Returns the dimension [`DataType`] of [`DataType::List`]
+    fn return_dimension(self, input_expr_type: DataType) -> u64 {


This is the main codebase, other changes are documentation improvements and new aliases.

izveigor · 2023-07-19T11:14:33Z

datafusion/expr/src/built_in_function.rs

                for input_expr_type in input_expr_types {
                    match input_expr_type {
                        List(field) => {
                            if !field.data_type().equals_datatype(&Null) {
-                                expr_type = field.data_type().clone();
-                                break;
+                                let dims = self.return_dimension(input_expr_type.clone());


This is the main codebase, other changes are documentation improvements and new aliases.

izveigor · 2023-07-19T11:23:08Z

@alamb @jayzhan211 I wonder if you have free time to review the PR.

jayzhan211

Done the review

jayzhan211 · 2023-07-19T12:33:32Z

datafusion/expr/src/built_in_function.rs

@@ -464,12 +479,16 @@ impl BuiltinScalarFunction {
            },
            BuiltinScalarFunction::ArrayConcat => {
                let mut expr_type = Null;
+                let mut max_dims = 1;


If the init value is 1 and all the lists are 1D, we will get null for expr_type, right? But why doesn't the 1D concat test fails?

init value for expr_type = null, and max_dims = 0, with updating expr_type with input_expr_type instead of field since much more straightforward to me.

Implemented your idea 👌.

jayzhan211 · 2023-07-19T12:50:07Z

datafusion/expr/src/built_in_function.rs

@@ -464,12 +479,16 @@ impl BuiltinScalarFunction {
            },
            BuiltinScalarFunction::ArrayConcat => {
                let mut expr_type = Null;
+                let mut max_dims = 1;
                for input_expr_type in input_expr_types {
                    match input_expr_type {
                        List(field) => {
                            if !field.data_type().equals_datatype(&Null) {


Is it possible to get a nested list array with a null type? i.e. List(List(Null))

Good catch! There is a bug #7028. Thanks, @jayzhan211!

alamb · 2023-07-19T21:02:51Z

I will plan to review this PR tomorrow -- thank you @izveigor

jayzhan211

LGTM!

alamb

This looks great to me @izveigor and @jayzhan211 -- thank you very much. I have some suggestions for comments but I can make a follow on PR if you prefer.

Thanks again

alamb · 2023-07-20T17:57:02Z

datafusion/expr/src/built_in_function.rs

@@ -430,7 +430,22 @@ impl BuiltinScalarFunction {
        )
    }

-    /// Returns the output [`DataType` of this function
+    /// Returns the dimension [`DataType`] of [`DataType::List`]


Suggested change

/// Returns the dimension [`DataType`] of [`DataType::List`]

/// Returns the dimension [`DataType`] of [`DataType::List`].

///

/// Dimension is defined as the deepest level of nesting.

/// * `Int64` has dimension 1

/// * `List(Int64)` has dimension 2

/// * `List(List(Int64))` has dimension 3

/// * etc.

Proposed PR to add this documentation: #7045

alamb · 2023-07-20T17:58:23Z

datafusion/physical-expr/src/array_expressions.rs

+    Ok(compute_array_ndims_with_datatype(arr)?.0)
+}
+
+/// Returns the dimension and lower datatype of the array


Suggested change

/// Returns the dimension and lower datatype of the array

/// Returns the dimension and the datatype of elements of the array

fix: array_concat with arrays with different dimensions does not work…

b541c49

… properly

izveigor marked this pull request as draft July 18, 2023 09:20

github-actions bot added logical-expr Logical plan and expressions core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Jul 18, 2023

fix: cargo doc

c9db975

izveigor marked this pull request as ready for review July 18, 2023 12:59

jayzhan211 reviewed Jul 19, 2023

View reviewed changes

izveigor commented Jul 19, 2023

View reviewed changes

Merge branch 'main' into improve_array_concat_input

b0e3774

jayzhan211 reviewed Jul 19, 2023

View reviewed changes

fix: array_concat does not work with nested empty arrays

ced6901

github-actions bot added the physical-expr Physical Expressions label Jul 19, 2023

jayzhan211 approved these changes Jul 19, 2023

View reviewed changes

alamb changed the title ~~fix: array_concat with arrays with different dimensions~~ fix: array_concat with arrays with different dimensions, add _list* aliases for _array* functions Jul 20, 2023

alamb approved these changes Jul 20, 2023

View reviewed changes

fix: some comments

68a6bed

alamb mentioned this pull request Jul 20, 2023

Replace array_contains with SQL array functions: array_has, array_has_any, array_has_all #6990

Merged

alamb merged commit 5907c21 into apache:main Jul 20, 2023
21 checks passed

alamb mentioned this pull request Jul 20, 2023

Minor: Refine doc comments for BuiltinScalarFunction::return_dimension #7045

Merged

izveigor mentioned this pull request Jul 30, 2023

Improve handling of NULL and empty array for array functions. #7143

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: `array_concat` with arrays with different dimensions, add `_list` aliases for `_array` functions #7008

fix: `array_concat` with arrays with different dimensions, add `_list` aliases for `_array` functions #7008

izveigor commented Jul 18, 2023 •

edited

Loading

jayzhan211 Jul 19, 2023

izveigor Jul 19, 2023

izveigor Jul 19, 2023

izveigor Jul 19, 2023

izveigor commented Jul 19, 2023

jayzhan211 left a comment

jayzhan211 Jul 19, 2023

izveigor Jul 19, 2023

jayzhan211 Jul 19, 2023

izveigor Jul 19, 2023

alamb commented Jul 19, 2023

jayzhan211 left a comment

alamb left a comment

alamb Jul 20, 2023

alamb Jul 20, 2023

alamb Jul 20, 2023

-    /// Returns the dimension [`DataType`] of [`DataType::List`]
+    /// Returns the dimension [`DataType`] of [`DataType::List`].
+    ///
+    /// Dimension is defined as the deepest level of nesting.
+    /// * `Int64` has dimension 1
+    /// * `List(Int64)` has dimension 2
+    /// * `List(List(Int64))` has dimension 3
+    /// * etc.

	/// Returns the dimension and lower datatype of the array
	/// Returns the dimension and the datatype of elements of the array

fix: array_concat with arrays with different dimensions, add _list* aliases for _array* functions #7008

fix: array_concat with arrays with different dimensions, add _list* aliases for _array* functions #7008

Conversation

izveigor commented Jul 18, 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

Choose a reason for hiding this comment

izveigor commented Jul 19, 2023

jayzhan211 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 Jul 19, 2023

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

fix: `array_concat` with arrays with different dimensions, add `_list` aliases for `_array` functions #7008

fix: `array_concat` with arrays with different dimensions, add `_list` aliases for `_array` functions #7008

izveigor commented Jul 18, 2023 •

edited

Loading