fix: normalize signed zero in nested float array comparisons

[divyankshah]

Which issue does this PR close?

Closes #5191.

Rationale for this change

Arrow's nested comparator (make_comparator, used by arrays_overlap's nested path and array_position's nested fallback) orders floats by total order, where -0.0 and 0.0 are distinct. Spark's ordering.equiv (used for structural equality of nested elements) checks numeric equality first, so -0.0 == 0.0 there, while NaN == NaN still holds in both.

What changes are included in this PR?

• Add nested_float_normalize.rs: recursively rebuilds nested (List/LargeList/FixedSizeList/Struct) arrays with -0.0 normalized to 0.0 in Float32/Float64 leaves, leaving NaN untouched.
• arrays_overlap.rs: normalize both sides before building the nested comparator; update test_nested_float_total_order to assert -0.0 and 0.0 now overlap; add test_struct_float_field_signed_zero_overlap covering a struct field.
• array_position.rs: normalize both sides before building the fallback comparator; update test_nested_float_and_null_position (result changes from [2, 2, 1] to [2, 1, 1] since row 1's -0.0 vs 0.0 now matches at position 1); add test_struct_float_field_signed_zero_position covering a struct field.

Note: #5194 is a separate issue (#5101, comparator-hoisting for perf) but touches the same comparator-construction code path. Happy to rebase on top of whichever lands first.

How are these changes tested?

• cargo test -p datafusion-comet-spark-expr — all 600+ tests pass, including the new/updated ones above.
• cargo clippy -p datafusion-comet-spark-expr --lib -- -D warnings — clean.
• cargo fmt -p datafusion-comet-spark-expr -- --check — clean.
• cargo check --workspace — clean.

[andygrove]

First pass, focused on the two items that need to change. I have not gone through test coverage or the docs yet, so expect a second round.

Nice find on the root cause, and the diagnosis matches what I see in Spark's SQLOrderingUtil.

1. The normalization is inside the per-row loop, and it is quadratic.

In arrays_overlap.rs:431 the comparator is built per row, so normalize_negative_zero runs once per row per side. probe is left.value(i), and arrow-rs's GenericListArray::slice only narrows the offsets and null buffer, it leaves values pointing at the entire child buffer. So list.values() in the DataType::List branch hands back every float in the column, and each row copies all of them.

I checked out the branch and added a nested benchmark to compare against apache/main:

benchmark	main	this PR
array<array<double>> 1024 rows x 4 inner x 8 floats	192.8 us	223.8 ms
array<array<double>> 4096 rows x 4 inner x 8 floats	760.9 us	3.574 s
array<array<int>> 4096 rows x 4 inner x 8 ints	753.0 us	980.3 us

4x the rows gives 16x the time, which confirms the shape. The int32 row also regresses 30% despite having no float leaves at all, from the unconditional ListArray::new rebuild.

Could you hoist the normalization above the row loop, normalizing left and right once and slicing per row from the normalized arrays? A cheap recursive DataType check to skip types with no float leaf would take care of the int32 case. array_position's position_fallback only normalizes once per call so it is fine on the loop question, but it would still benefit from the type gate. Since this touches the same lines as the comparator hoisting in #5194, rebasing on that first as you suggested is probably the easier path.

2. normalize_float already exists, and using it also fixes NaN.

There is a normalize_float at native/spark-expr/src/math_funcs/internal/normalize_nan.rs:110, and hll_plus_plus.rs:126 applies it to Float32/Float64 leaves with unary(), which is close to what the leaf arms here do. Reusing it drops the duplicate logic and lets you use unary(), which works on the values buffer and preserves the null buffer.

It also closes a second mismatch in the same code path. Spark's SQLOrderingUtil.compareDoubles is if (x == y) 0 else java.lang.Double.compare(x, y), and Double.compare goes through doubleToLongBits, which collapses every NaN payload including the sign bit. Arrow's comparator uses total_cmp, which sorts -NaN below -Infinity. On this branch:

[[-NaN]] overlaps [[NaN]] => false   (Spark returns true)

normalize_float canonicalizes NaN as well as signed zero, so it fixes this for free. Worth noting because the new comment on test_nested_float_total_order says NaN matches itself and matches Spark, which currently only holds for canonical positive NaN.

One process note: gh pr checks reports no checks on this branch yet, so nothing has been validated by CI. I will get the workflow approved.

[divyankshah]

First pass, focused on the two items that need to change. I have not gone through test coverage or the docs yet, so expect a second round.

Nice find on the root cause, and the diagnosis matches what I see in Spark's SQLOrderingUtil.

1. The normalization is inside the per-row loop, and it is quadratic.

In arrays_overlap.rs:431 the comparator is built per row, so normalize_negative_zero runs once per row per side. probe is left.value(i), and arrow-rs's GenericListArray::slice only narrows the offsets and null buffer, it leaves values pointing at the entire child buffer. So list.values() in the DataType::List branch hands back every float in the column, and each row copies all of them.

I checked out the branch and added a nested benchmark to compare against apache/main:

benchmark main this PR
array<array<double>> 1024 rows x 4 inner x 8 floats 192.8 us 223.8 ms
array<array<double>> 4096 rows x 4 inner x 8 floats 760.9 us 3.574 s
array<array<int>> 4096 rows x 4 inner x 8 ints 753.0 us 980.3 us
4x the rows gives 16x the time, which confirms the shape. The int32 row also regresses 30% despite having no float leaves at all, from the unconditional ListArray::new rebuild.

Could you hoist the normalization above the row loop, normalizing left and right once and slicing per row from the normalized arrays? A cheap recursive DataType check to skip types with no float leaf would take care of the int32 case. array_position's position_fallback only normalizes once per call so it is fine on the loop question, but it would still benefit from the type gate. Since this touches the same lines as the comparator hoisting in #5194, rebasing on that first as you suggested is probably the easier path.

2. normalize_float already exists, and using it also fixes NaN.

There is a normalize_float at native/spark-expr/src/math_funcs/internal/normalize_nan.rs:110, and hll_plus_plus.rs:126 applies it to Float32/Float64 leaves with unary(), which is close to what the leaf arms here do. Reusing it drops the duplicate logic and lets you use unary(), which works on the values buffer and preserves the null buffer.

It also closes a second mismatch in the same code path. Spark's SQLOrderingUtil.compareDoubles is if (x == y) 0 else java.lang.Double.compare(x, y), and Double.compare goes through doubleToLongBits, which collapses every NaN payload including the sign bit. Arrow's comparator uses total_cmp, which sorts -NaN below -Infinity. On this branch:

[[-NaN]] overlaps [[NaN]] => false   (Spark returns true)

normalize_float canonicalizes NaN as well as signed zero, so it fixes this for free. Worth noting because the new comment on test_nested_float_total_order says NaN matches itself and matches Spark, which currently only holds for canonical positive NaN.

One process note: gh pr checks reports no checks on this branch yet, so nothing has been validated by CI. I will get the workflow approved.

Hi @andygrove,

Thanks for the thorough review and feedback, this was really helpful.

Both points addressed in the latest commit:

1. Hoisted normalization above the row loop in arrays_overlap.rs, so it runs once per column instead of rebuilding the whole float buffer on every row. Also gated it behind a has_float_leaf check so non-float types (like your int32 case) skip it entirely. Re-ran your benchmark shape locally: 1024 rows went from 223.8ms to ~650µs, 4096 rows from 3.574s to 1.9ms.
2. Switched to the existing normalize_float (from normalize_nan.rs, already used in hll_plus_plus.rs) instead of the hand-rolled version, so NaN payloads get canonicalized too. Added a test for [[-NaN]] vs [[NaN]].

The array_position's fallback also got the type gate, though it didn't have the per-row issue since it already normalized once per call.

Please let me know if anything needs to be adjusted.

[divyankshah]

Reopening, this got closed by accident.

[peterxcli]

found there is a similar helper in hll agg:

datafusion-comet/native/spark-expr/src/agg_funcs/hll_plus_plus.rs

Lines 123 to 140 in 397b5bb

	/// Normalize a float/double column the way Spark's `NormalizeNaNAndZero` does before hashing:
	/// every NaN becomes the canonical NaN and `-0.0` becomes `0.0`. Returns the input unchanged for
	/// non-floating-point types.
	fn normalize_floats(array: &ArrayRef) -> ArrayRef {
	match array.data_type() {
	DataType::Float32 => {
	let normalized: Float32Array =
	array.as_primitive::<Float32Type>().unary(normalize_float);
	Arc::new(normalized)
	}
	DataType::Float64 => {
	let normalized: Float64Array =
	array.as_primitive::<Float64Type>().unary(normalize_float);
	Arc::new(normalized)
	}
	_ => Arc::clone(array),
	}
	}

considering extending it to cover more data type so we dont need another file, or at least we can have one centralized helper for similar usage.

[divyankshah]

Hi @peterxcli,

thanks for pointing that out. the normalization rule itself isn't duplicated, we already call the same normalize_float function you linked. there's a bit of repeated dispatch code around it (the Float32/Float64 match arms), but the actual behavior comes from one place. hll's version skips nested types (struct/list) right now, and I'm not sure if making it handle those too would change hll's behavior in ways that need separate testing. But I can follow up on it if it's worth doing. Please let me know if I should implement it in that way.

[andygrove]

Second round. Both items from the first pass look right to me. The hoist is in the right place, the has_float_leaf gate handles the int32 case cleanly, and reusing normalize_float picked up the NaN canonicalization as hoped. Nice work on the turnaround.

I checked the branch out, built it, and ran candidate SQL fixtures against both this PR and its merge-base. The fix works end to end. Below is what I would like added, plus one trap you will hit if you write these yourself.

The trap: a bare -0.0 literal is not a negative zero

Spark parses -0.0 as decimal(1,1), decimal has no signed zero, and the coercion to double yields +0.0. CAST(-0.0 AS DOUBLE) has the same problem. My first draft of these fixtures passed against the unpatched merge-base, which is how I caught it. You have to write double('-0.0').

That also means the existing row at arrays_overlap.sql:120 is testing nothing:

INSERT INTO test_overlap_dbl VALUES ..., (array(0.0), array(-0.0)), ...

Both sides store [0.0]. Could you fix that one to (array(double('0.0')), array(double('-0.0'))) while you are in the file? I filed #5271 to audit the rest of the corpus, since roughly a dozen other float fixtures have the same shape.

Suggested fixtures

These fail on the merge-base and pass on this branch, so they are load bearing.

Append to arrays_overlap.sql, after the nested arrays block at :175:

-- nested double arrays: Spark's nested path uses ordering.equiv, where -0.0 == 0.0
statement
CREATE TABLE test_overlap_nested_dbl(a array<array<double>>, b array<array<double>>) USING parquet

statement
INSERT INTO test_overlap_nested_dbl VALUES
  (array(array(double('-0.0'))), array(array(double('0.0')))),
  (array(array(double('0.0'))), array(array(double('-0.0')))),
  (array(array(1.0, double('-0.0'))), array(array(1.0, 0.0))),
  (array(array(double('NaN'))), array(array(double('NaN')))),
  (array(array(1.0)), array(array(2.0))),
  (array(array(double('-0.0')), cast(NULL as array<double>)), array(array(double('0.0')))),
  (array(cast(NULL as array<double>)), array(array(double('0.0'))))

query
SELECT a, b, arrays_overlap(a, b) FROM test_overlap_nested_dbl

-- struct element with a double field
statement
CREATE TABLE test_overlap_struct_dbl(a array<struct<x:double>>, b array<struct<x:double>>) USING parquet

statement
INSERT INTO test_overlap_struct_dbl VALUES
  (array(named_struct('x', double('-0.0'))), array(named_struct('x', double('0.0')))),
  (array(named_struct('x', double('0.0'))), array(named_struct('x', double('-0.0')))),
  (array(named_struct('x', double('NaN'))), array(named_struct('x', double('NaN')))),
  (array(named_struct('x', 1.0)), array(named_struct('x', 2.0))),
  (array(cast(NULL as struct<x:double>)), array(named_struct('x', double('0.0'))))

query
SELECT a, b, arrays_overlap(a, b) FROM test_overlap_struct_dbl

Append to array_position.sql, after the nested string block at :243:

-- nested double array column: -0.0 and 0.0 are equal under Spark's nested ordering
statement
CREATE TABLE test_ap_nested_dbl(arr array<array<double>>, val array<double>) USING parquet

statement
INSERT INTO test_ap_nested_dbl VALUES
  (array(array(1.0), array(double('-0.0'))), array(double('0.0'))),
  (array(array(double('-0.0')), array(1.0)), array(double('0.0'))),
  (array(array(double('0.0')), array(1.0)), array(double('-0.0'))),
  (array(array(double('NaN'))), array(double('NaN'))),
  (array(array(1.0)), array(2.0)),
  (NULL, array(double('0.0'))),
  (array(array(double('0.0'))), NULL)

query
SELECT array_position(arr, val) FROM test_ap_nested_dbl

The row with the trailing NULL inner list is worth keeping. On the merge-base that case returns null rather than false, so it covers a path the other rows do not.

Please add the comment about why the flat path is left alone

I raised this last round and the testing made it more important, not less. Spark's flat and nested paths genuinely disagree on signed zero, and after this PR Comet correctly disagrees with itself in the same way:

	signed zero	NaN sign
flat (fastEval, HashSet of boxed Double)	distinct	equal
nested (bruteForceEval, ordering.equiv)	equal	equal

I confirmed the flat half empirically. With double('-0.0') against double('0.0') on a flat array<double>, both Spark and Comet return false. So normalize_list_element_floats must never be applied to the flat path, and a comment on arrays_overlap_list_generic saying so would stop someone unifying them later.

Two things I filed separately, not for this PR

• #5270, the flat arrays_overlap NaN divergence I mentioned last round. Confirmed on this branch: arrays_overlap(array(-x), array(double('NaN'))) with x = NaN gives true in Spark and false in Comet. The issue spells out why the fix cannot just reuse normalize_float.
• #5269, a native panic in arrays_overlap on expression-constructed nested lists. It reproduces identically on the merge-base, so it has nothing to do with your change.

One note on the new struct test

array_position over array<struct<...>> always falls back to Spark, because ArraysBase.isTypeSupported rejects StructType for #1307. I hit that when trying to write a SQL fixture for it. So test_struct_float_field_signed_zero_position covers a path that cannot currently be reached from a query. Worth a one-line comment on the test saying so, otherwise it reads as if SQL-level coverage exists.

Still no CI on this branch. I will get the workflow approved.