jiwen624 commented on code in PR #57177:
URL: https://github.com/apache/spark/pull/57177#discussion_r3582610284
##########
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:
Thanks for the review @cloud-fan !As mentioned in the PR description this
issue is found while working on this PR and a follow-up ticket is raised at
https://issues.apache.org/jira/browse/SPARK-58096 . The fix is not introduced
to this PR to avoid confusion since the lossy decode is a different issue that
predates this PR and the PR itself is already big.
Does it make sense to you?
--
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]
