Int64 as default type for make_array function empty or null case by jayzhan211 · Pull Request #10790 · apache/datafusion

jayzhan211 · 2024-06-04T14:45:40Z

Which issue does this PR close?

Closes #10789 .

Rationale for this change

What changes are included in this PR?

Convert List(Null) and LargeList(Null) to List(I64) and LargeList(I64) respectively
Fix related array function

Are these changes tested?

Are there any user-facing changes?

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

jayzhan211 · 2024-06-04T14:49:26Z

datafusion/sqllogictest/test_files/array.slt

  (arrow_cast(make_array([[1,2]], [[4, 4]]), 'FixedSizeList(2, List(List(Int64)))'), arrow_cast(make_array([1,2,3], [1]), 'FixedSizeList(2, List(Int64))')),
-  (arrow_cast(make_array([[1], [2]], []), 'FixedSizeList(2, List(List(Int64)))'), arrow_cast(make_array([2], [3]), 'FixedSizeList(2, List(Int64))')),
-  (arrow_cast(make_array([[1], [2]], []), 'FixedSizeList(2, List(List(Int64)))'), arrow_cast(make_array([1], [2]), 'FixedSizeList(2, List(Int64))')),
+  (arrow_cast(make_array([[1], [2]], [[]]), 'FixedSizeList(2, List(List(Int64)))'), arrow_cast(make_array([2], [3]), 'FixedSizeList(2, List(Int64))')),


Now they should have same dimension unlike null type

jayzhan211 · 2024-06-04T14:50:44Z

datafusion/sqllogictest/test_files/array.slt

 select array_concat(make_array(make_array(1, 2), make_array(3, 4)), make_array(make_array()));
 ----
-[[1, 2], [3, 4]]
+[[1, 2], [3, 4], []]


Consistent with duckdb

jayzhan211 · 2024-06-04T14:54:32Z

datafusion/sqllogictest/test_files/array.slt

+query ?
+select array_sort([]);
+----
+[]


free fix because of changing default type to i64

jayzhan211 · 2024-06-04T14:54:40Z

datafusion/sqllogictest/test_files/array.slt

+query ?
+select array_concat([]);
+----
+[]


free fix because of changing default type to i64

Fixed #10200 👍

alamb

Looks like a reasonable change to me. Thanks @jayzhan211

BTW I tested in duckdb and it seems like the default type is actually int32 but I think int64 is close enough

D select [];
┌───────────────────┐
│ main.list_value() │
│      int32[]      │
├───────────────────┤
│ []                │
└───────────────────┘

jayzhan211 · 2024-06-06T00:07:31Z

Looks like a reasonable change to me. Thanks @jayzhan211

BTW I tested in duckdb and it seems like the default type is actually int32 but I think int64 is close enough
D select [];
┌───────────────────┐
│ main.list_value() │
│      int32[]      │
├───────────────────┤
│ []                │
└───────────────────┘

Yes, they use i32. The reason I choose i64 is that the default value in datafusion is mostly i64 so we can avoid the cast for most of the case. We can easily convert it to i32 anytime if there is any need

jayzhan211 · 2024-06-06T00:08:00Z

Thanks @alamb

…che#10790) * set default type i64 Signed-off-by: jayzhan211 <jayzhan211@gmail.com> * fmt Signed-off-by: jayzhan211 <jayzhan211@gmail.com> --------- Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

findepi · 2024-09-02T12:58:18Z

it seems counter-intuitive to me to infer Int64 where it was not provided by a user nor schema of tables a query operates on. It might be transparent to the user, in which case it's probably fine, but it's also likely that this will transpire to some error message, when an array made form make_array is used in some later processing.

> SELECT arrow_typeof(NULL), arrow_typeof([NULL][0]);
+--------------------+------------------------------------------+
| arrow_typeof(NULL) | arrow_typeof(make_array(NULL)[Int64(0)]) |
+--------------------+------------------------------------------+
| Null               | Int64                                    |
+--------------------+------------------------------------------+

Also, it's likely to affect coercion rules in the future.
NULL type is coercible to any other type, but Int64 probably won't be.

jayzhan211 · 2024-09-02T13:15:24Z

Also, it's likely to affect coercion rules in the future.
NULL type is coercible to any other type, but Int64 probably won't be.

