fix(spark): array_contains float equality matches Spark by mrhard9090 · Pull Request #26011 · apache/datafusion

mrhard9090 · 2026-10-03T18:46:46Z

Which issue does this PR close?

Closes spark: array_contains compares floats by bits, where Spark treats -0.0 as 0.0 and all NaNs as equal datafusion-spark#34.

Rationale for this change

SparkArrayContains delegates the comparison to array_has, which compares floats by their bits. Spark's array_contains treats -0.0 as equal to 0.0 and all NaNs as equal, so array_contains(array(-0.0D), 0.0D) returned false here (Spark: true), and two NaNs with different bit patterns did not match.

What changes are included in this PR?

array_contains.rs rewrites the float values before calling array_has: -0.0 becomes 0.0 and every NaN becomes the canonical NaN. It applies to a float needle (scalar or array) and to the elements of the list haystack, including List, LargeList and FixedSizeList, lists nested in lists, and float fields of structs (scalar or array needle). The existing null handling is unchanged.
Cases in spark/array/array_contains.slt: -0.0 vs 0.0 in both directions for double, -0.0 for float, NaN vs -NaN, NaN next to another value, a nested list, struct fields with NaN payloads, and plain misses that stay false. Unit tests in array_contains.rs cover scalar and array needles, f32, the three list types and structs.

Not covered: I did not look at arrays_overlap, which the issue says probably has the same equality.

What is the testing strategy for this PR?

cargo test -p datafusion-sqllogictest --test sqllogictests -- array_contains.slt passes. With array_contains.rs from main, the new float queries fail. cargo fmt and cargo clippy -p datafusion-spark --all-targets --all-features --no-deps -- -D warnings pass.

Are there any user-facing changes?

Spark array_contains on float or double elements now matches Spark for -0.0 and NaN. No API change.

AI assistance: written with Claude Code; I reviewed the diff and ran the tests above.

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>

codecov-commenter · 2026-10-03T19:11:10Z

Codecov Report

❌ Patch coverage is 94.66667% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.65%. Comparing base (95a1961) to head (4026025).
⚠️ Report is 337 commits behind head on main.

Files with missing lines	Patch %	Lines
...afusion/spark/src/function/array/array_contains.rs	94.66%	9 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   apache/datafusion#26011      +/-   ##
==========================================
+ Coverage   82.64%   82.65%   +0.01%     
==========================================
  Files        1147     1147              
  Lines      445502   446108     +606     
  Branches   445502   446108     +606     
==========================================
+ Hits       368179   368726     +547     
- Misses      54969    54990      +21     
- Partials    22354    22392      +38

☔ View full report in Codecov by Harness.
📢 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.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

txwyy123 · 2026-10-04T04:31:15Z

Thanks for the fix and for calling out the remaining struct scope. I cross-validated that follow-up at fa9a8460bd1dd59a4957a6fcf1400960c6755a96 and have a small local extension, rather than a competing PR.

Two repros for the struct portion of apache/datafusion-spark#34:

SELECT array_contains(
  array(named_struct('v', -CAST('NaN' AS DOUBLE))),
  named_struct('v', CAST('NaN' AS DOUBLE)));
-- Current PR: false; Spark 4.2.0: true

SELECT array_contains(
  array(named_struct('v', -CAST('NaN' AS DOUBLE)), NULL),
  named_struct('v', CAST('NaN' AS DOUBLE)));
-- Current PR: NULL; Spark 4.2.0: true

I observed these and the other new SQL cases in a live Spark 4.2.0 session (UTC), with ANSI both on and off. Signed zeros within structs already match through the shared nested comparison path; the failing struct cases here are NaN representations.

The scoped extension adds these two match arms (and imports StructArray), leaving array_has and the existing null-handling code untouched:

// In canonicalize_floats's ScalarValue match:
ScalarValue::Struct(struct_array) => {
    ColumnarValue::Scalar(ScalarValue::Struct(
        canonicalize_float_array(&(Arc::clone(struct_array) as ArrayRef))
            .as_struct()
            .clone()
            .into(),
    ))
}

// In canonicalize_float_array's DataType match:
DataType::Struct(fields) if !fields.is_empty() => {
    let struct_array = array.as_struct();
    Arc::new(StructArray::new(
        fields.clone(),
        struct_array.columns().iter()
            .map(canonicalize_float_array)
            .collect(),
        struct_array.nulls().cloned(),
    ))
}

Both paths matter: a struct needle can be scalar or columnar. Recursing through fields also covers struct/list combinations. The empty-fields guard avoids the length-inferring constructor for empty structs, preserving their row count.

Validation of the local extension:

