andygrove opened a new pull request, #6642: URL: https://github.com/apache/datafusion-comet/pull/6642
## 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 sort when the plan has no `Sort`, 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 `==`. - Rows are sorted with `float` and `double` fields compared by value, not by `toString`. A `toString` sort can separate two values that match within tolerance. Spark's `1.2246467991473532E-16` sorts after `0.5`, while Comet's `0.0` sorts before it, so the wrong rows would be paired. For single-column results, sorting by value pairs the rows correctly whenever a match exists. - New `CometAnswerToleranceSuite` pins the NaN, infinity sign, signed zero, sorting 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). With the old comparison restored, its NaN, infinity and sorting tests fail. - The full `CometSqlFileTestSuite` passes on Spark 4.1 (608 tests). On Spark 3.4, 3.5 (Scala 2.13), 4.0 and 4.2, the math, aggregate and window fixtures pass, and so do the Scala tests that use the helpers: `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(...)`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