I think any type is coercible for Null. Int64(Null) has no problem to be AnyType(Null) because it is Null.
In ScalarValue or Arrow, Null is actually None but wrapped in different type.

joroKr21 · 2025-03-10T10:30:30Z

datafusion/functions-array/src/set_ops.rs

+    // Handle empty array at rhs case
+    // array_union(arr, []) -> arr;
+    // array_intersect(arr, []) -> [];
+    if r.value_length(0).is_zero() {
+        if set_op == SetOp::Union {
+            return Ok(Arc::new(l.clone()) as ArrayRef);
+        } else {
+            return Ok(Arc::new(r.clone()) as ArrayRef);
+        }
+    }


This will panic when r is empty and return wrong results when the first value in the r array is an empty list but the rest are not empty.

Let's find such query first, since the case you mentioned might be handled already before entering to this line

diff --git a/datafusion/sqllogictest/test_files/array.slt b/datafusion/sqllogictest/test_files/array.slt index 3b7f12960..17598be8c 100644 --- a/datafusion/sqllogictest/test_files/array.slt +++ b/datafusion/sqllogictest/test_files/array.slt @@ -4371,7 +4371,8 @@ select array_union(arrow_cast([1, 2, 3, 4], 'LargeList(Int64)'), arrow_cast([5, statement ok CREATE TABLE arrays_with_repeating_elements_for_union AS VALUES - ([1], [2]), + ([0, 1, 1], []), + ([1, 1], [2]), ([2, 3], [3]), ([3], [3, 4]) ; @@ -4379,6 +4380,7 @@ AS VALUES query ? select array_union(column1, column2) from arrays_with_repeating_elements_for_union; ---- +[0, 1] [1, 2] [2, 3] [3, 4] @@ -4386,6 +4388,7 @@ select array_union(column1, column2) from arrays_with_repeating_elements_for_uni query ? select array_union(arrow_cast(column1, 'LargeList(Int64)'), arrow_cast(column2, 'LargeList(Int64)')) from arrays_with_repeating_elements_for_union; ---- +[0, 1] [1, 2] [2, 3] [3, 4]

[SQL] select array_union(column1, column2) from arrays_with_repeating_elements_for_union; [Diff] (-expected|+actual) - [0, 1] - [1, 2] + [0, 1, 1] + [1, 1] [2, 3] - [3, 4] + [3] at test_files/array.slt:4380

Trying to fix it here, 🤞 for green CI: #15149

joroKr21 · 2025-03-10T11:11:05Z

datafusion/functions-array/src/make_array.rs

+// Empty array is a special case that is useful for many other array functions
+pub(super) fn empty_array_type() -> DataType {
+    DataType::List(Arc::new(Field::new("item", DataType::Int64, true)))
+}


I don't get the rationale behind that. From type-theoretical point of view, an empty array has type Array<Null> because you can coerce Null to any other type.

You can think of this a type that is coerced from null to i64. You can't run the kernel function with null type, you need to coerce it to other type, and i64 is the default type we define

You can't run the kernel function with null type

That's the real issue. Coercing to i64 is just a band-aid that creates other problems.

continue on #15149 (comment), let's discussion about the issue it has

set default type i64

ae73385

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jun 4, 2024

fmt

14dc948

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

jayzhan211 commented Jun 4, 2024

View reviewed changes

jayzhan211 marked this pull request as ready for review June 4, 2024 23:06

alamb approved these changes Jun 5, 2024

View reviewed changes

jayzhan211 merged commit 053b53e into apache:main Jun 6, 2024

jonahgao mentioned this pull request Jun 6, 2024

select array_concat([]) panicked #10200

Closed

jayzhan211 deleted the array-i64-default branch September 2, 2024 13:15

joroKr21 reviewed Mar 10, 2025

View reviewed changes

joroKr21 mentioned this pull request Mar 11, 2025

Saner handling of nulls inside arrays #15149

Merged

Conversation

jayzhan211 commented Jun 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

jayzhan211 commented Jun 6, 2024

Uh oh!

jayzhan211 commented Jun 6, 2024

Uh oh!

findepi commented Sep 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayzhan211 commented Sep 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayzhan211 Mar 11, 2025 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jayzhan211 commented Jun 4, 2024 •

edited

Loading

findepi commented Sep 2, 2024 •

edited

Loading

jayzhan211 Mar 11, 2025 •

edited

Loading