Skip to content

test: query tolerance= in Comet SQL tests passes when either side is NaN #6616

Open

Description

@andygrove

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 3bc2faa.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bugSomething isn't workingpriority:lowMinor issues, test failures, tooling, cosmetictestTesting related

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions