[SPARK-55749][SQL][DOC] Clarify array_contains null-handling in the docs and example - #57413
[SPARK-55749][SQL][DOC] Clarify array_contains null-handling in the docs and example#57413comphead wants to merge 3 commits into
array_contains null-handling in the docs and example#57413Conversation
There was a problem hiding this comment.
The @ExpressionDescription usage field still reads "FUNC(array, value) - Returns true if the array contains the value." This one-liner surfaces in DESCRIBE FUNCTION output and the generated SQL function reference. The PR edits the examples block of this exact annotation to add a NULL-returning case, leaving the usage summary internally inconsistent with its own example and with the newly corrected Scaladoc. The sibling arrays_overlap (same file, line 1851) documents its null semantics in the usage field, so there is a direct precedent; bringing this line in line is a natural part of the stated goal.
uros-b
left a comment
There was a problem hiding this comment.
On another note, we nee to handle PySpark too. cc @HyukjinKwon for this
python/pyspark/sql/functions/builtin.py:19896
The Python array_contains docstring still reads "returning null if the array is null, true if the array contains the given value, and false otherwise"; the identical inaccuracy this PR corrects on the Scala side, in shipped user-facing docs. Its Example 4 only shows the value-found-before-null case (returns true) and never the value-absent-with-null-element case (returns NULL). Given the PR's stated goal of "clarifying array_contains null-handling in the docs", TODO: align the Python description text and add a NULL-returning example (e.g. array_contains(["a", None, "c"], "b") -> NULL).
peter-toth
left a comment
There was a problem hiding this comment.
Thanks for the PR, @comphead!
This clarifies the array_contains null semantics across the three doc surfaces (Scala functions.scala, PySpark builtin.py, and the SQL @ExpressionDescription), replacing the old "false otherwise" -- which hid the value-absent-but-array-has-null -> null case -- with an accurate description plus a NULL example. It reads correctly and matches the implementation, and @uros-b's earlier points (PySpark alignment + the SQL usage one-liner) look addressed in 8f551ff. One gap remains: the rewritten sentence is still inaccurate for a null search value, which also returns null.
Non-blocking
- Null search value also returns null.
ArrayContainsisnullIntolerant(extendsBinaryExpression), so a nullvalueyields null regardless of the array -- e.g.array_contains(array(1, 2, 3), CAST(NULL AS INT))is NULL, whereas the new wording's "otherwise returns false" predicts false. Since the PR's goal is null-handling completeness, worth covering this symmetric case across all three surfaces. [inline:sql/api/src/main/scala/org/apache/spark/sql/functions.scala:12936]
|
|
||
| /** | ||
| * Returns null if the array is null, true if the array contains `value`, and false otherwise. | ||
| * Returns true if the array contains `value`. Returns null if the array is null, or if `value` |
There was a problem hiding this comment.
ArrayContains extends BinaryExpression with nullIntolerant = true and only defines nullSafeEval, so the inherited eval returns null whenever either input is null -- including a null search value (nullSafeEval never runs in that case). So array_contains(array(1, 2, 3), CAST(NULL AS INT)) returns NULL, but this sentence's "otherwise returns false" predicts false -- the same "false otherwise" gap this PR is fixing on the array side.
Worth covering the value-null case symmetrically, e.g.:
* Returns true if the array contains `value`. Returns null if the array is null, if `value`
* is null, or if `value` is not found and the array contains a null element; otherwise returns
* false.The same applies to the PySpark summary in python/pyspark/sql/functions/builtin.py, and (if you want all three surfaces to match) the SQL usage string in collectionOperations.scala.
|
Thanks @uros-b and @peter-toth for the review. I hope all the comments have been addressed |
peter-toth
left a comment
There was a problem hiding this comment.
Thanks for addressing the review, @comphead -- re-checked through bca4b09833a; my value-null finding is resolved (the summary now covers a null value, consistently across the Scala, PySpark, and SQL @ExpressionDescription surfaces, and it matches the impl). Nothing new.
… docs and example
### What changes were proposed in this pull request?
Clarify the `array_contains` documentation:
- `sql/api/.../functions.scala`: rewrote the Scaladoc summary to cover all three return cases (true / false / null).
- `sql/catalyst/.../collectionOperations.scala`: added a SQL example showing the null result when the value is absent but the array contains a null element.
### Why are the changes needed?
The old docs said `array_contains` returns "false otherwise", omitting the null-propagation case:
```
scala> spark.sql("select array_contains(array(1, null, 3), 2)").show(false)
+------------------------------------+
|array_contains(array(1, NULL, 3), 2)|
+------------------------------------+
|NULL |
+------------------------------------+
```
### Does this PR introduce _any_ user-facing change?
No. Documentation-only.
### How was this patch tested?
N/A — docs only.
### Was this patch authored or co-authored using generative AI tooling?
No
Closes #57413 from comphead/arr_contains_doc.
Lead-authored-by: Oleks V <10247224+comphead@users.noreply.github.com>
Co-authored-by: ovoievodin <o_voievodin@apple.com>
Signed-off-by: Peter Toth <peter.toth@gmail.com>
(cherry picked from commit 9ccb1dd)
Signed-off-by: Peter Toth <peter.toth@gmail.com>
What changes were proposed in this pull request?
Clarify the
array_containsdocumentation:sql/api/.../functions.scala: rewrote the Scaladoc summary to cover all three return cases (true / false / null).sql/catalyst/.../collectionOperations.scala: added a SQL example showing the null result when the value is absent but the array contains a null element.Why are the changes needed?
The old docs said
array_containsreturns "false otherwise", omitting the null-propagation case:Does this PR introduce any user-facing change?
No. Documentation-only.
How was this patch tested?
N/A — docs only.
Was this patch authored or co-authored using generative AI tooling?
No