cloud-fan commented on code in PR #57272:
URL: https://github.com/apache/spark/pull/57272#discussion_r3596000375
##########
sql/core/src/test/scala/org/apache/spark/sql/ApproxTopKSuite.scala:
##########
@@ -310,6 +310,23 @@ class ApproxTopKSuite extends SharedSparkSession {
}
}
+ // scalastyle:off nonascii
+ test("SPARK-58096: approx_top_k keys on raw sort-key bytes, not a
lossy-decoded String, " +
Review Comment:
The same lossy-`toString` bug also lived in
`ArrayOfCollatedStringsSerDe.wrap` (the deserialize path), and the fix corrects
it there too — but this new test only exercises the direct `approx_top_k`
update path. The existing SPARK-58069 `accumulate`/`combine` tests above use
ASCII-only fixtures (`'HELLO'`/`'world'`), which can't observe the over-merge,
so the serde path currently has no non-ASCII regression guard. Consider
mirroring this case through
`approx_top_k_estimate(approx_top_k_accumulate(...))` (and/or `combine`) to
cover that second key-producing site. Non-blocking.
--
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]
