[SPARK-47327][SQL] Fix thread safety issue in ICU Collator #45436
Conversation
| @@ -138,11 +138,13 @@ public Collation( | |||
| collationTable[2] = new Collation( | |||
| "UNICODE", Collator.getInstance(ULocale.ROOT), "153.120.0.0", true); | |||
| collationTable[2].collator.setStrength(Collator.TERTIARY); | |||
| collationTable[2].collator.freeze(); | |||
There was a problem hiding this comment.
I've never seen "freeze" before, so I'm wondering how this affects us.
As I understood from the ICU docs: Once frozen, an object can never be unfrozen, so it is thread-safe from that point onward.
So what are the drawbacks of this apprach?
There was a problem hiding this comment.
This is interesting, I thought Collator is just holding a bunch of functions, but it has mutable states?
There was a problem hiding this comment.
@cloud-fan collator uses a buffer while writing collation keys, freezing it makes this operation safe by using a reentrant lock around it (source)
this of course raises performance issues which we should probably discuss, because now we can't generate sort keys in parallel on a single collator
There was a problem hiding this comment.
they should use one buffer per thread... Anyway this is out of our control and calling freeze LGTM
There was a problem hiding this comment.
Yeah, as soon as we get benchmarks working we should revisit this decision.
One option that we also prototyped is to keep Collator in ThreadLocal fields, which also solved the problem. But freeze is a bit cleaner and we don't have microbenchmarks yet so we can't make data driven decision at this point.
LGTM.
| // generating ICU sort keys is not thread-safe by default so this should fail | ||
| // if we don't handle the concurrency properly on Collator level | ||
|
|
||
| for (_ <- 1 to 100) { |
There was a problem hiding this comment.
just wondering: have you tried 1000, 10000, etc. or is 100 proven to be sufficient?
| @@ -438,6 +438,39 @@ class CollationSuite extends DatasourceV2SQLBase with AdaptiveSparkPlanHelper { | |||
| } | |||
| } | |||
|
|
|||
| test("test concurrently running aggregates") { | |||
There was a problem hiding this comment.
on another note, do we have some kind of truly multithread tests?
| ("unicode_CI", Seq("aaa", "aaa"), Seq(Row(2, "aaa"))), | ||
| ("unicode_CI", Seq("AAA", "aaa"), Seq(Row(2, "AAA"))), | ||
| ("unicode_CI", Seq("aaa", "bbb"), Seq(Row(1, "aaa"), Row(1, "bbb"))) | ||
| ).foreach { |
There was a problem hiding this comment.
How this sequential execution proofs the fix? Maybe use a parallel collection?
There was a problem hiding this comment.
that's a very good point, the test would fail simply because it would be ran 100 times and at least one of those execution would have a data race - i improved it now to just call getCollationKey in a parallel for each and not really on spark's execution of the aggregate query
There was a problem hiding this comment.
PySpark test failures look unrelated. I think #45436 (comment) has a point - might need to double check to make sure.
Otherwise, LGTM
| @@ -438,6 +439,19 @@ class CollationSuite extends DatasourceV2SQLBase with AdaptiveSparkPlanHelper { | |||
| } | |||
| } | |||
|
|
|||
| test("test concurrently generating collation keys") { | |||
There was a problem hiding this comment.
Can this test go to CollationFactorySuite?
There was a problem hiding this comment.
it could but I decided to put it here because it would require adding a new dependency for parallel collections which I'd like to avoid
|
Merged to master. |
### What changes were proposed in this pull request? Freezing the ICU collator upon creation. ### Why are the changes needed? In order to avoid multiple threads writing to the collation buffer during the generation of collation sort keys which then results in data corruption and an internal error. You can read more about collator thread safety [here](https://unicode-org.github.io/icu/userguide/icu/design.html#icu-threading-model-and-open-and-close-model) ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? New unti test ### Was this patch authored or co-authored using generative AI tooling? no Closes apache#45436 from stefankandic/icuConcurrencyIssue. Authored-by: Stefan Kandic <stefan.kandic@databricks.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
### What changes were proposed in this pull request? Freezing the ICU collator upon creation. ### Why are the changes needed? In order to avoid multiple threads writing to the collation buffer during the generation of collation sort keys which then results in data corruption and an internal error. You can read more about collator thread safety [here](https://unicode-org.github.io/icu/userguide/icu/design.html#icu-threading-model-and-open-and-close-model) ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? New unti test ### Was this patch authored or co-authored using generative AI tooling? no Closes apache#45436 from stefankandic/icuConcurrencyIssue. Authored-by: Stefan Kandic <stefan.kandic@databricks.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
What changes were proposed in this pull request?
Freezing the ICU collator upon creation.
Why are the changes needed?
In order to avoid multiple threads writing to the collation buffer during the generation of collation sort keys which then results in data corruption and an internal error.
You can read more about collator thread safety here
Does this PR introduce any user-facing change?
No
How was this patch tested?
New unti test
Was this patch authored or co-authored using generative AI tooling?
no