feat: support `NULL` in array functions #6662

izveigor · 2023-06-13T18:50:48Z

Which issue does this PR close?

Closes #6556

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Yes

Are there any user-facing changes?

Yes

izveigor · 2023-06-19T19:01:21Z

@alamb can you review this PR if you have time.
I have some problems with ArrayFill function. To get its return type, we should know the length of its second element (List). So, I think we should create a separate ticket to discuss possible solutions..

alamb · 2023-06-20T21:30:55Z

Hi @izveigor -- I will review this tomorrow. Sorry I am behind

alamb

Thank you for this @izveigor -- I didn't go through this entire PR carefully, but I am not sure about the approach to null handling. I tried to explain how the rest of DataFusion handles this -- hopefully it is clear

alamb · 2023-06-21T18:23:12Z

datafusion/expr/src/built_in_function.rs

+                Ok(List(Arc::new(Field::new("item", expr_type, true))))
+            }
+            BuiltinScalarFunction::ArrayDims => Ok(UInt8),
+            BuiltinScalarFunction::ArrayFill => Ok(Null),


I don't understand why ArrayFill always returns Null as its data type

My reading of https://www.postgresql.org/docs/9.1/functions-array.html suggests that it should return something like List(args[0].type)

The function returns nested list:

postgres=# select array_fill(3, array[2, 3, 2]); array_fill ------------------------------------------- {{{3,3},{3,3},{3,3}},{{3,3},{3,3},{3,3}}} (1 row)

Therefore it should return nested list. I think this is the serious problem because to return nested list with right dimensions we should know the length of the second argument (list).
P.S. after the changes: #6595 it does not work.

alamb · 2023-06-21T18:25:00Z

datafusion/optimizer/src/analyzer/type_coercion.rs

