Skip to content

Fix: array contains null handling#3372

Merged
andygrove merged 4 commits intoapache:mainfrom
Shekharrajak:fix/array-contains-null-handling
Feb 4, 2026
Merged

Fix: array contains null handling#3372
andygrove merged 4 commits intoapache:mainfrom
Shekharrajak:fix/array-contains-null-handling

Conversation

@Shekharrajak
Copy link
Contributor

Which issue does this PR close?

Closes #3345

Rationale for this change

When array_contains() is called with a literal cast(NULL as array) argument, Comet returns a different result than Spark. Spark follows SQL
three-valued logic where array_contains(NULL, value) should return NULL, but Comet was delegating directly to DataFusion's array_has without proper NULL
array handling.

Other array functions like CometArrayRemove and CometArrayAppend already handle NULL arrays correctly using a CASE WHEN wrapper, but CometArrayContains
was missing this handling.

What changes are included in this PR?

  • Added NULL array handling to CometArrayContains in arrays.scala by wrapping the array_has call in a CASE WHEN expression:
    • WHEN array IS NOT NULL THEN array_has(array, key)
    • ELSE NULL
  • This follows the same pattern used by CometArrayRemove and CometArrayAppend

How are these changes tested?

Added new test "array_contains - NULL array returns NULL" in CometArrayExpressionSuite

@mbutrovich
Copy link
Contributor

Thanks @Shekharrajak! Can we add/migrate the tests to the new framework?

https://datafusion.apache.org/comet/contributor-guide/sql-file-tests.html

@codecov-commenter
Copy link

codecov-commenter commented Feb 3, 2026

Codecov Report

❌ Patch coverage is 80.95238% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.01%. Comparing base (f09f8af) to head (7cbb557).
⚠️ Report is 920 commits behind head on main.

Files with missing lines Patch % Lines
...src/main/scala/org/apache/comet/serde/arrays.scala 80.95% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #3372      +/-   ##
============================================
+ Coverage     56.12%   60.01%   +3.89%     
- Complexity      976     1475     +499     
============================================
  Files           119      175      +56     
  Lines         11743    16185    +4442     
  Branches       2251     2684     +433     
============================================
+ Hits           6591     9714    +3123     
- Misses         4012     5116    +1104     
- Partials       1140     1355     +215     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Shekharrajak
Copy link
Contributor Author

Thanks @Shekharrajak! Can we add/migrate the tests to the new framework?

https://datafusion.apache.org/comet/contributor-guide/sql-file-tests.html

Updated b7929c0

Please trigger the checks .

}
}

test("array_contains - NULL array returns NULL") {
Copy link
Member

Choose a reason for hiding this comment

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

Unless this test is testing functionality that isn't already tested in the SQL-based tests, I think we can just remove this.

Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @Shekharrajak

@Shekharrajak
Copy link
Contributor Author

Shekharrajak commented Feb 3, 2026

@Shekharrajak
Copy link
Contributor Author

All checks looks fine now.

@andygrove andygrove merged commit a4bf90a into apache:main Feb 4, 2026
123 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

array_contains returns wrong result for literal array with NULL cast

4 participants