cloud-fan commented on code in PR #57177:
URL: https://github.com/apache/spark/pull/57177#discussion_r3581377330
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/ApproxTopKAggregates.scala:
##########
@@ -358,8 +379,14 @@ class ApproxTopKAggregateBuffer[T](val sketch:
ItemsSketch[T], private var nullC
case _: TimestampNTZType =>
sketch.asInstanceOf[ItemsSketch[Long]].update(v.asInstanceOf[Long])
case st: StringType =>
- val cKey =
CollationFactory.getCollationKey(v.asInstanceOf[UTF8String], st.collationId)
- sketch.asInstanceOf[ItemsSketch[String]].update(cKey.toString)
+ val orig = v.asInstanceOf[UTF8String]
+ if (UnsafeRowUtils.isBinaryStable(st)) {
+ sketch.asInstanceOf[ItemsSketch[String]].update(orig.toString)
+ } else {
+ val cKey = CollationFactory.getCollationKey(orig,
st.collationId).toString
Review Comment:
The dedup key here is a lossy UTF-8 decode of the ICU sort-key bytes:
`getCollationKey(...)` returns a `UTF8String` of the raw sort key, and
`.toString` runs `new String(bytes, UTF_8)` (`UTF8String.java:2018`), which
replaces every invalid-UTF-8 byte sequence with U+FFFD. ICU sort keys are
arbitrary bytes, not valid UTF-8, so two collation-distinct values whose keys
differ only within invalid-byte regions can decode to the same `String`,
compare equal in `CollatedString.equals/hashCode`, and be over-merged into one
item — inflating the survivor's count and dropping a distinct value from the
top-K.
The `mode()` pattern this PR follows avoids that by keying on the
`UTF8String` collation key directly (`Mode.scala:118`), which is byte-exact.
Consider keying `CollatedString` on the `UTF8String` (or the raw key bytes)
rather than the decoded `String`. Non-blocking — the lossy decode predates this
PR — but since this path is being reworked and `CollatedString` is new, it's a
clean spot to close the gap.
--
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]
