fix: normalize noncanonical NaN literals in comparisons

[sunchao]

Which issue does this PR close?

No linked issue. This fixes scalar floating-point comparisons and membership tests involving NaNs or signed zero.

Rationale for this change

Spark treats all NaNs as equal, regardless of their sign or payload bits, and orders them above every non-NaN value. Comet must preserve those rules when an application supplies a NaN literal with a different bit representation from the usual Float.NaN or Double.NaN.

Comet already normalizes floating-point comparison operands, but its shortcut for literals skips everything except negative zero. That leaves unusual NaN literals unchanged even when the column on the other side has been normalized. Native comparisons can then distinguish values that Spark considers equal, producing incorrect Boolean results or silently dropping rows from a filter.

For example, suppose readings is a DataFrame read from Parquet whose value column contains an ordinary Double.NaN:

import org.apache.spark.sql.functions._

// Keep the predicate in Comet rather than pushing it into the Parquet reader.
spark.conf.set("spark.sql.parquet.filterPushdown", "false")
val nanWithPayload = java.lang.Double.longBitsToDouble(0x7ff8000000000001L)
readings.filter(col("value") === lit(nanWithPayload))

Spark retains that NaN row. Without this fix, Comet can drop it because the literal and column contain different NaN bits. Signed NaN literals can also produce incorrect ordering against finite values. The affected case specifically involves a literal with a noncanonical sign or payload, such as the application-supplied value above.

What changes are included in this PR?

The fix makes normalization consistent on both sides of a comparison. A NaN literal now goes through the same existing normalization path as a column operand, so differences in the literal's sign or payload no longer change the result of equality or ordering.

This reuses Comet's existing handling of NaNs and signed zero rather than introducing a new comparison algorithm. Ordinary numbers retain their comparison fast path, and negative zero keeps its existing normalization behavior.

The review also identified a pre-existing membership bug: DataFusion's static floating-point IN filter hashes raw bits, so distinct NaN encodings and zero signs can fail to match. IN, InSet, and the fused NOT IN path now normalize both the membership value and its candidates. Literal normalization is folded during serialization so constant lists remain scalar and retain the native static-filter optimization. Non-floating types and the existing collation/legacy-empty-list fallback rules are unchanged.

When serialization rejects a normalized membership operand, the reason is now preserved on the original IN expression, including the fused NOT IN path. Disabling literal or floating-point normalization support therefore retains the specific EXPLAIN diagnostic and no longer trips the test-only strict fallback assertion. Successful serialization and native membership admission are unchanged.

How are these changes tested?

Review fix at 9dd5c044c: a fresh default-release native build and the full Spark 4.1.3 JVM reactor passed, along with Spotless and CI's Scalafix 0.14.6 syntactic check. All 37 selected tests passed: 33 planner tests and four native comparison/membership tests. The actual loaded native library was checked against the rebuilt and bundled library.

The four new serde/planner tests cover 72 FLOAT/DOUBLE combinations across both operand positions, IN/NOT IN, disabled literals/normalizers, and strict mode on/off. Restoring only the previous membership serializer makes each of the four tests fail. A separate 18-case probe confirms unchanged native/fallback admission, including InSet. These local runs use Spark 4.1.3; CI for the new commit is pending.

New FLOAT and DOUBLE regressions construct NaNs with payload bits and either sign programmatically and compare them with stored column values. They exercise equality, inequality, null-safe equality, and ordering in both operand orders, alongside nulls, finite values, infinities, and signed zero. Every comparison also checks retained row identities through a native filter in both operand orders. The tests use the default floating-point mode.

The checks require native Comet projections and filters, with Parquet filter pushdown disabled so the predicates exercise native comparison execution. Comparing Boolean outputs also prevents Spark's NaN-aware answer checker from hiding an incorrect comparison.

CI previously passed for head 9b6f7c05. The Spark 4.1 expression job explicitly records both new regressions passing, with 1,268 tests passing overall.

For the earlier review follow-up, the full Spark 4.1.3 / Scala 2.13 / JDK 17 JVM reactor and style checks passed. All four expanded regression tests passed against the rebuilt JVM classes. The membership cases force both In and InSet with multi-element lists, include NOT IN, both zero signs, null candidates/input, and negative NaNs generated after the Parquet scan. Both new membership tests fail when only the previous predicate implementation is restored, confirming that they detect the bug. NOT IN with a null candidate is checked for its empty result without requiring a filter that Spark legitimately optimizes away.

That earlier local execution reused a previously built OSS native library; it changed Scala serialization/tests and did not include a new native build. The earlier local Spark 4.0 attempt stopped at dependency resolution. The CI results above describe the prior head; CI for the new commit remains to be confirmed.

[andygrove]

Note on this review: this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled.

Thanks for tracking this down. The root cause is clear and the fix lines up with how Spark's own NormalizeFloatingNumbers decides what needs normalizing, so the direction looks right to me. The test that builds the payload NaNs programmatically and compares boolean outputs rather than relying on the NaN-aware answer checker is a nice touch.

A few things I would like to understand better before this goes in.

Constant-folding the literal instead of wrapping it

In CometExecRule.scala the fall-through now wraps the literal in KnownFloatingPointNormalized(NormalizeNaNAndZero(expr)). For a literal we already know the value at plan time, so we could just replace it with the canonical literal (Literal(Float.NaN, FloatType) or Literal(0.0f, FloatType)) and skip emitting a normalization node into the serialized plan entirely. That keeps the plan smaller and avoids asking the native side to evaluate a normalization over a constant. Was there a reason to prefer the wrapper here, or is folding worth doing?

