Skip to content

fix: normalize signed zero in nested float array comparisons - #5235

Open
divyankshah wants to merge 1 commit into
apache:mainfrom
divyankshah:fix/gh-5191-signed-zero-nested-float-comparison
Open

fix: normalize signed zero in nested float array comparisons#5235
divyankshah wants to merge 1 commit into
apache:mainfrom
divyankshah:fix/gh-5191-signed-zero-nested-float-comparison

Conversation

@divyankshah

Copy link
Copy Markdown

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.

Arrow's nested comparator uses total order, where -0.0 and 0.0 are
distinct, but Spark's ordering.equiv treats them as equal, while
still treating NaN as equal to itself. Normalize negative zero in
nested float leaves before building the comparator so
arrays_overlap and array_position match Spark's semantics.

Ref apache#5191

@andygrove andygrove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

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.

Nested array comparison does not match Spark for signed zero

2 participants