HyukjinKwon commented on code in PR #59162:
URL: https://github.com/apache/spark/pull/59162#discussion_r4173742590
##########
sql/core/src/test/scala/org/apache/spark/sql/DataFrameStatSuite.scala:
##########
@@ -461,6 +466,39 @@ class DataFrameStatSuite extends SharedSparkSession {
assert(resultRow.get(1).asInstanceOf[scala.collection.Seq[String]].toSet
== Set("a", "b", null))
}
+ test("SPARK-59898: freqItems compares BinaryType values by content") {
+ // Every row holds the same value, each in its own byte array.
+ Seq(10, 11, 12).foreach { n =>
+ val df = Seq.fill(n)(Array[Byte](1, 2)).toDF("b")
+ val items = df.stat.freqItems(Array("b"),
0.5).collect().head.getSeq[Array[Byte]](0)
+ assert(items.map(_.toSeq) === Seq(Seq[Byte](1, 2)))
+ }
+ // A value in half of the rows of a column spread across partitions, whose
partial results
+ // are serialized and merged.
+ val distinct = $"id".cast("string").cast("binary")
+ val skewed = spark.range(0, 100, 1, 4)
+ .select(when($"id" % 2 === 0, lit(Array[Byte](1,
2))).otherwise(distinct).as("b"))
+ val items = skewed.stat.freqItems(Array("b"),
0.4).collect().head.getSeq[Array[Byte]](0)
Review Comment:
**Nit (P3):** The new tests only assert that equal binary values collapse to
one key. Neither inserts two *different* byte-array contents into a single
aggregation and asserts they stay separate frequent items: the skewed case here
checks only `.contains(Seq[Byte](1, 2))`, which still holds even if
`BinaryKey.equals` over-merged (the surviving key keeps the first inserted
value's bytes), and the aggregate-level test inserts just one distinct content
plus null. So an over-merging regression in `BinaryKey.equals`/`hashCode` would
silently conflate distinct binary values into one entry — wrong, under-counted
`freqItems` results — without failing any test. Consider adding a case with at
least two distinct byte-array values and asserting both are returned as
separate items, so the content-equality contract is locked in both directions.
**Verification:**
- **Regression:** Demonstrate that conflating two distinct binary contents
into one key changes an assertion, confirming the
distinct-content->distinct-key direction is now covered in addition to the
already-covered equal-content->same-key direction.
--
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]
