MaxGekk commented on code in PR #47771:
URL: https://github.com/apache/spark/pull/47771#discussion_r1739193793
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -550,6 +549,152 @@ public static UTF8String toTitleCase(final UTF8String
target, final int collatio
BreakIterator.getWordInstance(locale)));
}
+ /**
+ * This 'HashMap' is introduced as a performance speedup. Since title-casing
a codepoint can
Review Comment:
Is this performance speedup reflected somehow in the collation benchmark? If
not, how about to add new benchmarks to `CollationBenchmark`?
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -48,6 +49,16 @@ public class CollationAwareUTF8String {
*/
private static final int MATCH_NOT_FOUND = -1;
+ /**
+ * `COMBINED_ASCII_SMALL_I_COMBINING_DOT` is an internal representation of
the combined
+ * lowercase code point for ASCII lowercase letter i with an additional
combining dot character
+ * (U+0307). This integer value is not a valid code point itself, but rather
an artificial code
+ * point marker used to represent the two lowercase characters that are the
result of converting
+ * the uppercase Turkish dotted letter I with a combining dot character
(U+0130) to lowercase.
+ */
+ private static final int COMBINED_ASCII_SMALL_I_COMBINING_DOT =
+ SpecialCodePointConstants.ASCII_SMALL_I << 16 |
SpecialCodePointConstants.COMBINING_DOT;
Review Comment:
adjust indentation
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -550,6 +549,152 @@ public static UTF8String toTitleCase(final UTF8String
target, final int collatio
BreakIterator.getWordInstance(locale)));
}
+ /**
+ * This 'HashMap' is introduced as a performance speedup. Since title-casing
a codepoint can
+ * result in more than a single codepoint, for correctness, we would use
+ * 'UCharacter.toTitleCase(String)' which returns a 'String'. If we use
+ * 'UCharacter.toTitleCase(int)' (the version of the same function which
converts a single
+ * codepoint to its title-case codepoint), it would be faster than the
previously mentioned
+ * version, but the problem here is that we don't handle when title-casing a
codepoint yields more
+ * than 1 codepoint. Since there are only 48 codepoints that are mapped to
more than 1 codepoint
+ * when title-cased, they are precalculated here, so that the faster
function for title-casing
+ * could be used in combination with this 'HashMap' in the method
'appendCodepointToTitleCase'.
+ */
+ private static final HashMap<Integer, String>
codepointOneToManyTitleCaseLookupTable =
+ new HashMap<>(){{
+ StringBuilder sb = new StringBuilder();
+ for (int i = Character.MIN_CODE_POINT; i <= Character.MAX_CODE_POINT; ++i)
{
+ sb.appendCodePoint(i);
+ String titleCase = UCharacter.toTitleCase(sb.toString(), null);
+ if (titleCase.codePointCount(0, titleCase.length()) > 1) {
+ put(i, titleCase);
+ }
+ sb.setLength(0);
+ }
+ }};
+
+ /**
+ * Title-casing a string using ICU case mappings. Iterates over the string
and title-cases
+ * the first character in each word, and lowercases every other character.
Handles lowercasing
+ * capital Greek letter sigma ('Σ') separately, taking into account if it
should be a small final
+ * Greek sigma ('ς') or small non-final Greek sigma ('σ'). Words are
separated by ASCII
+ * space(\u0020).
+ *
+ * @param target UTF8String to be title cased
+ * @return title cased target
+ */
+ public static UTF8String toTitleCaseICU(UTF8String target) {
+ // In the default UTF8String implementation, `toLowerCase` method
implicitly does UTF8String
+ // validation (replacing invalid UTF-8 byte sequences with Unicode
replacement character
+ // U+FFFD), but now we have to do the validation manually.
+ target = target.makeValid();
+
+ // Building the title cased target with 'sb'.
+ UTF8StringBuilder sb = new UTF8StringBuilder();
+
+ // 'newWord' is true if the current character is the beginning of a word,
false otherwise.
+ boolean newWord = true;
Review Comment:
nit: isNewWord
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -550,6 +549,152 @@ public static UTF8String toTitleCase(final UTF8String
target, final int collatio
BreakIterator.getWordInstance(locale)));
}
+ /**
+ * This 'HashMap' is introduced as a performance speedup. Since title-casing
a codepoint can
+ * result in more than a single codepoint, for correctness, we would use
+ * 'UCharacter.toTitleCase(String)' which returns a 'String'. If we use
+ * 'UCharacter.toTitleCase(int)' (the version of the same function which
converts a single
+ * codepoint to its title-case codepoint), it would be faster than the
previously mentioned
+ * version, but the problem here is that we don't handle when title-casing a
codepoint yields more
+ * than 1 codepoint. Since there are only 48 codepoints that are mapped to
more than 1 codepoint
+ * when title-cased, they are precalculated here, so that the faster
function for title-casing
+ * could be used in combination with this 'HashMap' in the method
'appendCodepointToTitleCase'.
+ */
+ private static final HashMap<Integer, String>
codepointOneToManyTitleCaseLookupTable =
+ new HashMap<>(){{
+ StringBuilder sb = new StringBuilder();
+ for (int i = Character.MIN_CODE_POINT; i <= Character.MAX_CODE_POINT; ++i)
{
+ sb.appendCodePoint(i);
+ String titleCase = UCharacter.toTitleCase(sb.toString(), null);
+ if (titleCase.codePointCount(0, titleCase.length()) > 1) {
+ put(i, titleCase);
+ }
+ sb.setLength(0);
+ }
+ }};
+
+ /**
+ * Title-casing a string using ICU case mappings. Iterates over the string
and title-cases
+ * the first character in each word, and lowercases every other character.
Handles lowercasing
+ * capital Greek letter sigma ('Σ') separately, taking into account if it
should be a small final
+ * Greek sigma ('ς') or small non-final Greek sigma ('σ'). Words are
separated by ASCII
+ * space(\u0020).
+ *
+ * @param target UTF8String to be title cased
+ * @return title cased target
+ */
+ public static UTF8String toTitleCaseICU(UTF8String target) {
Review Comment:
confusing name `target`. Isn't this `source`, but the result is a target?
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -550,6 +549,152 @@ public static UTF8String toTitleCase(final UTF8String
target, final int collatio
BreakIterator.getWordInstance(locale)));
}
+ /**
+ * This 'HashMap' is introduced as a performance speedup. Since title-casing
a codepoint can
+ * result in more than a single codepoint, for correctness, we would use
+ * 'UCharacter.toTitleCase(String)' which returns a 'String'. If we use
+ * 'UCharacter.toTitleCase(int)' (the version of the same function which
converts a single
+ * codepoint to its title-case codepoint), it would be faster than the
previously mentioned
+ * version, but the problem here is that we don't handle when title-casing a
codepoint yields more
+ * than 1 codepoint. Since there are only 48 codepoints that are mapped to
more than 1 codepoint
Review Comment:
For just 48 elements, isn't linear search in an array faster than the hash
map?
--
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]
