test: compare NaN, infinity sign and row order in tolerance checks

[andygrove]

Which issue does this PR close?

Closes #6616.

Rationale for this change

query tolerance= in Comet SQL tests and the Scala checkSparkAnswer*WithTol* helpers share one comparison, and it had three gaps. It skipped the comparison when either side was NaN, accepted +Infinity against -Infinity, and paired rows in collect order instead of sorting them like the exact comparison does. So no NaN result under tolerance was ever checked: not the math fixtures' sqrt(-1.0), acos(2) and NaN inputs, not the NaN permutations in corr.sql, and not the special values in avg.sql.

What changes are included in this PR?

• CometTestBase: the tolerance check now goes through the same code as checkCometAnswer, so it gets the same normalization, the same exception wrapping and the same side-by-side failure report. NaN matches only NaN, and an infinity matches only the infinity of the same sign. Finite values match within the tolerance, which keeps +0.0 matching -0.0. Values nested in arrays, structs and maps are still compared exactly. They now go through QueryTest.compare, as on the exact path, instead of ==.
• Unless the plan has a Sort, each Spark row must match a Comet row of its own, in any order, so duplicate counts must agree. Two rows can only pair when their fields other than the top-level floats compare equal, so the rows are grouped by those fields, with a key that follows QueryTest.compare: maps compare as sets of entries and nested floats by their bits. Within a group the rows are sorted by their float values, compared by value rather than by toString, which would put Spark's 1.2246467991473532E-16 after 0.5 but Comet's 0.0 before it. With one float field, pairing the rows in that order works whenever any pairing does. With more, rows that match can sort differently on each side, so a group that does not pair in order falls back to Kuhn's algorithm, which can reassign earlier pairs.
• New CometAnswerToleranceSuite pins the NaN, infinity sign, signed zero, one-to-one pairing and ORDER BY behavior. It is listed in both PR workflows.
• Contributor guide: describes the new semantics, and fixes the example, which used tolerance=0.0001 although the helper rejects tolerances above 1e-6. Out-of-date comments in csc.sql and pow.sql are updated.

How are these changes tested?

• CometAnswerToleranceSuite (new) passes on Spark 3.5 (Scala 2.12) and 4.1. With the old comparison restored, its NaN, infinity and sorting tests fail. Its one-to-one checks fail if the rows are paired by zipping the sorted answers, if the fallback becomes a greedy first match, or if map keys keep their entry order, and its nested NaN check fails if nested doubles are keyed by value.
• The full CometSqlFileTestSuite passes on Spark 4.1 (702 tests). On Spark 3.4, 3.5, 4.0 and 4.2, the math, aggregate and window fixtures pass, and so do the Scala tests that use the helpers, which also pass on 4.1: various math scalar functions, signum, expm1, randn expression with random parameters, multiple nondetermenistic expressions with shuffle, aggregate window function for all types, Decimal Avg with DF, covariance & correlation, var_pop and var_samp, and stddev_pop and stddev_samp.
• To confirm the NaN rows are now compared, I temporarily made NaN never match. 34 fixture runs failed, covering every math function listed in the issue plus avg.sql and corr.sql. With the real comparison they pass, so the stricter check exposes no Comet mismatches and no query needed ignore(...).

[sunchao]

Summary

• Prior state and problem: Tolerance comparisons accepted NaN against finite values, ignored infinity signs, and paired unordered results in collection order.
• Design approach: Reuse checkCometAnswer preparation and diagnostics, with explicit floating-point matching and numeric row sorting.
• Correctness / compatibility analysis: Two introduced P2 issues were reproduced: Spark 3.5 strict compilation fails, and unordered multi-column comparisons can reject valid answers. Checked QueryTest sources for Spark 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0.
• Key design decisions: Sharing normalization and reporting reduces duplication. Sorting adds O(n log n) work to unordered tolerance tests. Production execution is unchanged.
• Implementation sketch: Adds comparison helpers and CometAnswerToleranceSuite, registers it in both PR workflows, and updates contributor documentation and fixture comments.
• Behavioral changes worth calling out: Against branch-1.1, stricter special-value checks and unordered comparison are intended. The multi-column rejection is unintended. Signed-zero tolerance remains lenient, while nested values use exact comparison.
• Suggested improvements: Make Float widening explicit and fix unordered multi-column matching, with regression coverage for near-equal leading fields.

