sunchao commented on code in PR #6642:
URL: https://github.com/apache/datafusion-comet/pull/6642#discussion_r4179349643
##########
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:
[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.
##########
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:
[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.
--
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]
