spark-test commented on code in PR #59615:
URL: https://github.com/apache/spark/pull/59615#discussion_r4361420259
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/aggregate/VectorAggSuite.scala:
##########
@@ -405,6 +405,151 @@ class VectorAggSuite extends SparkFunSuite {
assert(result(1) === 3.0f)
}
+ test("VectorAvg - opposite extreme values do not overflow") {
+ val (agg, buffer, _) = createVectorAvg()
+ agg.update(buffer, createInputRow(Array(Float.MaxValue, -Float.MaxValue)))
+ agg.update(buffer, createInputRow(Array(-Float.MaxValue, Float.MaxValue)))
+ assertResult(Array(0.0f, 0.0f))(evalAsFloatArray(agg, buffer))
+
+ val (_, buffer1, _) = createVectorAvg()
+ val (_, buffer2, _) = createVectorAvg()
+ agg.update(buffer1, createInputRow(Array(Float.MaxValue, -Float.MaxValue)))
+ agg.update(buffer2, createInputRow(Array(-Float.MaxValue, Float.MaxValue)))
+ agg.merge(buffer1, buffer2)
+ assertResult(Array(0.0f, 0.0f))(evalAsFloatArray(agg, buffer1))
+ }
+
+ test("VectorAvg - finite overflow preserves cumulative weights") {
+ val (agg, buffer, _) = createVectorAvg()
+ val first = Array(Float.MaxValue, -Float.MaxValue)
+ agg.update(buffer, createInputRow(first))
+ agg.update(buffer, createInputRow(first))
+ // The third update overflows the delta. An unweighted midpoint would
incorrectly return zero.
+ agg.update(buffer, createInputRow(first.map(-_)))
+ val expected = (Float.MaxValue.toDouble / 3).toFloat
+ assertFloatArrayEquals(
+ evalAsFloatArray(agg, buffer),
+ Array(expected, -expected),
+ tolerance = math.ulp(expected) * 2)
+ }
+
+ test("VectorAvg - non-finite results do not depend on input order") {
+ val first =
+ Array(Float.PositiveInfinity, 1.0f, Float.NegativeInfinity,
Float.PositiveInfinity)
+ val second = Array(1.0f, Float.PositiveInfinity, 1.0f,
Float.NegativeInfinity)
+ def checkResult(result: Array[Float]): Unit = {
+ assertResult(Float.PositiveInfinity)(result(0))
+ assertResult(Float.PositiveInfinity)(result(1))
+ assertResult(Float.NegativeInfinity)(result(2))
+ assert(result(3).isNaN)
+ }
+
+ Seq(Seq(first, second), Seq(second, first)).foreach { vectors =>
+ val (agg, buffer, _) = createVectorAvg()
+ vectors.foreach(v => agg.update(buffer, createInputRow(v)))
+ checkResult(evalAsFloatArray(agg, buffer))
+ agg.update(buffer, createInputRow(Array(2.0f, 2.0f, 2.0f, 2.0f)))
+ checkResult(evalAsFloatArray(agg, buffer))
+
+ val (_, buffer1, _) = createVectorAvg()
+ val (_, buffer2, _) = createVectorAvg()
+ agg.update(buffer1, createInputRow(vectors.head))
+ agg.update(buffer2, createInputRow(vectors(1)))
+ agg.merge(buffer1, buffer2)
+ checkResult(evalAsFloatArray(agg, buffer1))
+ }
+ }
+
+ test("VectorAvg - finite elements around non-finite deltas") {
+ val first = Array(1.0f, Float.MaxValue, 3.0f, Float.PositiveInfinity, 5.0f,
+ Float.NegativeInfinity, 7.0f, Float.PositiveInfinity, 9.0f, Float.NaN,
11.0f)
+ val second = Array(3.0f, -Float.MaxValue, 5.0f, 1.0f, 7.0f, 1.0f, 9.0f,
+ Float.NegativeInfinity, 11.0f, 2.0f, 13.0f)
+ val expected = Array(2.0f, 0.0f, 4.0f, Float.PositiveInfinity, 6.0f,
+ Float.NegativeInfinity, 8.0f, Float.NaN, 10.0f, Float.NaN, 12.0f)
+
+ Seq(false, true).foreach { useGenericArray =>
+ val (agg, buffer, _) = createVectorAvg()
+ Seq(first, second).foreach { values =>
+ val array = if (useGenericArray) {
+ new GenericArrayData(values)
+ } else {
+ ArrayData.toArrayData(values)
+ }
+ agg.update(buffer, InternalRow(array))
+ }
+ assertResult(expected.map(JFloat.floatToIntBits))(
+ evalAsFloatArray(agg, buffer).map(JFloat.floatToIntBits))
+ }
+ }
+
+ test("VectorAvg - finite updates preserve running-average rounding") {
+ Seq(false, true).foreach { includeInfinity =>
+ val (agg, buffer, _) = createVectorAvg()
+ val first = Array.tabulate(17)(i => if (i % 2 == 0) 1.0f else -1.0f)
+ val last = first.map(-_)
Review Comment:
**Nit (P3):** This test is meant to guard the PR's guarantee that finite,
non-overflowing updates keep the original `oldAvg + (value - oldAvg) *
invCount` rounding. With these inputs it can't fail if finite updates switch to
the scale-first expression `oldAvg + (value * invCount - oldAvg * invCount)`
that this PR adds for overflow.
At the third update every finite element has oldAvg = +/-1 and value = -/+1.
`value - oldAvg` is exactly +/-2 and `2 * invCount` equals `invCount +
invCount`, so both formulas give 0x3eaaaaaa/0xbeaaaaaa. The other new exact-bit
assertions run at newCount = 2, where `invCount = 0.5f` makes scaling exact.
The pre-existing exact multi-row cases (1, 2, 3) also come out to 2.0f either
way. As a result, the plausible simplification of always scaling first, which
would drop the special case, would pass the whole suite while finite
`vector_avg` results drift by about an ulp per update. For example, inputs 1,
1, 6 would give 0x402aaaaa instead of 0x402aaaab.
Making the third input discriminating fixes this cheaply, for example:
```scala
val last = first.map(_ * 6)
val expected = Array.tabulate(17)(i => if (i % 2 == 0) 0x402aaaab else
0xc02aaaab)
```
Keep the infinity variant as is (`last(8) = 1.0f`, `expected(8) =
0x7f800000`). In a float32 simulation of the kernel, the current implementation
passes both variants. With scale-first finite updates the test fails on
block-path elements (0-15, or 0-7 in the infinity variant) and on scalar-path
elements (16, or 9-16 in the infinity variant).
**Verification:**
- **Behavior:** Run the vector_avg aggregate unit tests and confirm that the
strengthened finite-rounding test passes against the current implementation for
both the all-finite and the infinity-at-element-8 variants.
- **Regression:** Temporarily replace the finite update expression with the
scale-first expression in the block and scalar paths and confirm the
strengthened finite-rounding test fails on the finite elements, then restore
the implementation.
--
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]