Reviewed all seven changed files at ea70afba3fade730a3002031476f6246bd975cd9 against 207fb6f4b9801adea9f270e816c3fa33da4b1534. The PR is not a draft. Routed skill: review-comet-pr. No sibling subsystem skill applies. The snapshot and refreshed discussion contain no existing reviews or comments.

Exact-head CI: 21 successful, one failed, three running, and 14 skipped checks. The failure is Strict Scala warnings (Spark 3.5, JDK 17). Linux expressions, execution, and TPC-DS jobs remain running. macOS and upstream Spark SQL suites were skipped.

Validation: Isolated Scala probes using the exact comparison helpers reproduced both findings and confirmed the intended special-value and single-column fixes. Explicit .toDouble conversions passed the strict compiler probe. Full Comet/native suites were not run locally because this checkout has no build artifacts. No project code or GitHub state was changed. Recommend request changes for the two findings.

[sunchao]

[P2] [P2] Make the Float-to-Double conversions explicit. Both pattern variables are Float, but valuesMatch accepts Double. The Spark 3.5 strict profile enables -Ywarn-numeric-widen and -Xfatal-warnings, so this new call produces two compilation errors and fails the PR build. Please pass s.toDouble and c.toDouble explicitly.

Evidence: Exact-head CI job https://github.com/apache/datafusion-comet/actions/runs/37233330700/job/111528238545 ran ./mvnw -B test-compile -Pspark-3.5 -Pstrict-warnings -DskipTests and reported two CometTestBase.scala:625: implicit numeric widening errors. An isolated Scala 2.12.18 compiler probe reproduced the failure. The same probe with explicit .toDouble conversions compiled successfully.

[andygrove]

Fixed in 054ae39. Both values are now widened with .toDouble. The test sources compile with -Pspark-3.5 -Pstrict-warnings locally, and putting the old call back reproduces the two implicit numeric widening errors.

[sunchao]

[P2] [P2] Preserve valid pairings for unordered multi-column answers. With tolerance 1e-6, Spark rows [(0.0, "a"), (0.0, "b")] and Comet rows [(1e-7, "a"), (0.0, "b")] should match. The new exact numeric ordering reverses only the Comet rows, so this zip compares "a" with "b" and fails. The base helper accepts these results in the supplied collection order. This introduces false failures in the shared SQL and Scala tolerance helpers whenever small leading-field differences change row order. Please use one-to-one tolerance-aware matching for unordered multi-column results, preserving duplicate counts, and cover this case in the suite.

Evidence: A Scala 2.13.17 probe using the exact head preparation, ordering, and matching helpers with Spark 4.1.3 Row and QueryTest.compare printed multi-column near tie: base=true, head=false. Sorted Spark rows were List([0.0,a], [0.0,b]), while Comet rows became List([0.0,b], [1.0E-7,a]). Replacing the string column with 1.0 and 2.0 reproduced the same regression. Preserving row order made both cases pass.

[rich7420]

Single-column maps also fail, confirmed with an extracted-helper probe on Scala 2.12/2.13:

val spark = Seq(
  Row(ListMap("a" -> 1, "b" -> 2)),
  Row(ListMap("b" -> 1, "a" -> 2)))
val comet = Seq(
  Row(ListMap("b" -> 2, "a" -> 1)),
  Row(ListMap("a" -> 2, "b" -> 1)))

The original pairs match, but toString sorting swaps only Comet’s rows. Could we add this regression and match unordered rows one-to-one, preserving duplicates and allowing reassignment? A greedy first-match fallback can also miss valid pairings.

[andygrove]

Fixed in 054ae39. Unordered rows now pair one-to-one instead of zipping the two sorted answers. Rows are first grouped by their fields other than the top-level floats, using a key that follows QueryTest.compare (maps compare as sets of entries, nested floats by their bits), and each group has to pair up on its own, so duplicate counts must agree. Within a group the rows are sorted by their float values. With a single float field that pairing succeeds whenever any pairing exists. With more than one, a group that does not pair in sorted order falls back to Kuhn's algorithm, which can reassign earlier pairs, so a greedy first match can no longer strand a row.

CometAnswerToleranceSuite now covers the (0.0, "a") example and its all-double variant, the map rows from @rich7420, a case that greedy first-match gets wrong, and mismatched duplicate counts. Each of those checks fails if I put back the sorted zip, a greedy fallback, an entry-order key for maps or a by-value key for nested doubles.

