mkaravel commented on code in PR #47014:
URL: https://github.com/apache/spark/pull/47014#discussion_r1653313761
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -382,12 +395,17 @@ public static String toLowerCase(final String target) {
* @return the lowercase string
*/
public static UTF8String toLowerCase(final UTF8String target, final int
collationId) {
- return UTF8String.fromString(toLowerCase(target.toString(), collationId));
+ if (target.isFullAscii()) return target.toLowerCaseAscii();
+ return toLowerCaseSlow(target, collationId);
}
- public static String toLowerCase(final String target, final int collationId)
{
+
+ private static UTF8String toLowerCaseSlow(final UTF8String target, final int
collationId) {
+ // Note: In order to achieve the desired behaviour, we use the ICU
UCharacter class to
+ // convert the string to lowercase, which only accepts a Java strings as
input.
ULocale locale = CollationFactory.fetchCollation(collationId)
.collator.getLocale(ULocale.ACTUAL_LOCALE);
- return UCharacter.toLowerCase(locale, target);
+ // TODO: All UTF8String -> String conversions should use `makeValid`
(SPARK-48715)
Review Comment:
And of course here.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -369,10 +377,15 @@ public static String toUpperCase(final String target,
final int collationId) {
* @return the lowercase string
*/
public static UTF8String toLowerCase(final UTF8String target) {
- return UTF8String.fromString(toLowerCase(target.toString()));
+ if (target.isFullAscii()) return target.toLowerCaseAscii();
+ return toLowerCaseSlow(target);
}
- public static String toLowerCase(final String target) {
- return UCharacter.toLowerCase(target);
+
+ private static UTF8String toLowerCaseSlow(final UTF8String target) {
+ // Note: In order to achieve the desired behaviour, we use the ICU
UCharacter class to
+ // convert the string to lowercase, which only accepts a Java strings as
input.
+ // TODO: All UTF8String -> String conversions should use `makeValid`
(SPARK-48715)
Review Comment:
And here please.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -353,13 +357,17 @@ public static String toUpperCase(final String target) {
* @return the uppercase string
*/
public static UTF8String toUpperCase(final UTF8String target, final int
collationId) {
- return UTF8String.fromString(toUpperCase(target.toString(), collationId));
+ if (target.isFullAscii()) return target.toUpperCaseAscii();
+ return toUpperCaseSlow(target, collationId);
}
- public static String toUpperCase(final String target, final int collationId)
{
+ private static UTF8String toUpperCaseSlow(final UTF8String target, final int
collationId) {
+ // Note: In order to achieve the desired behaviour, we use the ICU
UCharacter class to
+ // convert the string to uppercase, which only accepts a Java strings as
input.
ULocale locale = CollationFactory.fetchCollation(collationId)
.collator.getLocale(ULocale.ACTUAL_LOCALE);
- return UCharacter.toUpperCase(locale, target);
+ // TODO: All UTF8String -> String conversions should use `makeValid`
(SPARK-48715)
Review Comment:
See my other comment about this.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -339,11 +339,15 @@ public static UTF8String lowercaseReplace(final
UTF8String src, final UTF8String
* @return the uppercase string
*/
public static UTF8String toUpperCase(final UTF8String target) {
- return UTF8String.fromString(toUpperCase(target.toString()));
+ if (target.isFullAscii()) return target.toUpperCaseAscii();
+ return toUpperCaseSlow(target);
}
- public static String toUpperCase(final String target) {
- return UCharacter.toUpperCase(target);
+ private static UTF8String toUpperCaseSlow(final UTF8String target) {
+ // Note: In order to achieve the desired behaviour, we use the ICU
UCharacter class to
+ // convert the string to uppercase, which only accepts a Java strings as
input.
+ // TODO: All UTF8String -> String conversions should use `makeValid`
(SPARK-48715)
Review Comment:
I believe the proper etiquette for `TODO` comments is to either add a JIRA
or a user name.
So this should probably be:
```java
// TODO(SPARK-48715): All UTF8String -> String conversions should use
`makeValid`
```
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -424,36 +442,39 @@ else if (codePoint == 0x03C2) {
* @param target The target string to convert to lowercase.
* @return The string converted to lowercase in a context-unaware manner.
*/
- public static String lowerCaseCodePoints(final String target) {
+ public static UTF8String lowerCaseCodePoints(final UTF8String target) {
+ if (target.isFullAscii()) return target.toLowerCaseAscii();
+ return lowerCaseCodePointsSlow(target);
+ }
+
+ private static UTF8String lowerCaseCodePointsSlow(final UTF8String target) {
+ String targetString = target.toString();
StringBuilder sb = new StringBuilder();
- for (int i = 0; i < target.length(); ++i) {
- lowercaseCodePoint(target.codePointAt(i), sb);
+ for (int i = 0; i < targetString.length(); ++i) {
+ lowercaseCodePoint(targetString.codePointAt(i), sb);
}
- return sb.toString();
+ return UTF8String.fromString(sb.toString());
}
/**
* Convert the input string to titlecase using the ICU root locale rules.
*/
public static UTF8String toTitleCase(final UTF8String target) {
- return UTF8String.fromString(toTitleCase(target.toString()));
- }
-
- public static String toTitleCase(final String target) {
- return UCharacter.toTitleCase(target, BreakIterator.getWordInstance());
+ // Note: In order to achieve the desired behaviour, we use the ICU
UCharacter class to
+ // convert the string to titlecase, which only accepts a Java strings as
input.
+ // TODO: All UTF8String -> String conversions should use `makeValid`
(SPARK-48715)
+ return UTF8String.fromString(UCharacter.toTitleCase(target.toString(),
+ BreakIterator.getWordInstance()));
}
/**
* Convert the input string to titlecase using the specified ICU collation
rules.
*/
public static UTF8String toTitleCase(final UTF8String target, final int
collationId) {
- return UTF8String.fromString(toTitleCase(target.toString(), collationId));
- }
-
- public static String toTitleCase(final String target, final int collationId)
{
ULocale locale = CollationFactory.fetchCollation(collationId)
.collator.getLocale(ULocale.ACTUAL_LOCALE);
- return UCharacter.toTitleCase(locale, target,
BreakIterator.getWordInstance(locale));
+ return UTF8String.fromString(UCharacter.toTitleCase(locale,
target.toString(),
Review Comment:
And I would suggest to add the same TODO here for completeness.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -424,36 +442,39 @@ else if (codePoint == 0x03C2) {
* @param target The target string to convert to lowercase.
* @return The string converted to lowercase in a context-unaware manner.
*/
- public static String lowerCaseCodePoints(final String target) {
+ public static UTF8String lowerCaseCodePoints(final UTF8String target) {
+ if (target.isFullAscii()) return target.toLowerCaseAscii();
+ return lowerCaseCodePointsSlow(target);
+ }
+
+ private static UTF8String lowerCaseCodePointsSlow(final UTF8String target) {
+ String targetString = target.toString();
Review Comment:
I believe having the same TODO here would help (to keep track of what needs
to be changed).
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -424,36 +442,39 @@ else if (codePoint == 0x03C2) {
* @param target The target string to convert to lowercase.
* @return The string converted to lowercase in a context-unaware manner.
*/
- public static String lowerCaseCodePoints(final String target) {
+ public static UTF8String lowerCaseCodePoints(final UTF8String target) {
+ if (target.isFullAscii()) return target.toLowerCaseAscii();
+ return lowerCaseCodePointsSlow(target);
+ }
+
+ private static UTF8String lowerCaseCodePointsSlow(final UTF8String target) {
+ String targetString = target.toString();
StringBuilder sb = new StringBuilder();
- for (int i = 0; i < target.length(); ++i) {
- lowercaseCodePoint(target.codePointAt(i), sb);
+ for (int i = 0; i < targetString.length(); ++i) {
+ lowercaseCodePoint(targetString.codePointAt(i), sb);
}
- return sb.toString();
+ return UTF8String.fromString(sb.toString());
}
/**
* Convert the input string to titlecase using the ICU root locale rules.
*/
public static UTF8String toTitleCase(final UTF8String target) {
- return UTF8String.fromString(toTitleCase(target.toString()));
- }
-
- public static String toTitleCase(final String target) {
- return UCharacter.toTitleCase(target, BreakIterator.getWordInstance());
+ // Note: In order to achieve the desired behaviour, we use the ICU
UCharacter class to
+ // convert the string to titlecase, which only accepts a Java strings as
input.
+ // TODO: All UTF8String -> String conversions should use `makeValid`
(SPARK-48715)
Review Comment:
And here.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