Tests added first: the Rust NaN-payload regression failed on this PR's head, and 8 added SQL queries mismatched.
After the extension: the Spark crate's 300 unit tests passed; its extended-feature test binary also passed all 302 tests. All 244 Spark SLT files passed.
Coverage includes FLOAT/DOUBLE NaN signs and payloads, signed zeros, all four scalar/array argument shapes, both struct/list nesting directions, null behavior, sliced structs, field metadata, unchanged non-float child buffers, and empty structs.
Workspace cargo clippy --locked --profile ci --all-targets --all-features -- -D warnings, formatting, and diff checks passed.
Limitation: the full extended workspace test run was interrupted during compilation when local disk space dropped to about 2 GiB; I am not claiming a complete workspace test pass.

Would you prefer incorporating this small extension and the regression tests here, or handling structs in a follow-up after this PR? I can share the full test diff if helpful.

Surfaced by the audit-datafusion-spark-expression skill.

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>

mrhard9090 · 2026-10-04T04:53:21Z

Thanks, @txwyy123. I took the struct extension into this PR (both the scalar and the array path, with the empty-struct guard) and added unit tests for the scalar/array needle, f32, the three list types and structs, plus three struct queries in array_contains.slt. I have not checked the struct results against a live Spark; the expected values in the tests come from your report and from Spark's NaN equality semantics. The Spark crate's array_contains tests, the slt file and clippy with -D warnings pass here.

kosiew · 2026-10-22T07:54:14Z

#25971 also canonicalizes signed zeros and NaNs

kosiew

@mrhard9090,

Thanks for the fix. Canonicalizing signed zeros and NaNs before Spark's array_contains delegates to array_has is an appropriate approach, and the added regression tests cover the intended equality behavior, including nested structs.

Keeping this normalization within the Spark adapter is the right architectural boundary. Generic DataFusion comparison semantics remain unchanged, while the recursive adapter handles Spark compatibility. That adapter must also preserve array types, row counts, validity, and nested layout during reconstruction.

I'm requesting changes because reconstructing zero-width fixed-size lists can silently drop rows or panic when nested, including for non-float inputs. This is a correctness regression that needs a length-preserving fix and UDF regression coverage. The API simplification and potential reconstruction optimizations are optional follow-ups and should not block this fix.

kosiew · 2026-10-22T07:55:16Z