[rich7420]

Rechecked 054ae3978d on Scala 2.12/2.13 with an extracted-helper probe. This addresses my comment.

[andygrove]

@rich7420 this is in the float semantics area you have been working in. If you have time, would you be able to take a look at the tolerance check changes? Thanks.

[sunchao]

Summary

• Prior state and problem: Tolerance checks accepted NaN against finite values, ignored infinity signs, and paired unordered results in collection order. This weakened both SQL fixtures and Scala tests.
• Design approach: Route tolerance checks through checkCometAnswer, sharing normalization, exception reporting, and diagnostics. Add explicit floating-point matching and numeric row ordering.
• Correctness: The special-value fixes and single-column ordering work in focused probes. The existing multi-column blocker remains reproducible: at tolerance 1e-6, Spark rows [(0.0, "a"), (0.0, "b")] and Comet rows [(1e-7, "a"), (0.0, "b")] pass the base helper but fail the head because sorting pairs different strings.
• Compatibility analysis: Checked Spark QueryTest sources for 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0, including sort detection, recursive comparison, NaNs, and signed zero. Existing NaN normalization accommodates version differences. The reported Spark 3.5 strict-warning compilation failure remains unresolved.
• Key design decisions: Tolerance applies only to top-level Float and Double fields. NaN matches NaN, infinity signs must agree, and signed zeros remain equivalent. Queries containing a logical Sort retain their collected order.
• Implementation sketch: Updates the shared comparison helpers, adds seven tests in CometAnswerToleranceSuite, registers that suite in both PR workflows, and revises contributor documentation and fixture comments.
• Performance: Unordered tolerance comparisons now add normalization and O(n log n) sorting to the previous linear comparison. This affects test execution only. No separate P1/P2 performance issue was identified.
• Design: Sharing preparation and diagnostics is sound. Sorting independently and then zipping does not provide valid tolerance matching for general multi-column results, as the existing review demonstrates.
• Abstraction & complexity: The private ordering and matching helpers keep the refactor localized and remove duplicated reporting logic. The remaining problem is the row-pairing algorithm, rather than unnecessary abstraction.
• Behavioral changes worth calling out: Compared with branch-1.1 at e9efd9f764ee0a59b7898ff028d6985d4a7a28e1, whose relevant comparison code matches the supplied base, stricter special-value checks and unordered matching are intended. Nested values now use QueryTest.compare, including signed-zero distinctions. Rejection of valid multi-column answers is unintended. Production query behavior is unchanged.
• Suggested improvements: Address the existing comments by explicitly widening the two floats with .toDouble and implementing one-to-one matching for unordered tolerance results while preserving duplicate counts. Add the demonstrated multi-column case to the suite. No additional P1/P2 improvement was identified.

Reviewed all seven changed files in the full diff from 207fb6f4b9801adea9f270e816c3fa33da4b1534 to ea70afba3fade730a3002031476f6246bd975cd9. The PR remains open and non-draft. Read AGENTS.md, used review-comet-pr, and checked sibling routing. No subsystem sibling skill applies to these test-infrastructure, documentation, and CI changes. Read and refreshed all existing reviews, conversation comments, inline comments, and threads.

No additional introduced P1/P2 issues found within this review. Both existing P2 threads remain unresolved: implicit Float widening and unordered multi-column pairing. They are not duplicated as new findings.

Exact-head CI: 24 successful checks, two failed, and 15 skipped. The failures are Strict Scala warnings (Spark 3.5, JDK 17) and the aggregate Required Checks. The strict job reports two implicit numeric-widening errors at CometTestBase.scala:625. Linux Spark 4.1 test jobs and TPC-H/TPC-DS verification passed. The expressions job passed 2,075 tests, including all seven new tolerance tests. macOS, upstream Spark SQL, and Iceberg suites were skipped.

Validation: Bounded Scala 2.12.18 probes used the exact head/base comparison helpers, Spark 3.5.9 Row, and the copied upstream QueryTest.compare implementation. They reproduced the existing pairing regression and verified special values, nulls, nested comparisons, duplicate counts, tolerance boundaries, and ordered comparisons. A strict compiler probe failed on the current float call and passed with explicit conversions. Full Comet/native suites were not run locally because this checkout has no build artifacts. The author's broader version-matrix runs were not independently repeated. No project code or GitHub state was changed.

[rich7420]

LGTM