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

Merged
merged 3 commits into from
Jan 22, 2025
Merged

Make scalar and array handling for array_has consistent #13683

Show file tree
Hide file tree
Changes from all commits
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
8 changes: 2 additions & 6 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 @@ -214,8 +214,7 @@ fn array_has_dispatch_for_array<O: OffsetSizeTrait>(
let is_nested = arr.data_type().is_nested();
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)
boolean_builder.append_value(eq_array.true_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.

this is not a behavior change I don't think - just a refactor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this was just matching the style of the other

}

Ok(Arc::new(boolean_builder.finish()))
Expand Down Expand Up @@ -248,10 +247,7 @@ fn array_has_dispatch_for_scalar<O: OffsetSizeTrait>(
continue;
}
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);
}
final_contained[i] = Some(sliced_array.true_count() > 0);
}

Ok(Arc::new(BooleanArray::from(final_contained)))
Expand Down
33 changes: 30 additions & 3 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

# Always return false if not contained even if list has null elements
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
Contributor 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
Contributor 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
Contributor 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);
----
false false

#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
false
false

query B
select array_has(column1, column2)
from fixed_size_array_has_table_1D;
Expand Down Expand Up @@ -5574,9 +5601,9 @@ false false false true
true false true false
true false false true
false true false false
NULL NULL false false
false false NULL false
false false false NULL
false false false false
false false false false
false false false false

query BBBBBBBBBBBBB
select array_has_all(make_array(1,2,3), make_array(1,3)),
Expand Down
Loading