Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make scalar and array handling for array_has consistent #13683

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+32 −9
Open

Make scalar and array handling for array_has consistent #13683

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
828ccff
Make scalar and array handling for array_has consistent
Kimahriman Dec 7, 2024
40781a3
Ignore null elements for scalars and arrays
Kimahriman Dec 16, 2024
f5d5244
Upate comment
Kimahriman Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions datafusion/functions-nested/src/array_has.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,11 @@ fn array_has_dispatch_for_array<O: OffsetSizeTrait>(
let needle_row = Scalar::new(needle.slice(i, 1));
let eq_array = compare_with_eq(&arr, &needle_row, is_nested)?;
let is_contained = eq_array.true_count() > 0;
boolean_builder.append_value(is_contained)
if is_contained || eq_array.null_count() == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am surprised this isn't

Suggested change
if is_contained || eq_array.null_count() == 0 {
if is_contained && eq_array.null_count() == 0 {

I thought if the eq_array is null that means there was at least one comparison that is not known 🤔

Copy link
Author

Choose a reason for hiding this comment

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

The semantics are basically:

  • if the element is contained, return true
  • otherwise if there is a null value in the array, return null
  • otherwise return false

So nulls don't matter if there's match, it only matters if there's no match

boolean_builder.append_value(is_contained);
} else {
boolean_builder.append_null();
}
}

Ok(Arc::new(boolean_builder.finish()))
Expand Down Expand Up @@ -249,8 +253,9 @@ fn array_has_dispatch_for_scalar<O: OffsetSizeTrait>(
}
let sliced_array = eq_array.slice(start, length);
// For nested list, check number of nulls
if sliced_array.null_count() != length {
final_contained[i] = Some(sliced_array.true_count() > 0);
let is_contained = sliced_array.true_count() > 0;
if is_contained || sliced_array.null_count() == 0 {
final_contained[i] = Some(is_contained);
}
}

Expand Down
39 changes: 33 additions & 6 deletions datafusion/sqllogictest/test_files/array.slt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,16 @@ AS VALUES
(arrow_cast(make_array([[1], [2]], [[2], [3]]), 'FixedSizeList(2, List(List(Int64)))'), arrow_cast(make_array([1], [2]), 'FixedSizeList(2, List(Int64))'))
;

statement ok
CREATE TABLE array_has_table_null
AS VALUES
(make_array(1, 2), 1),
(make_array(1, NULL), 1),
(make_array(3, 4, 5), 2),
(make_array(3, NULL, 5), 2),
(make_array(NULL, NULL, NULL), 2)
;

statement ok
CREATE TABLE array_distinct_table_1D
AS VALUES
Expand Down Expand Up @@ -5260,6 +5270,13 @@ select array_has([], null),
----
NULL NULL NULL

# If lhs is has any Nulls, we return Null instead of false
query BB
select array_has([1, null, 2], 3),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is a correct behavior.

Checking DuckDB

D select array_has([1, null, 2], 3);
┌───────────────────────────────────────────┐
│ array_has(main.list_value(1, NULL, 2), 3) │
│                  boolean                  │
├───────────────────────────────────────────┤
│ false                                     │
└───────────────────────────────────────────┘

Copy link
Author

Choose a reason for hiding this comment

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

Hmm DuckDB is different then Spark and Postgres then

spark-sql (default)> SELECT array_contains(array(1, NULL, 3), 4);
NULL

postgres=# SELECT 4 =ANY('{1, NULL, 3}');
 ?column? 
----------
 
(1 row)

Was hoping to use this to implement ArrayContains for Comet, and this is the one behavior difference.

Copy link
Author

Choose a reason for hiding this comment

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

Turns out the existing "correct" behavior for scalars doesn't match DuckDB either, which never returns null unless the whole array is null

DuckDB

D select array_has([null, null], 4);
┌───────────────────────────────────────────┐
│ array_has(main.list_value(NULL, NULL), 4) │
│                  boolean                  │
├───────────────────────────────────────────┤
│ false                                     │
└───────────────────────────────────────────┘

DF

DataFusion CLI v43.0.0
> select array_has([null, null], 4);
+-------------------------------------------+
| array_has(make_array(NULL,NULL),Int64(4)) |
+-------------------------------------------+
|                                           |
+-------------------------------------------+
1 row(s) fetched. 
Elapsed 0.001 seconds.

Copy link
Author

Choose a reason for hiding this comment

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

Updated to match DuckDB behavior and updated description

array_has([null, null, null], 3);
----
NULL NULL

#TODO: array_has_all and array_has_any cannot handle NULL
#query BBBB
#select array_has_any([], null),
Expand Down Expand Up @@ -5338,6 +5355,16 @@ from array_has_table_1D;
true true true
false false false

query B
select array_has(column1, column2)
from array_has_table_null;
----
true
true
false
NULL
NULL

query B
select array_has(column1, column2)
from fixed_size_array_has_table_1D;
Expand Down Expand Up @@ -5541,9 +5568,9 @@ select array_has(column1, make_array(5, 6)),
from arrays;
----
false false false true
true false true false
true false true NULL
true false false true
false true false false
false true NULL false
NULL NULL false false
false false NULL false
false false false NULL
Expand All @@ -5556,9 +5583,9 @@ select array_has(arrow_cast(column1, 'LargeList(List(Int64))'), make_array(5, 6)
from arrays;
----
false false false true
true false true false
true false true NULL
true false false true
false true false false
false true NULL false
NULL NULL false false
false false NULL false
false false false NULL
Expand All @@ -5571,9 +5598,9 @@ select array_has(column1, make_array(5, 6)),
from fixed_size_arrays;
----
false false false true
true false true false
true false true NULL
true false false true
false true false false
false true NULL false
NULL NULL false false
false false NULL false
false false false NULL
Expand Down
Loading