fix: normalize noncanonical NaN literals in comparisons - #5472
Open
sunchao wants to merge 1 commit into
Open
Conversation
sunchao
marked this pull request as ready for review
August 26, 2026 18:04
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
No linked issue. This fixes floating-point comparisons with noncanonical NaN literals.
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.NaNorDouble.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
readingsis a DataFrame read from Parquet whosevaluecolumn contains an ordinaryDouble.NaN: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. The production change is limited to two literal guards. Ordinary numbers retain their fast path, and negative zero keeps its existing normalization behavior.
How are these changes tested?
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. 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 now passes for head
9b6f7c05. The Spark 4.1 expression job explicitly records both new regressions passing, with 1,268 tests passing overall.The earlier local Spark 4.0 attempt stopped during Maven dependency resolution before tests ran. A local comparison demonstrating failures on the unchanged base and passes with this patch has not been completed; the passing test evidence above comes from CI.