andygrove commented on code in PR #6642:
URL: https://github.com/apache/datafusion-comet/pull/6642#discussion_r4261000571
##########
spark/src/test/scala/org/apache/spark/sql/CometTestBase.scala:
##########
@@ -564,56 +596,41 @@ abstract class CometTestBase
}
/**
- * A helper function for comparing Comet DataFrame with Spark result using
absolute tolerance.
+ * Orders rows for a tolerance comparison: field by field, with `float` and
`double` fields
+ * compared by value and everything else by its string form. Sorting whole
rows by `toString`,
+ * as the exact comparison does, can put two values that match within
tolerance at different
+ * positions: `1.2246467991473532E-16` sorts after `0.5`, but `0.0` sorts
before it. The two
+ * zeros are equal here because the comparison matches them, and NaN sorts
last.
*/
- private def checkAnswerWithTolerance(
- dataFrame: DataFrame,
- expectedAnswer: Seq[Row],
- absTol: Double): Unit = {
- val actualAnswer = dataFrame.collect()
- require(
- actualAnswer.length == expectedAnswer.length,
- s"actual num rows ${actualAnswer.length} != expected num of rows
${expectedAnswer.length}")
-
- actualAnswer.zip(expectedAnswer).foreach { case (actualRow, expectedRow) =>
- checkAnswerWithTolerance(actualRow, expectedRow, absTol)
+ private val toleranceOrdering: Ordering[Row] = new Ordering[Row] {
+ override def compare(x: Row, y: Row): Int =
+ x.toSeq.iterator
+ .zip(y.toSeq.iterator)
+ .map { case (l, r) => compareField(l, r) }
+ .find(_ != 0)
+ .getOrElse(0)
+
+ private def compareField(left: Any, right: Any): Int = (left, right) match
{
+ case (null, null) => 0
+ case (null, _) => -1
+ case (_, null) => 1
+ case (l: Float, r: Float) => if (l == r) 0 else
java.lang.Float.compare(l, r)
+ case (l: Double, r: Double) => if (l == r) 0 else
java.lang.Double.compare(l, r)
+ case _ => left.toString.compareTo(right.toString)
}
}
- /**
- * Compares two answers and makes sure the answer is within absTol of the
expected result.
- */
- private def checkAnswerWithTolerance(
- actualAnswer: Row,
- expectedAnswer: Row,
- absTol: Double): Unit = {
- require(
- actualAnswer.length == expectedAnswer.length,
- s"actual answer length ${actualAnswer.length} != " +
- s"expected answer length ${expectedAnswer.length}")
- require(absTol > 0 && absTol <= 1e-6, s"absTol $absTol is out of range (0,
1e-6]")
-
- actualAnswer.toSeq.zip(expectedAnswer.toSeq).foreach {
- case (actual: Float, expected: Float) =>
- if (actual.isInfinity || expected.isInfinity) {
- assert(actual.isInfinity == expected.isInfinity, s"actual answer
$actual != $expected")
- } else if (!actual.isNaN && !expected.isNaN) {
- assert(
- math.abs(actual - expected) < absTol,
- s"actual answer $actual not within $absTol of correct answer
$expected")
- }
- case (actual: Double, expected: Double) =>
- if (actual.isInfinity || expected.isInfinity) {
- assert(actual.isInfinity == expected.isInfinity, s"actual answer
$actual != $expected")
- } else if (!actual.isNaN && !expected.isNaN) {
- assert(
- math.abs(actual - expected) < absTol,
- s"actual answer $actual not within $absTol of correct answer
$expected")
- }
- case (actual, expected) =>
- assert(actual == expected, s"$actualAnswer did not equal
$expectedAnswer")
+ private def rowsMatch(spark: Row, comet: Row, absTol: Double): Boolean =
+ spark.length == comet.length && spark.toSeq.zip(comet.toSeq).forall {
+ case (s: Float, c: Float) => valuesMatch(s, c, absTol)
Review Comment:
Fixed in 054ae3978d. 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.
##########
spark/src/test/scala/org/apache/spark/sql/CometTestBase.scala:
##########
@@ -497,14 +517,23 @@ abstract class CometTestBase
|${org.apache.spark.sql.catalyst.util.stackTraceToString(e)}
""".stripMargin)
}
- val preparedSpark = prepareCometAnswer(sparkAnswer, isSorted)
- val preparedComet = prepareCometAnswer(cometAnswer, isSorted)
- if (!QueryTest.compare(preparedSpark, preparedComet)) {
+ val ordering =
+ if (absTol.isDefined) toleranceOrdering else Ordering.by[Row,
String](_.toString)
+ val preparedSpark = prepareCometAnswer(sparkAnswer, isSorted, ordering)
+ val preparedComet = prepareCometAnswer(cometAnswer, isSorted, ordering)
+ val matches = absTol match {
+ case Some(tol) =>
+ preparedSpark.length == preparedComet.length &&
+ preparedSpark.zip(preparedComet).forall { case (s, c) => rowsMatch(s,
c, tol) }
Review Comment:
Fixed in 054ae3978d. 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.
--
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]
