andygrove opened a new issue, #6616:
URL: https://github.com/apache/datafusion-comet/issues/6616
Part of #6615.
### Describe the bug
`query tolerance=X` in Comet SQL tests compares results with
`CometTestBase.checkAnswerWithTolerance`. So do the Scala helpers
`checkSparkAnswerWithTolerance` and `checkSparkAnswerAndOperatorWithTolerance`.
That comparison has three gaps:
1. **NaN.** It skips the comparison when either side is NaN (`else if
(!actual.isNaN && !expected.isNaN)`). Comet returning NaN where Spark returns a
number passes, and so does the reverse.
2. **Infinity sign.** It accepts `+Infinity` against `-Infinity`
(`assert(actual.isInfinity == expected.isInfinity)`). The contributor guide
documents this, but nothing needs it.
3. **Row order.** It zips the two collected results in collect order. The
exact path, `checkCometAnswer`, sorts both sides when the plan has no `Sort`;
this one does not. A query without `ORDER BY` therefore pairs rows by whatever
order each engine returned them in.
Because of the first gap, NaN results under tolerance are never compared:
- **Math fixtures:** every per-function fixture runs its NaN-producing
inputs only under `tolerance=`. That covers `sqrt(-1.0)`, `acos(2)`,
`sin(Infinity)` and NaN input in `acos`, `asin`, `atan`, `atan2`, `cos`,
`cosh`, `cot`, `exp`, `expm1`, `log`, `log10`, `log2`, `sin`, `sinh`, `sqrt`,
`tan` and `tanh`.
- **Aggregate fixtures:**
- `aggregate/corr.sql` `test_corr_nan` ("Test permutations of NULL and
NaN") exists only to check NaN results, so it checks nothing for those rows.
- `avg.sql` `test_avg_special` and the Infinity rows in `sum.sql` have the
same problem.
- **Scala tests:** the math tests that use these helpers have the same blind
spot.
### Expected behavior
- NaN matches only NaN.
- Infinities compare with their sign.
- Both sides are sorted the way the exact path sorts them when the plan has
no `Sort`.
- Signed zero can stay lenient under tolerance, but the contributor guide
should say so.
Fixing this may expose real mismatches in the fixtures above. Treat those as
Comet bugs: file each one and mark the query `ignore(<link>)`. Don't loosen the
check.
This blocks the math and aggregate parts of the epic: their conversions move
NaN and infinity results into plain `query` blocks, and the remaining tolerance
queries should be trustworthy.
### Additional context
Found while auditing the Scala expression suites against the SQL fixtures
for #6615, on `main` at 3bc2faa934.
--
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]
