peter-toth commented on code in PR #59567:
URL: https://github.com/apache/spark/pull/59567#discussion_r4312682098
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -195,7 +195,10 @@ case class Mode(
// The new map is then used in the rest of the Mode evaluation logic.
//
// It is expected to work for all simple and complex types with collated
fields.
+ //
+ // Unwrap binary keys (see `toBufferKey`) before they are ordered or
returned.
val collationAwareBuffer = getCollationAwareBuffer(child.dataType, buffer)
Review Comment:
**Finding 1.** The collation fold here still compares binary values by
reference when they sit inside a struct, array or map next to a collated
string. `getCollationAwareBuffer` groups such a key by
`collationAwareTransform`, which returns a binary field as the raw
`Array[Byte]` (the `isBinaryStable` case at `Mode.scala:88`). That array lands
in the `Seq` grouping key, and `Seq` compares it by reference. I ran this at
the head:
```sql
SELECT hex(m.b), m.s FROM (
SELECT mode(named_struct('b', b, 's', c COLLATE UTF8_LCASE)) AS m
FROM VALUES (X'01', 'a'), (X'01', 'a'), (X'01', 'A'), (X'01', 'A'),
(X'02', 'z'), (X'02', 'z'), (X'02', 'z') AS t(b, c))
```
- head: `02, z` (count 3), though `a` and `A` fold into a group of 4
- with an `INT` instead of the binary field: `1, A`, as expected
- `mode() WITHIN GROUP` and `mode(s, true)` give the same answers
So the description's "A struct or array key that contains binary fields was
already compared by content" holds for the buffer, but not for this fold. It is
not new in this PR, since the base gives the same `02, z`. Keying the binary
value by content in `collationAwareTransform` fixes it:
```scala
dataType match {
case _ if data.isInstanceOf[Array[Byte]] => new
BinaryKey(data.asInstanceOf[Array[Byte]])
case _ if UnsafeRowUtils.isBinaryStable(dataType) => data
```
With that, all three forms return `01`, and `CollationAggregationSuite` and
`DataFrameAggregateSuite` pass. A test could check only `hex(m.b) = '01'`,
since the folded group keeps whichever of `a` and `A` it saw first.
##########
sql/core/src/test/scala/org/apache/spark/sql/DataFrameAggregateSuite.scala:
##########
@@ -1321,6 +1321,40 @@ class DataFrameAggregateSuite extends SharedSparkSession
Row(Row(Double.NaN)))
}
+ test("SPARK-XXXXX: mode compares binary values by content") {
Review Comment:
**Finding 2.** The ticket ID is still a placeholder.
```suggestion
test("SPARK-60309: mode compares binary values by content") {
```
--
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]
