You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "jiwen624 (via GitHub)" <gi...@apache.org> on 2026/07/09 23:40:43 UTC
[PR] [SPARK-58069][SQL] approx_top_k returns incorrect/garbage sort-key bytes instead of actual values for collated strings [spark]
jiwen624 opened a new pull request, #57177:
URL: https://github.com/apache/spark/pull/57177
### What changes were proposed in this pull request?
For a non-`UTF8_BINARY` collated STRING column, `approx_top_k` (and `approx_top_k_accumulate` / `_combine` / `_estimate`) stored the collation sort key in the sketch and returned it verbatim as the item. This makes the sketch store `CollatedString` instead: it dedupes by the collation key (so collation-equal values still count as one) but retains an actual input value to return, the way `mode()` does. The serde writes only the original value and recomputes the key on read, so the `UTF8_BINARY` format is unchanged.
### Why are the changes needed?
The returned items were wrong: raw ICU sort-key bytes (invalid UTF-8) for ICU collations, and the lower-cased form for `UTF8_LCASE` (a value that may never have appeared). Counts were correct, but the item labels were garbage. Silent wrong result, no error.
Before the fix:
```sql
scala> spark.sql("""SELECT approx_top_k(c, 5, 100) FROM (
| SELECT CAST(col AS STRING COLLATE UNICODE_CI) AS c
| FROM VALUES ('HELLO'), ('HELLO'), ('HELLO'), ('hello'), ('world') AS t(col)
| )""").show(100, false)
+--------------------------------+
|approx_top_k(c, 5, 100) |
+--------------------------------+
|[{93AAG\t, 4}, {WGMA1\t, 1}]|
+--------------------------------+
scala> spark.sql("""SELECT approx_top_k(c, 5, 100) FROM (
| SELECT CAST(col AS STRING COLLATE UNICODE) AS c
| FROM VALUES ('HELLO'), ('HELLO'), ('HELLO'), ('hello'), ('world') AS t(col)
| )""").show(100, false)
+------------------------------------------------------------+
|approx_top_k(c, 5, 100) |
+------------------------------------------------------------+
|[{93AAG\t�����, 3}, {93AAG\t\t, 1}, {WGMA1\t\t, 1}]|
+------------------------------------------------------------+
scala> spark.sql("""SELECT approx_top_k(c, 5, 100) FROM (
| SELECT CAST(col AS STRING COLLATE UTF8_LCASE) AS c
| FROM VALUES ('HELLO'), ('HELLO'), ('HELLO'), ('hello'), ('world') AS t(col)
| )""").show(100, false)
+------------------------+
|approx_top_k(c, 5, 100) |
+------------------------+
|[{hello, 4}, {world, 1}]|
+------------------------+
```
### Does this PR introduce _any_ user-facing change?
Yes. approx_top_k over a collated string column now returns an actual input value instead of the internal collation key. Sketches persisted by approx_top_k_accumulate over a non-binary collated column before this change are not readable after it (they contained wrong data); the UTF8_BINARY format is unchanged.
### How was this patch tested?
Added test cases.
### Was this patch authored or co-authored using generative AI tooling?
Yes.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58069][SQL] approx_top_k returns incorrect/garbage sort-key bytes instead of actual values for collated strings [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #57177: [SPARK-58069][SQL] approx_top_k returns incorrect/garbage sort-key bytes instead of actual values for collated strings
URL: https://github.com/apache/spark/pull/57177
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58069][SQL] approx_top_k returns incorrect/garbage sort-key bytes instead of actual values for collated strings [spark]
Posted by "jiwen624 (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58069][SQL] approx_top_k returns incorrect/garbage sort-key bytes instead of actual values for collated strings [spark]
Posted by "jiwen624 (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58069][SQL] approx_top_k returns incorrect/garbage sort-key bytes instead of actual values for collated strings [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #57177:
URL: https://github.com/apache/spark/pull/57177#issuecomment-4977263693
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/384d5e869861c802373a6f9e936bdd9e7b3465bb
- merged into branch-4.x https://github.com/apache/spark/commit/addcf3e73822e6871ff115df883592612b6090ae
*Posted by `merge_spark_pr.py`*
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58069][SQL] approx_top_k returns incorrect/garbage sort-key bytes instead of actual values for collated strings [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58069][SQL] approx_top_k returns incorrect/garbage sort-key bytes instead of actual values for collated strings [spark]
Posted by "jiwen624 (via GitHub)" <gi...@apache.org>.
jiwen624 commented on PR #57177:
URL: https://github.com/apache/spark/pull/57177#issuecomment-4963818388
Hi @cloud-fan @gengliangwang @yhuang-db since you've worked on and/or reviewed code of this area, could you take a look at this bug fix when you get a chance? Thanks.
Note that as mentioned in the PR description, there are two follow-up Jiras/fixes that need to be addressed after this PR.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org