+        }
+        DataType::FixedSizeList(field, size) => {
+            let list = array.as_fixed_size_list();
+            Arc::new(FixedSizeListArray::new(


A zero-width FixedSizeListArray can contain one valid row, but FixedSizeListArray::new infers length zero, silently dropping that row or panicking when nested inside a List with offsets [0, 1]. This also affects non-float children. Please return the original array when size == 0 or use a length-preserving constructor, and add UDF tests verifying one valid false result for a non-null scalar needle against an array-valued haystack containing one zero-width list, plus preserved row counts without panic for a zero-width list nested inside a List.

kosiew · 2026-10-22T07:55:16Z

+/// `-0.0` becomes `0.0` and every NaN becomes the canonical NaN. Applies to a
+/// float argument and to the elements of a list argument; other values are
+/// returned as they are.
+fn canonicalize_floats(value: &ColumnarValue) -> Result<ColumnarValue> {


Optional simplification: could canonicalize_floats return ColumnarValue directly? Neither it nor its callees returns a recoverable error, so removing Result, the outer Ok, and the caller's ? would clarify the failure boundary. This does not address the reconstruction panic, which Result cannot catch.

kosiew · 2026-10-22T07:55:16Z

+            array.as_primitive::<Float64Type>(),
+            canonical_f64,
+        )),
+        DataType::List(field) => {


Optional performance follow-up: consider reusing list/struct arrays when their children are unchanged. Currently even List is reconstructed, while a small slice of a large float list normalizes the entire retained child buffer. Please benchmark these cases before introducing a type gate or visible-range optimization; no typical-workload slowdown is established yet.

alamb · 2026-10-30T19:11:24Z

Likely AI spam -- no response from prolific author. Closing

## Which issue does this PR close? - Addresses the float equality difference in #34. The untyped `NULL` argument difference mentioned in the comments there is not changed by this PR, so I left the issue open. ## Rationale for this change `array_contains` delegates the element comparison to `array_has`, which compares floating point values by their bits. Spark compares them with `-0.0 == 0.0` and `NaN == NaN`: `CodegenContext.genEqual` in Spark 4.2.0 generates `(java.lang.Double.isNaN(c1) && java.lang.Double.isNaN(c2)) || c1 == c2` (and the same for `Float`). So `array_contains(array(-0.0D), 0.0D)` returned `false` here and `true` in Spark, and two NaNs with different sign bits or payloads did not match, at any depth of list and struct elements. ## What changes are included in this PR? - `array_contains.rs` rewrites the float values of both arguments before calling `array_has`: `-0.0` becomes `0.0` and every NaN becomes the canonical NaN. It applies to scalar and array arguments, and to `Float32`/`Float64` at any depth of `List`, `LargeList`, `FixedSizeList` and `Struct`. Arrays that hold no floats are returned as they are. The existing null handling is unchanged. - `FixedSizeList` is rebuilt with `try_new_with_length`, so a zero-width list keeps its rows. - `array/array_contains.slt`: new queries for both zeros in both directions (`double` and `float`), NaN with a flipped sign bit, `LargeList` and `FixedSizeList` haystacks, nested lists and structs, null semantics, and a table with column arguments. - Unit tests in `array_contains.rs` for the normalization (scalar and nested shapes, zero-width lists, reuse of children without floats). An earlier attempt at this issue, apache/datafusion#26011, was closed before the move. Its review asked for a length-preserving rebuild of zero-width `FixedSizeList` and tests for it, which this PR includes. ## What is the testing strategy for this PR? - `cargo fmt --all -- --check`, `cargo clippy --all-targets --all-features -- -D warnings`, `cargo test -p datafusion-spark` (331 passed) and `cargo test -p datafusion-spark-sqllogictest` (245 of 245 files) pass. - With `array_contains.rs` from `main`, the new float queries fail. Run one by one, 9 of 14 probe queries fail on `main` (top-level `-0.0`, NaN with a flipped sign bit at every depth, `LargeList`/`FixedSizeList`); nested `-0.0` already compares equal on `main` and is kept as a guard. - Each branch of the new code is covered by at least one test that fails when the branch is removed (checked by hand: no normalization, NaN branch, zero branch for `f32` and `f64`, struct, list, scalar path, zero-width length, early return). ## Unknowns and assumptions - I could not run Spark here. The expected values come from Spark's source (`genEqual`, 4.2.0) and from the report in #34, not from a live PySpark session. - `arrays_overlap` (mentioned in #34) is not touched. ## Are there any user-facing changes? `array_contains` on float or double elements now matches Spark for `-0.0` and NaN. No API change.

fix(spark): array_contains float equality matches Spark

fa9a846

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>

github-actions Bot added sqllogictest SQL Logic Tests (.slt) spark Changes to datafusion-spark crate labels Oct 3, 2026

fix(spark): canonicalize float fields inside structs in array_contains

4026025

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>

mkleen mentioned this pull request Oct 6, 2026

feat: Treat all NaNs as the same value (-NaN = NaN) #26091

Open

kosiew requested changes Oct 22, 2026

View reviewed changes

alamb added the Prolific Author author opened PRs or issues in more than 20 repositories in the last 30 days label Oct 22, 2026

alamb mentioned this pull request Oct 22, 2026

[DISCUSSION] Rethinking Code Review and Testing in the Agent Era #23839

Open

alamb closed this Oct 30, 2026

Yupsecous mentioned this pull request Nov 13, 2026

fix(spark): array_contains compares floats the way Spark does apache/datafusion-spark#83

Merged

Navigation Menu

Repository navigation

fix(spark): array_contains float equality matches Spark - #26011

fix(spark): array_contains float equality matches Spark#26011
mrhard9090 wants to merge 2 commits into
apache:mainfrom
mrhard9090:fix/spark-array-contains-float-eq

mrhard9090 commented Oct 3, 2026 •

edited

Loading

Uh oh!

codecov-commenter commented Oct 3, 2026 •

edited

Loading

Uh oh!

txwyy123 commented Oct 4, 2026 •

edited

Loading

Uh oh!

mrhard9090 commented Oct 4, 2026

Uh oh!

kosiew commented Oct 22, 2026 •

edited

Loading

Uh oh!

kosiew left a comment

Uh oh!

kosiew Oct 22, 2026

Uh oh!

kosiew Oct 22, 2026

Uh oh!

kosiew Oct 22, 2026

Uh oh!

alamb commented Oct 30, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

mrhard9090 commented Oct 3, 2026 • 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?

What is the testing strategy for this PR?

Are there any user-facing changes?

Uh oh!

codecov-commenter commented Oct 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

txwyy123 commented Oct 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrhard9090 commented Oct 4, 2026

Uh oh!

kosiew commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kosiew left a comment

Choose a reason for hiding this comment

Uh oh!

kosiew Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

kosiew Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

kosiew Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

alamb commented Oct 30, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mrhard9090 commented Oct 3, 2026 •

edited

Loading

codecov-commenter commented Oct 3, 2026 •

edited

Loading

txwyy123 commented Oct 4, 2026 •

edited

Loading

kosiew commented Oct 22, 2026 •

edited

Loading