In and InSet are not in the normalize list

normalize rewrites EqualTo, EqualNullSafe, the four ordering comparisons, Divide, and Remainder, but not In or InSet. Both are registered in QueryPlanSerde (CometIn, CometInSet). Spark's In.eval compares through the type's Ordering, which puts all NaNs in one equivalence class, so WHERE value IN (double('NaN')) matches NaN rows in Spark. Does the native path give the same answer today, or does this have the same class of bug the PR is fixing for EqualTo? If it does, I would rather see it either covered here or filed as a tracking issue and linked from this PR, since it is the same user-visible symptom of rows silently disappearing from a filter.

Scope of normalizePlan

normalizePlan only transforms ProjectExec and FilterExec. That is presumably deliberate because Spark's own rule covers join keys and window partition specs. It would help to have a short comment there saying which operators intentionally rely on Spark's rule, so the next person does not have to reconstruct that reasoning. #5469 seems to be filling in the sort and window rank side of this. Is there an umbrella issue tying these together?

Test coverage question

The test drives the comparison operators through CometProjectExec and only the === case through CometFilterExec. Would it be worth pushing the ordering comparisons through the filter path too? The filter path is where a wrong answer silently drops rows rather than producing a visibly wrong boolean column, so that is the case users are most likely to hit.

[sunchao]

Updated in 4a151c4c7.

I addressed the membership issue in this PR rather than deferring it. IN, InSet, and fused NOT IN now normalize scalar FLOAT/DOUBLE values and list candidates. Literal normalization is evaluated during serialization, keeping literal lists scalar so DataFusion can still build its static membership filter. Non-floating expressions retain the existing path. This membership bug predates the original two-line NaN-literal fix; it was not introduced by it.

The regressions preserve multiple candidates and pin the optimized expression to In or InSet by varying Spark's conversion threshold, so a singleton rewrite to equality cannot give a false pass. They cover both widths, NaN payloads/signs, both zero signs, null values/candidates, and negated membership in projections and filters. Unary negation creates negative NaNs after the Parquet scan to exercise normalization of the value as well as the list. Spark folds NOT IN (..., NULL) filters to an empty relation; those cases check the result without incorrectly requiring a native filter.

I also expanded the original comparison matrix: all seven comparison forms, both operand orders, and both widths now check surviving row identities through CometFilterExec with Parquet pushdown disabled, alongside the Boolean projection assertions.

I kept the scope to the confirmed scalar comparison and membership paths. The literal guards are still needed for ordinary comparisons; constant folding was an optional optimization there, while folding the membership literals is necessary to retain the static IN filter. Existing join/window/grouping normalization is governed by its own Spark/planner paths, so this is not evidence that every such operator was affected by the original literal shortcut. Nested floating-point ordering is separate work, not silently covered by this scalar fix.

Validation: full Spark 4.1.3 JVM reactor, Spotless, and Scalastyle passed; all four expanded tests passed against the rebuilt JVM code. Restoring only the old predicate implementation makes both new membership tests fail with the expected Boolean result mismatch. Local runs reused the existing OSS native library, and no full native rebuild is claimed. Fresh CI remains pending.

[sunchao]

Fixed the normalized-membership fallback issue in 9dd5c044c. The serializer now retains its normalized operands and copies failure reasons back to the original membership expression. Disabling Literal or KnownFloatingPointNormalized preserves the specific EXPLAIN reason and works with the test-only strict fallback check enabled. Successful native serialization and admission are unchanged.

The full Spark 4.1.3 JVM reactor passed all 37 selected tests (33 planner and four native expression tests), using a freshly built and verified native library. The four new tests cover 72 combinations; each fails when the previous membership serializer is restored. Spotless and the exact CI syntactic Scalafix check also pass. I updated the PR description with the scope and validation; CI for this new commit is pending.

[sunchao]

Addressed the pruning follow-up in d48800c93.

Floating IN/InSet lists containing only finite nonzero literals (and optional nulls) now keep the column unwrapped, so Parquet can use its statistics. NaNs, signed zeros, infinities, dynamic candidates, and existing normalization keep their previous handling. The change stays in the existing serializer; native code is unchanged.

Validation: 39 tests passed on Spark 4.1 and 37 on Spark 3.4, with two expected version-based cancellations on 3.4. Both new serialization tests fail against the previous serializer. A fresh controlled Parquet probe confirms that the restored expression shape prunes 15 of 16 row groups instead of zero while returning the same two rows.

[sunchao]

Fixed the remaining infinity-list pruning regression in be281b7.

The fast path now accepts infinity literals by checking for non-NaN, nonzero candidates. Lists containing NaN or either zero sign still normalize. The change is limited to the two guard checks, their comment, and extensions to the existing serialization and membership tests.

Fresh full-reactor JVM runs, including formatting checks, passed: 39 tests on Spark 4.1.3 and 37 on Spark 3.4.3, with two expected version-related cancellations. The updated serialization tests fail with the previous guard and pass with this fix.

In a fresh Spark 4.1 native scan, f IN (1, CAST('Infinity' AS DOUBLE)) now prunes 15 of 16 row groups and reads 1,583 data bytes instead of 25,328, with the same result. FLOAT, negative infinity, and InSet also pass; lists of both infinities prune all 16 groups. Queries over actual infinity values also match Spark with native row filtering enabled or disabled.

Native code is unchanged; these runs reused the matching native library and verified the loaded classes/library. CI for this commit is pending.