+    to_type: &DataType,
+    schema: &DFSchema,
+) -> Result<Expr> {
+    if from_type.equals_datatype(&DataType::Null) {


I would have thought we should be casting all the arguments that are null to the specific type of the rest of the arguments...

For reference, this is the first way to solve the problem (see below).

alamb · 2023-06-21T18:26:18Z

datafusion/expr/src/built_in_function.rs

+                            }
+                        }
+                        _ => {
+                            return Err(DataFusionError::Internal(format!(


I think Internal errors are only intended for bugs in DataFusion -- this error seems like it could come from bad user input too

Suggested change

return Err(DataFusionError::Internal(format!(

return Err(DataFusionError::Plan(format!(

I think I should consider other functions as well.

Indeed: #6108

alamb · 2023-06-21T18:27:59Z

datafusion/physical-expr/src/array_expressions.rs

 use datafusion_common::ScalarValue;
 use datafusion_common::{DataFusionError, Result};
 use datafusion_expr::ColumnarValue;
 use std::sync::Arc;

+macro_rules! downcast_arg {


I think this is the same as downcast_value: https://docs.rs/datafusion-common/26.0.0/datafusion_common/macro.downcast_value.html

alamb · 2023-06-21T18:32:00Z

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

 caused by
-Error during planning: Cannot automatically convert List\(Field \{ name: "item", data_type: List\(Field \{ name: "item", data_type: List\(Field \{ name: "item", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: \{\} \}\), nullable: true, dict_id: 0, dict_is_ordered: false, metadata: \{\} \}\), nullable: true, dict_id: 0, dict_is_ordered: false, metadata: \{\} \}\) to List\(Field \{ name: "item", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: \{\} \}\)
+Internal error: Optimizer rule 'simplify_expressions' failed, due to generate a different schema, original schema: DFSchema \{ fields: \[DFField \{ qualifier: None, field: Field \{ name: "array_fill\(Int64\(1\),make_array\(\)\)", data_type: List\(Field \{ name: "item", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: \{\} \}\), nullable: true, dict_id: 0, dict_is_ordered: false, metadata: \{\} \} \}\], metadata: \{\} \}, new schema: DFSchema \{ fields: \[DFField \{ qualifier: None, field: Field \{ name: "array_fill\(Int64\(1\),make_array\(\)\)", data_type: List\(Field \{ name: "item", data_type: Null, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: \{\} \}\), nullable: false, dict_id: 0, dict_is_ordered: false, metadata: \{\} \} \}\], metadata: \{\} \}\. This was likely caused by a bug in DataFusion's code and we would welcome that you file an bug report in our issue tracker


something seems wrong with this test

Weird, there seems to be a merge problem.

alamb · 2023-06-21T18:33:02Z

datafusion/physical-expr/src/array_expressions.rs

-                    builder.values().append_null();
-                } else {
-                    builder.values().append_value(arg.value(index));
+        for index in 0..$ARGS[0].len() {


I am not sure about this approach of taking either a ListArray or a NullArray

In the other functions, the way NULL is treated is that the input types are always the same (in this case ListArray) and the values would be null (aka array.is_valid(i) would return false for rows that are null.

Complicating matters is if you type a literal null in sql like:

select array_concat([1,2], null)

That comes to DataFusion as a null literal (with DataType::Null). The coercion / casting logic normally will coerce this to the appropriate type.

For example, here is how I think arithmetic works with null:

select 1 + NULL

Arrives like

ScalarValue::Int32(Some(1)) + ScalarValue::Null

And then the coercion logic will add a cast to Int32:

ScalarValue::Int32(Some(1)) + CAST(ScalarValue::Null, DataType::Int32)

And then the constant folder will collapse this into:

ScalarValue::Int32(Some(1)) + ScalarValue::Int32(None)

So by the time the arithmetic kernel sees it, it only has to deal with arguments of Int32

Your arguments seem reasonable.
I have two ideas regarding the introduction of this feature:

Cast all nulls to other data types:

select make_array(1, Null, 2 Null); ---- 1, 0, 2, 0

Ignore nulls:

select make_array(1, Null, 2, Null); ---- 1, 2

What do you think of these approaches @alamb?

I would expect that the array contains nulls values

Like

select make_array(1, Null, 2 Null); ---- 1, NULL, 2, NULL

Where the element of the ListArray are marked as null 🤔

izveigor · 2023-06-23T18:05:19Z

Hello, @alamb!
I am well aware of your arguments about NULL handling in arrays and initially agreed with it.
But I found a serious problem that only initial approach can solve.
As you know arrays in PostgreSQL and other SQL database are not data types for arithmetic operations.
They are rather data types that help you look up information and form new columns.
For example, if we want to create table with unnest function (which I hope will soon be implemented in Arrow Datafusion)

postgres=# SELECT * FROM
  unnest(
    ARRAY[1, 2, 3, 4, 5],
    ARRAY['Ringo', 'George', 'Paul', 'John', NULL],
    ARRAY[29, 27, 27, 29, NULL]
  ) AS data(id,name,age);
 id |  name  | age 
----+--------+-----
  1 | Ringo  |  29
  2 | George |  27
  3 | Paul   |  27
  4 | John   |  29
  5 |        |    
(5 rows)

Or find the information:

postgres=# SELECT array_positions(array[1, 2, 3, 4, 5], 1);
 array_positions 
-----------------
 {1}
(1 row)

As I know we can create column with NULLS values.
So, I think NULLS must exist in their usual understanding (without casting and passes).
I hope I explained well.
What do you think?

alamb · 2023-06-24T12:34:47Z

Thanks @izveigor -- I will try and find time to review / comment on this PR in more detail

alamb · 2023-06-27T20:29:58Z

As I know we can create column with NULLS values.
So, I think NULLS must exist in their usual understanding (without casting and passes).

Yes, I agree with this sentiment -- I think I am confused. I was imagning that the ARRAY would be a ListArray with null elements that represented NULL.

My comments about NULLs above I think refer to how coercion works (it should be done early in the query rather than in the physical expr).

alamb · 2023-06-27T20:30:39Z

For example, if we want to create table with unnest function (which I hope will soon be implemented in Arrow Datafusion)

That would be awesome! I found you have filed #6555 to track it 👍

izveigor · 2023-06-28T18:38:35Z

It would be desirable to quickly decide the fate of this PR @alamb.

alamb · 2023-06-28T18:44:12Z

I will review it again shortly

alamb

@izveigor here are my thoughts on this PR:

The test cases demonstrate that this PR has much better behavior than we currently have so 👍
I think the approach in this PR to casting is not consistent with how expressions are handled in the rest of the DataFusion codebase and that worries me a lot. Specifically, the rest of the codebase will effectively convert ScalarValue::Null to ScalarValue::TheRightType(None) during the TypeCoercion phase of the Analyzer type coercion and then the rest of the code has the correctly typed NULLs. This is why you don't see Null handling in BuiltInScalarFunction::return_type for other functions. I would expect that coerce_arguments_for_signature would be updated to have the appropriate logic to determine how to cast the arguments to make_array and other functions.

Given your past history of follow on PRs to improve the code, I think it would be acceptable to me if we wanted to merge this PR in and work in improving things as a follow on. However, I really think the current approach in this PR is problematic long term.

How would you like to proceed?

alamb · 2023-06-28T19:11:23Z

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

-query error DataFusion error: SQL error: ParserError\("Expected an SQL statement, found: caused"\)
-caused by
-Error during planning: Cannot automatically convert List\(Field \{ name: "item", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: \{\} \}\) to List\(Field \{ name: "item", data_type: Null, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: \{\} \}\)
+# array scalar function with nulls


these examples look much better to me

izveigor · 2023-06-28T19:58:15Z

@alamb I think it's worth creating a separate ticket for the problem. Therefore, if there are no serious claims, I think it is worth merging this PR.

alamb · 2023-06-28T20:01:58Z

@alamb I think it's worth creating a separate ticket for the problem.

Can you please file this ticket?

Therefore, if there are no serious claims, I think it is worth merging this PR.

Let's do it - I'll do so when the CI passes

alamb · 2023-06-28T21:27:50Z

I merged up to resolve conflicts in array.slt

alamb · 2023-07-07T18:33:05Z

I believe this PR introduced a regression: #6887 cc @izveigor

feat: supports NULL in arrays

4e1bfae

izveigor marked this pull request as draft June 13, 2023 18:50

github-actions bot added core Core DataFusion crate physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt) labels Jun 13, 2023

izveigor mentioned this pull request Jun 13, 2023

fix: from_plan shouldn't use original schema #6595

Merged

izveigor added 2 commits June 17, 2023 19:06

Merge branch 'main' into nulls_array

665c9e1

feat: supports NULL in array functions

dc1e949

github-actions bot added logical-expr Logical plan and expressions optimizer Optimizer rules labels Jun 19, 2023

izveigor marked this pull request as ready for review June 19, 2023 18:55

alamb changed the title ~~feat: supports NULL in arrays~~ feat: support NULL in array functions Jun 21, 2023

alamb reviewed Jun 21, 2023

View reviewed changes

izveigor added 2 commits June 23, 2023 20:07

Merge branch 'main' into nulls_array

07b9ca1

fix: array_fill error

a4e30ee

tustvold force-pushed the main branch from 9dd3c2b to 70d633a Compare June 27, 2023 09:44

Merge branch 'main' into nulls_array

b45ea5f

alamb reviewed Jun 28, 2023

View reviewed changes

izveigor added 2 commits June 28, 2023 22:35

fix: merge

ae274f3

fix: cargo fmt

070ab8e

Merge remote-tracking branch 'apache/main' into nulls_array

46a2f0c

alamb merged commit 9edfcdc into apache:main Jun 29, 2023

izveigor mentioned this pull request Jun 30, 2023

Сustomize behavior of NULLS in array functions #6808

Open

jayzhan211 mentioned this pull request Jul 5, 2023

Support fixed_size_list for make_array #6759

Merged

2 tasks

alamb mentioned this pull request Jul 7, 2023

make_array does not properly support nulls #6887

Closed

This was referenced Jul 30, 2023

All array functions should represent NULL as an element #7142

Open

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

feat: support `NULL` in array functions #6662

feat: support `NULL` in array functions #6662

izveigor commented Jun 13, 2023

izveigor commented Jun 19, 2023

alamb commented Jun 20, 2023

alamb left a comment

alamb Jun 21, 2023

izveigor Jun 21, 2023

izveigor Jun 27, 2023

alamb Jun 21, 2023

izveigor Jun 21, 2023

alamb Jun 21, 2023

izveigor Jun 21, 2023

alamb Jun 27, 2023

alamb Jun 21, 2023

alamb Jun 21, 2023

izveigor Jun 21, 2023

alamb Jun 21, 2023

izveigor Jun 21, 2023

alamb Jun 27, 2023

izveigor commented Jun 23, 2023

alamb commented Jun 24, 2023

alamb commented Jun 27, 2023

alamb commented Jun 27, 2023

izveigor commented Jun 28, 2023

alamb commented Jun 28, 2023

alamb left a comment

alamb Jun 28, 2023

izveigor commented Jun 28, 2023

alamb commented Jun 28, 2023 •

edited

Loading

alamb commented Jun 28, 2023

alamb commented Jul 7, 2023

	return Err(DataFusionError::Internal(format!(
	return Err(DataFusionError::Plan(format!(

feat: support NULL in array functions #6662

feat: support NULL in array functions #6662

Conversation

izveigor commented Jun 13, 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?

izveigor commented Jun 19, 2023

alamb commented Jun 20, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izveigor commented Jun 23, 2023

alamb commented Jun 24, 2023

alamb commented Jun 27, 2023

alamb commented Jun 27, 2023

izveigor commented Jun 28, 2023

alamb commented Jun 28, 2023

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izveigor commented Jun 28, 2023

alamb commented Jun 28, 2023 • edited Loading

alamb commented Jun 28, 2023

alamb commented Jul 7, 2023

feat: support `NULL` in array functions #6662

feat: support `NULL` in array functions #6662

alamb commented Jun 28, 2023 •

edited

Loading