uros-b commented on code in PR #57900:
URL: https://github.com/apache/spark/pull/57900#discussion_r3751810918
##########
sql/core/src/test/scala/org/apache/spark/sql/collation/CollationSuite.scala:
##########
@@ -96,6 +96,18 @@ class CollationSuite extends DatasourceV2SQLBase with
AdaptiveSparkPlanHelper {
}
}
+ test("initcap uses locale-specific collation rules") {
+ Seq(
+ ("NL", "Erik IJzermans"),
+ ("UTF8_LCASE", "Erik Ijzermans"),
+ ("UTF8_BINARY", "Erik Ijzermans")
Review Comment:
Please add UNICODE & UNICODE_CI cases, for completeness.
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -1410,6 +1410,9 @@ public void testInitCap() throws SparkException {
assertInitCap("ÄBĆΔE", UNICODE_CI, "Äbćδe");
assertInitCap("êéfgh", "AF_CI_AI", "Êéfgh");
assertInitCap("öoAÄ", "DE_CI_AI", "Öoaä");
+ assertInitCap("erik ijzermans", "NL", "Erik IJzermans");
+ assertInitCap("erik ijzermans", "NL_CI", "Erik IJzermans");
+ assertInitCap("erik ijzermans", UNICODE, "Erik Ijzermans");
Review Comment:
```suggestion
assertInitCap("erik ijzermans", UNICODE, "Erik Ijzermans");
assertInitCap("erik ijzermans", UNICODE_CI, "Erik Ijzermans");
```
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationFactory.java:
##########
@@ -111,6 +111,7 @@ public static class Collation {
public final String collationName;
public final String provider;
private final ThreadLocal<Collator> threadLocalCollator;
+ public final ULocale caseConversionLocale;
Review Comment:
The new field has no Javadoc, while every other public field on Collation is
documented. This one especially needs it, because "the requested locale,
deliberately not the collator's resolved actual locale" is exactly the kind of
thing someone will "fix" back in six months. The ULocale.ROOT passed for the
Spark-provider collations deserves a word too; UTF8_BINARY and UTF8_LCASE never
reach these code paths, since CollationSupport.InitCap.exec and friends
dispatch them to the non-collation overloads.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationFactory.java:
##########
@@ -1145,6 +1151,7 @@ private static class IndeterminateCollation extends
Collation {
"null",
"null",
null,
+ ULocale.ROOT,
(s1, s2) -> {
throw indeterminateError();
},
Review Comment:
One thing that should be fixed: the indeterminate-collation guard is
silently dropped.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationFactory.java:
##########
@@ -111,6 +111,7 @@ public static class Collation {
public final String collationName;
public final String provider;
private final ThreadLocal<Collator> threadLocalCollator;
+ public final ULocale caseConversionLocale;
Review Comment:
One open question is whether to add a config or not. In any case, we should
probably update the migration guide (starting 4.3+, since these changes will
likely land in 4.3.0).
Blast radius is narrower than the description suggests. I enumerated all 145
ICU locales Spark exposes and diffed old-locale vs. new-locale output for
upper, lower, and titlecase over a full sweep of defined code points up to
U+2FFFF plus curated Turkish/Greek/German/Lithuanian/Dutch strings.
- Locales where ACTUAL_LOCALE differs from requested | 43 of 145
- Locales where upper output changes | 0
- Locales where lower output changes | 0
- Locales where initcap output changes | 1 (nl)
The locales with real case tailorings (tr, az, lt, el) all have collation
tailorings too, so their collator already resolved to the right locale and
nothing changes for them. Dutch is the only one that falls through, and only
for titlecasing. Worth stating in the "user-facing change" section, since
"correctness fix for collation-aware case conversion" reads much broader than
it is. It also explains why no golden files need regenerating — I confirmed
nothing under sql/ uses COLLATE NL.
Given that, I don't think a legacy config is warranted. A line in
docs/sql-migration-guide.md would still be reasonable: a future ICU upgrade
could add case tailorings for locales that lack collation tailorings, and those
would then start changing results silently through this code path.
--
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]
