You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "uros-db (via GitHub)" <gi...@apache.org> on 2024/05/09 15:31:45 UTC
[PR] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation [spark]
uros-db opened a new pull request, #46511:
URL: https://github.com/apache/spark/pull/46511
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1604485097
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -709,12 +774,24 @@ public void testLocate() throws SparkException {
assertLocate("大千", "test大千世界大千世界", 9, "UNICODE_CI", 9);
assertLocate("大千", "大千世界大千世界", 1, "UNICODE_CI", 1);
// Case-variable character length
+ assertLocate("\u0307", "i̇", 1, "UTF8_BINARY", 2);
+ assertLocate("\u0307", "İ", 1, "UTF8_BINARY_LCASE", 0); // != UTF8_BINARY
Review Comment:
not wrong, but the intent was a bit different than other cases: instead of comparing differences between UNICODE_CI and UTF8_BINARY_LCASE, I wanted to compare UTF8_BINARY and UTF8_BINARY_LCASE - essentially ensuring that the new UTF8_BINARY_LCASE (character-wise) searching logic does `not` equal to applying UTF8_BINARY (byte-wise) searching logic on lowercased strings
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1604439268
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with
+ * the specified prefix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the prefix is already lowercased. The method only
+ * considers the part of target string that starts from the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the first occurrence of the pattern string
+ * in the target string from the specified position (0-based index),
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the pattern string is already lowercased prior to method call.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return the position of the first occurrence of pattern in target, if not found, -1 returned.
+ */
+ public static int lowercaseFind(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ for (int i = startPos; i <= target.numChars(); ++i) {
+ if (lowercaseMatchFrom(target, lowercasePattern, i)) {
+ return i;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns whether the target string ends with the specified suffix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the suffix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same suffix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return whether the target string ends with the specified suffix in lowercase
+ */
+ public static boolean lowercaseMatchUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ return lowercaseMatchLengthUntil(target, lowercasePattern, endPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that ends with
+ * the specified suffix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the suffix is already lowercased. The method only
+ * considers the part of target string that ends at the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ assert endPos <= target.numChars();
+ for (int len = 0; len <= endPos; ++len) {
+ if (target.substring(endPos - len, endPos).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the last occurrence of the pattern string
+ * in the target string until the specified position (0-based index),
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the pattern string is already lowercased prior to method call.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return the position of the last occurrence of pattern in target, if not found, -1 returned.
+ */
+ public static int lowercaseRFind(
Review Comment:
UTF8String uses `find` and `rfind`, so I suggest we stick with `lowercaseFind` and `lowercaseRFind`
--
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-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1609894240
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,155 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found when searching for a pattern
+ * string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix, starting from the
+ * specified position (0-based index referring to character position in UTF8String), with respect
+ * to the UTF8_BINARY_LCASE collation. The method assumes that the prefix is already lowercased
+ * prior to method call to avoid the overhead of calling .toLowerCase() multiple times on the
+ * same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with the specified
+ * prefix, starting from the specified position (0-based index referring to character position
+ * in UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * prefix is already lowercased. The method only considers the part of target string that
+ * starts from the specified (inclusive) position (that is, the method does not look at UTF8
+ * characters of the target string at or after position `endPos`). If the prefix is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return length of the target substring that ends with the specified prefix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the first occurrence of the pattern string in the target string,
+ * starting from the specified position (0-based index referring to character position in
+ * UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * pattern string is already lowercased prior to call. If the pattern is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return the position of the first occurrence of pattern in target
+ */
+ public static int lowercaseFind(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int i = startPos; i <= target.numChars(); ++i) {
+ if (lowercaseMatchFrom(target, lowercasePattern, i)) {
+ return i;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns whether the target string ends with the specified suffix, ending at the specified
+ * position (0-based index referring to character position in UTF8String), with respect to the
+ * UTF8_BINARY_LCASE collation. The method assumes that the suffix is already lowercased prior
+ * to method call to avoid the overhead of calling .toLowerCase() multiple times on the same
+ * suffix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return whether the target string ends with the specified suffix in lowercase
+ */
+ public static boolean lowercaseMatchUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ return lowercaseMatchLengthUntil(target, lowercasePattern, endPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that ends with the specified
+ * suffix, ending at the specified position (0-based index referring to character position in
+ * UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * suffix is already lowercased. The method only considers the part of target string that ends
+ * at the specified (non-inclusive) position (that is, the method does not look at UTF8
+ * characters of the target string at or after position `endPos`). If the suffix is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ assert endPos <= target.numChars();
+ for (int len = 0; len <= endPos; ++len) {
+ if (target.substring(endPos - len, endPos).toLowerCase().equals(lowercasePattern)) {
Review Comment:
> Is it the same if we lower case the entire target string first
no, it's not - the changes in this PR (and some follow-up PRs) are related to changes
consider the example in the PR description: `contains("İ", "i")` where `haystack = "İ"` & `needle = "i"`
`lower(haystack) = lower("İ") = "i\u0307"` would produce `substr(lower("İ"), 1, 1) = "i" = needle`
however, we can see that these is no `substr(haystack, start, len)` such that `lower(substr(haystack, start, len)) == needle` - so these 2 behaviours are not actually equivalent
--
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-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1609897510
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,155 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found when searching for a pattern
+ * string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix, starting from the
+ * specified position (0-based index referring to character position in UTF8String), with respect
+ * to the UTF8_BINARY_LCASE collation. The method assumes that the prefix is already lowercased
+ * prior to method call to avoid the overhead of calling .toLowerCase() multiple times on the
+ * same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with the specified
+ * prefix, starting from the specified position (0-based index referring to character position
+ * in UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * prefix is already lowercased. The method only considers the part of target string that
+ * starts from the specified (inclusive) position (that is, the method does not look at UTF8
+ * characters of the target string at or after position `endPos`). If the prefix is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return length of the target substring that ends with the specified prefix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the first occurrence of the pattern string in the target string,
+ * starting from the specified position (0-based index referring to character position in
+ * UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * pattern string is already lowercased prior to call. If the pattern is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return the position of the first occurrence of pattern in target
+ */
+ public static int lowercaseFind(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int i = startPos; i <= target.numChars(); ++i) {
+ if (lowercaseMatchFrom(target, lowercasePattern, i)) {
+ return i;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns whether the target string ends with the specified suffix, ending at the specified
+ * position (0-based index referring to character position in UTF8String), with respect to the
+ * UTF8_BINARY_LCASE collation. The method assumes that the suffix is already lowercased prior
+ * to method call to avoid the overhead of calling .toLowerCase() multiple times on the same
+ * suffix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return whether the target string ends with the specified suffix in lowercase
+ */
+ public static boolean lowercaseMatchUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ return lowercaseMatchLengthUntil(target, lowercasePattern, endPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that ends with the specified
+ * suffix, ending at the specified position (0-based index referring to character position in
+ * UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * suffix is already lowercased. The method only considers the part of target string that ends
+ * at the specified (non-inclusive) position (that is, the method does not look at UTF8
+ * characters of the target string at or after position `endPos`). If the suffix is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ assert endPos <= target.numChars();
+ for (int len = 0; len <= endPos; ++len) {
+ if (target.substring(endPos - len, endPos).toLowerCase().equals(lowercasePattern)) {
Review Comment:
that said, we are focusing on correctness for now, and are aware of possible performance regression for string searching in UTF8_BINARY_LCASE - we intend to work on perf optimization in a subsequent 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
Re: [PR] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation [spark]
Posted by "mkaravel (via GitHub)" <gi...@apache.org>.
mkaravel commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1596266251
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -193,7 +197,10 @@ public static boolean execBinary(final UTF8String l, final UTF8String r) {
return l.endsWith(r);
}
public static boolean execLowercase(final UTF8String l, final UTF8String r) {
- return l.endsWithInLowerCase(r);
+ if (r.numBytes() == 0) return true;
+ if (l.numChars() < r.numChars()) return false;
Review Comment:
Same counterexample: Consider `l` being `İİİ` and `r` being `i̇i̇`. The answer should be true.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -118,7 +118,9 @@ public static boolean execBinary(final UTF8String l, final UTF8String r) {
return l.contains(r);
}
public static boolean execLowercase(final UTF8String l, final UTF8String r) {
- return l.containsInLowerCase(r);
+ if (r.numChars() == 0) return true;
+ if (l.numChars() < r.numChars()) return false;
Review Comment:
I don't think this is correct. Consider for example `l` being `İİİ` and `r` being `i̇i̇`. The result is obviously `true` but `l` has 3 characters and `r` has 4.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -183,6 +192,22 @@ public static int findInSet(final UTF8String match, final UTF8String set, int co
return 0;
}
+ public static int lowercaseIndexOf(final UTF8String target, final UTF8String pattern,
+ final int start) {
+ if (pattern.numChars() == 0) return 0;
+ int lenHaystack = target.numChars(), lenNeedle = pattern.numChars();
+ final UTF8String firstLower = pattern.substring(0, 1).toLowerCase();
+ final UTF8String needle = pattern.toLowerCase();
+ for (int i = start; i <= (lenHaystack - lenNeedle); i++) {
+ if (target.substring(i, i + 1).toLowerCase().equals(firstLower)) {
Review Comment:
What happens if `target` contains `i̇` at positions i and i+1 and `firstLower` is `İ`? I think this would give the wrong result.
Consider for example: `l` equals `i̇i̇` and `r` is `İ`. We want to find, if exists, the first a position `p` and a length `n` for `l` such that `substr(l, p, n)` lowercase-equals `r`. Here the return value for the function should be 0 because for `p=0` and `n=4` we have a match.
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -191,20 +199,36 @@ public void testStartsWith() throws SparkException {
assertStartsWith("ab世De", "AB世dE", "UNICODE_CI", true);
assertStartsWith("äbćδe", "ÄbćδE", "UNICODE_CI", true);
assertStartsWith("äbćδe", "ÄBcΔÉ", "UNICODE_CI", false);
- // Case-variable character length
- assertStartsWith("İonic", "i̇o", "UNICODE_CI", true);
- assertStartsWith("i̇onic", "İo", "UNICODE_CI", true);
- assertStartsWith("the İodine", "the i̇odine", "UTF8_BINARY_LCASE", true);
- assertStartsWith("the i̇odine", "the İodine", "UTF8_BINARY_LCASE", true);
- assertStartsWith("İodiNe", "i̇oDin", "UTF8_BINARY_LCASE", true);
- assertStartsWith("The İodiNe", "i̇oDin", "UTF8_BINARY_LCASE", false);
// Characters with the same binary lowercase representation
assertStartsWith("Kelvin.", "Kelvin", "UTF8_BINARY_LCASE", true);
assertStartsWith("Kelvin.", "Kelvin", "UTF8_BINARY_LCASE", true);
assertStartsWith("KKelvin.", "KKelvin", "UTF8_BINARY_LCASE", true);
assertStartsWith("2 Kelvin.", "2 Kelvin", "UTF8_BINARY_LCASE", true);
assertStartsWith("2 Kelvin.", "2 Kelvin", "UTF8_BINARY_LCASE", true);
assertStartsWith("KKelvin.", "KKelvin,", "UTF8_BINARY_LCASE", false);
+ // Case-variable character length
+ assertStartsWith("i̇", "i", "UNICODE_CI", false);
+ assertStartsWith("i̇", "İ", "UNICODE_CI", true);
+ assertStartsWith("İ", "i", "UNICODE_CI", false);
+ assertStartsWith("i̇onic", "io", "UNICODE_CI", false);
+ assertStartsWith("i̇onic", "Io", "UNICODE_CI", false);
+ assertStartsWith("i̇onic", "i̇o", "UNICODE_CI", true);
+ assertStartsWith("i̇onic", "İo", "UNICODE_CI", true);
+ assertStartsWith("İonic", "io", "UNICODE_CI", false);
+ assertStartsWith("İonic", "Io", "UNICODE_CI", false);
+ assertStartsWith("İonic", "i̇o", "UNICODE_CI", true);
+ assertStartsWith("İonic", "İo", "UNICODE_CI", true);
+ assertStartsWith("i̇", "i", "UTF8_BINARY_LCASE", true); // note: different from UNICODE_CI
+ assertStartsWith("i̇", "İ", "UTF8_BINARY_LCASE", false); // note: different from UNICODE_CI
+ assertStartsWith("İ", "i", "UTF8_BINARY_LCASE", false);
+ assertStartsWith("i̇onic", "io", "UTF8_BINARY_LCASE", false);
+ assertStartsWith("i̇onic", "Io", "UTF8_BINARY_LCASE", false);
+ assertStartsWith("i̇onic", "i̇o", "UTF8_BINARY_LCASE", true);
+ assertStartsWith("i̇onic", "İo", "UTF8_BINARY_LCASE", false); // note: different from UNICODE_CI
+ assertStartsWith("İonic", "io", "UTF8_BINARY_LCASE", false);
+ assertStartsWith("İonic", "Io", "UTF8_BINARY_LCASE", false);
+ assertStartsWith("İonic", "i̇o", "UTF8_BINARY_LCASE", false); // note: different from UNICODE_CI
Review Comment:
Should also be true. Here `substr('İonic', 1, 2)` is lowercase-equals to `i̇o`.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,15 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ public static boolean lowercaseMatchAt(final UTF8String l, final UTF8String r,
+ int pos, int len) {
+ if (len + pos > l.numChars() || pos < 0) {
+ return false;
+ }
Review Comment:
Would we ever pass a negative `pos`?
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -156,7 +158,9 @@ public static boolean execBinary(final UTF8String l, final UTF8String r) {
return l.startsWith(r);
}
public static boolean execLowercase(final UTF8String l, final UTF8String r) {
- return l.startsWithInLowerCase(r);
+ if (r.numBytes() == 0) return true;
+ if (l.numChars() < r.numChars()) return false;
Review Comment:
Same counterexample: Consider `l` being `İİİ` and `r` being `i̇i̇`. The answer should be true, but here we return false.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,15 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ public static boolean lowercaseMatchAt(final UTF8String l, final UTF8String r,
+ int pos, int len) {
Review Comment:
I think this API is a bit problematic. We want `len` to be the number of characters of `r` all the time, right?
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -191,20 +199,36 @@ public void testStartsWith() throws SparkException {
assertStartsWith("ab世De", "AB世dE", "UNICODE_CI", true);
assertStartsWith("äbćδe", "ÄbćδE", "UNICODE_CI", true);
assertStartsWith("äbćδe", "ÄBcΔÉ", "UNICODE_CI", false);
- // Case-variable character length
- assertStartsWith("İonic", "i̇o", "UNICODE_CI", true);
- assertStartsWith("i̇onic", "İo", "UNICODE_CI", true);
- assertStartsWith("the İodine", "the i̇odine", "UTF8_BINARY_LCASE", true);
- assertStartsWith("the i̇odine", "the İodine", "UTF8_BINARY_LCASE", true);
- assertStartsWith("İodiNe", "i̇oDin", "UTF8_BINARY_LCASE", true);
- assertStartsWith("The İodiNe", "i̇oDin", "UTF8_BINARY_LCASE", false);
// Characters with the same binary lowercase representation
assertStartsWith("Kelvin.", "Kelvin", "UTF8_BINARY_LCASE", true);
assertStartsWith("Kelvin.", "Kelvin", "UTF8_BINARY_LCASE", true);
assertStartsWith("KKelvin.", "KKelvin", "UTF8_BINARY_LCASE", true);
assertStartsWith("2 Kelvin.", "2 Kelvin", "UTF8_BINARY_LCASE", true);
assertStartsWith("2 Kelvin.", "2 Kelvin", "UTF8_BINARY_LCASE", true);
assertStartsWith("KKelvin.", "KKelvin,", "UTF8_BINARY_LCASE", false);
+ // Case-variable character length
+ assertStartsWith("i̇", "i", "UNICODE_CI", false);
+ assertStartsWith("i̇", "İ", "UNICODE_CI", true);
+ assertStartsWith("İ", "i", "UNICODE_CI", false);
+ assertStartsWith("i̇onic", "io", "UNICODE_CI", false);
+ assertStartsWith("i̇onic", "Io", "UNICODE_CI", false);
+ assertStartsWith("i̇onic", "i̇o", "UNICODE_CI", true);
+ assertStartsWith("i̇onic", "İo", "UNICODE_CI", true);
+ assertStartsWith("İonic", "io", "UNICODE_CI", false);
+ assertStartsWith("İonic", "Io", "UNICODE_CI", false);
+ assertStartsWith("İonic", "i̇o", "UNICODE_CI", true);
+ assertStartsWith("İonic", "İo", "UNICODE_CI", true);
+ assertStartsWith("i̇", "i", "UTF8_BINARY_LCASE", true); // note: different from UNICODE_CI
+ assertStartsWith("i̇", "İ", "UTF8_BINARY_LCASE", false); // note: different from UNICODE_CI
+ assertStartsWith("İ", "i", "UTF8_BINARY_LCASE", false);
+ assertStartsWith("i̇onic", "io", "UTF8_BINARY_LCASE", false);
+ assertStartsWith("i̇onic", "Io", "UTF8_BINARY_LCASE", false);
+ assertStartsWith("i̇onic", "i̇o", "UTF8_BINARY_LCASE", true);
+ assertStartsWith("i̇onic", "İo", "UTF8_BINARY_LCASE", false); // note: different from UNICODE_CI
Review Comment:
This should also be true. I can find a prefix substring of `i̇onic` that is lowercase-equal to `İo`. Specifically (1-based position notation): `substr('i̇onic', 1, 3)`.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,15 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ public static boolean lowercaseMatchAt(final UTF8String l, final UTF8String r,
+ int pos, int len) {
+ if (len + pos > l.numChars() || pos < 0) {
+ return false;
+ }
+ return l.substring(pos, pos + len).toLowerCase().equals(r);
Review Comment:
Shouldn't this be?
```java
return l.substring(pos, pos + len).toLowerCase().equals(r.toLowerCase());
```
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -102,20 +102,28 @@ public void testContains() throws SparkException {
assertContains("äbćδe", "ÄbćδE", "UNICODE_CI", true);
assertContains("äbćδe", "ÄBcΔÉ", "UNICODE_CI", false);
// Case-variable character length
- assertContains("abİo12", "i̇o", "UNICODE_CI", true);
- assertContains("abi̇o12", "İo", "UNICODE_CI", true);
- assertContains("the İodine", "the i̇odine", "UTF8_BINARY_LCASE", true);
- assertContains("the i̇odine", "the İodine", "UTF8_BINARY_LCASE", true);
- assertContains("The İodiNe", " i̇oDin", "UTF8_BINARY_LCASE", true);
- assertContains("İodiNe", "i̇oDine", "UTF8_BINARY_LCASE", true);
- assertContains("İodiNe", " i̇oDin", "UTF8_BINARY_LCASE", false);
- // Characters with the same binary lowercase representation
- assertContains("The Kelvin.", "Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("The Kelvin.", "Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("The KKelvin.", "KKelvin", "UTF8_BINARY_LCASE", true);
- assertContains("2 Kelvin.", "2 Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("2 Kelvin.", "2 Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("The KKelvin.", "KKelvin,", "UTF8_BINARY_LCASE", false);
+ assertContains("i̇", "i", "UNICODE_CI", false);
+ assertContains("i̇", "İ", "UNICODE_CI", true);
+ assertContains("İ", "i", "UNICODE_CI", false);
+ assertContains("adi̇os", "io", "UNICODE_CI", false);
+ assertContains("adi̇os", "Io", "UNICODE_CI", false);
+ assertContains("adi̇os", "i̇o", "UNICODE_CI", true);
+ assertContains("adi̇os", "İo", "UNICODE_CI", true);
+ assertContains("adİos", "io", "UNICODE_CI", false);
+ assertContains("adİos", "Io", "UNICODE_CI", false);
+ assertContains("adİos", "i̇o", "UNICODE_CI", true);
+ assertContains("adİos", "İo", "UNICODE_CI", true);
+ assertContains("i̇", "i", "UTF8_BINARY_LCASE", true); // note: different from UNICODE_CI
+ assertContains("i̇", "İ", "UTF8_BINARY_LCASE", false); // note: different from UNICODE_CI
+ assertContains("İ", "i", "UTF8_BINARY_LCASE", false);
+ assertContains("adi̇os", "io", "UTF8_BINARY_LCASE", false);
+ assertContains("adi̇os", "Io", "UTF8_BINARY_LCASE", false);
+ assertContains("adi̇os", "i̇o", "UTF8_BINARY_LCASE", true);
+ assertContains("adi̇os", "İo", "UTF8_BINARY_LCASE", false); // note: different from UNICODE_CI
+ assertContains("adİos", "io", "UTF8_BINARY_LCASE", false);
+ assertContains("adİos", "Io", "UTF8_BINARY_LCASE", false);
+ assertContains("adİos", "i̇o", "UTF8_BINARY_LCASE", false); // note: different from UNICODE_CI
Review Comment:
Why is this false? I can find a substring of `adİos` such that it is lowercase-equal to `i̇o`. The substring (1-based position counting) is `substr('adİos', 3, 2)`.
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -279,20 +303,36 @@ public void testEndsWith() throws SparkException {
assertEndsWith("ab世De", "AB世dE", "UNICODE_CI", true);
assertEndsWith("äbćδe", "ÄbćδE", "UNICODE_CI", true);
assertEndsWith("äbćδe", "ÄBcΔÉ", "UNICODE_CI", false);
- // Case-variable character length
- assertEndsWith("The İo", "i̇o", "UNICODE_CI", true);
- assertEndsWith("The i̇o", "İo", "UNICODE_CI", true);
- assertEndsWith("the İodine", "the i̇odine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("the i̇odine", "the İodine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("The İodiNe", "i̇oDine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("The İodiNe", "i̇oDin", "UTF8_BINARY_LCASE", false);
// Characters with the same binary lowercase representation
assertEndsWith("The Kelvin", "Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The Kelvin", "Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The KKelvin", "KKelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The 2 Kelvin", "2 Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The 2 Kelvin", "2 Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The KKelvin", "KKelvin,", "UTF8_BINARY_LCASE", false);
+ // Case-variable character length
+ assertEndsWith("i̇", "i", "UNICODE_CI", false);
+ assertEndsWith("i̇", "İ", "UNICODE_CI", true);
+ assertEndsWith("İ", "i", "UNICODE_CI", false);
+ assertEndsWith("the i̇o", "io", "UNICODE_CI", false);
+ assertEndsWith("the i̇o", "Io", "UNICODE_CI", false);
+ assertEndsWith("the i̇o", "i̇o", "UNICODE_CI", true);
+ assertEndsWith("the i̇o", "İo", "UNICODE_CI", true);
+ assertEndsWith("the İo", "io", "UNICODE_CI", false);
+ assertEndsWith("the İo", "Io", "UNICODE_CI", false);
+ assertEndsWith("the İo", "i̇o", "UNICODE_CI", true);
+ assertEndsWith("the İo", "İo", "UNICODE_CI", true);
+ assertEndsWith("i̇", "i", "UTF8_BINARY_LCASE", false);
+ assertEndsWith("i̇", "İ", "UTF8_BINARY_LCASE", false); // note: different from UNICODE_CI
+ assertEndsWith("İ", "i", "UTF8_BINARY_LCASE", false);
+ assertEndsWith("the i̇o", "io", "UTF8_BINARY_LCASE", false);
+ assertEndsWith("the i̇o", "Io", "UTF8_BINARY_LCASE", false);
+ assertEndsWith("the i̇o", "i̇o", "UTF8_BINARY_LCASE", true);
+ assertEndsWith("the i̇o", "İo", "UTF8_BINARY_LCASE", false); // note: different from UNICODE_CI
Review Comment:
Should be true.
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -709,6 +749,16 @@ public void testLocate() throws SparkException {
assertLocate("大千", "test大千世界大千世界", 9, "UNICODE_CI", 9);
assertLocate("大千", "大千世界大千世界", 1, "UNICODE_CI", 1);
// Case-variable character length
+ assertLocate("̇", "i̇", 1, "UTF8_BINARY", 2);
+ assertLocate("̇", "İ", 1, "UTF8_BINARY_LCASE", 0); // note: different from UTF8_BINARY
+ assertLocate("i", "i̇", 1, "UNICODE_CI", 0);
+ assertLocate("i̇", "i", 1, "UNICODE_CI", 0);
+ assertLocate("İ", "i̇", 1, "UNICODE_CI", 1);
+ assertLocate("İ", "i", 1, "UNICODE_CI", 0);
+ assertLocate("i", "i̇", 1, "UTF8_BINARY_LCASE", 1); // note: different from UNICODE_CI
+ assertLocate("i̇", "i", 1, "UTF8_BINARY_LCASE", 0);
+ assertLocate("İ", "i̇", 1, "UTF8_BINARY_LCASE", 0); // note: different from UNICODE_CI
Review Comment:
The two strings are lowercase-equal. The result should be 1.
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -279,20 +303,36 @@ public void testEndsWith() throws SparkException {
assertEndsWith("ab世De", "AB世dE", "UNICODE_CI", true);
assertEndsWith("äbćδe", "ÄbćδE", "UNICODE_CI", true);
assertEndsWith("äbćδe", "ÄBcΔÉ", "UNICODE_CI", false);
- // Case-variable character length
- assertEndsWith("The İo", "i̇o", "UNICODE_CI", true);
- assertEndsWith("The i̇o", "İo", "UNICODE_CI", true);
- assertEndsWith("the İodine", "the i̇odine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("the i̇odine", "the İodine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("The İodiNe", "i̇oDine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("The İodiNe", "i̇oDin", "UTF8_BINARY_LCASE", false);
// Characters with the same binary lowercase representation
assertEndsWith("The Kelvin", "Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The Kelvin", "Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The KKelvin", "KKelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The 2 Kelvin", "2 Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The 2 Kelvin", "2 Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The KKelvin", "KKelvin,", "UTF8_BINARY_LCASE", false);
+ // Case-variable character length
+ assertEndsWith("i̇", "i", "UNICODE_CI", false);
+ assertEndsWith("i̇", "İ", "UNICODE_CI", true);
+ assertEndsWith("İ", "i", "UNICODE_CI", false);
+ assertEndsWith("the i̇o", "io", "UNICODE_CI", false);
+ assertEndsWith("the i̇o", "Io", "UNICODE_CI", false);
+ assertEndsWith("the i̇o", "i̇o", "UNICODE_CI", true);
+ assertEndsWith("the i̇o", "İo", "UNICODE_CI", true);
+ assertEndsWith("the İo", "io", "UNICODE_CI", false);
+ assertEndsWith("the İo", "Io", "UNICODE_CI", false);
+ assertEndsWith("the İo", "i̇o", "UNICODE_CI", true);
+ assertEndsWith("the İo", "İo", "UNICODE_CI", true);
+ assertEndsWith("i̇", "i", "UTF8_BINARY_LCASE", false);
+ assertEndsWith("i̇", "İ", "UTF8_BINARY_LCASE", false); // note: different from UNICODE_CI
+ assertEndsWith("İ", "i", "UTF8_BINARY_LCASE", false);
+ assertEndsWith("the i̇o", "io", "UTF8_BINARY_LCASE", false);
+ assertEndsWith("the i̇o", "Io", "UTF8_BINARY_LCASE", false);
+ assertEndsWith("the i̇o", "i̇o", "UTF8_BINARY_LCASE", true);
+ assertEndsWith("the i̇o", "İo", "UTF8_BINARY_LCASE", false); // note: different from UNICODE_CI
+ assertEndsWith("the İo", "io", "UTF8_BINARY_LCASE", false);
+ assertEndsWith("the İo", "Io", "UTF8_BINARY_LCASE", false);
+ assertEndsWith("the İo", "i̇o", "UTF8_BINARY_LCASE", false); // note: different from UNICODE_CI
Review Comment:
Should be true.
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java:
##########
@@ -215,43 +215,6 @@ public void contains() {
assertFalse(fromString("大千世界").contains(fromString("大千世界好")));
}
- @Test
- public void containsInLowerCase() {
Review Comment:
Here and below, why are we removing these test cases?
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,15 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ public static boolean lowercaseMatchAt(final UTF8String l, final UTF8String r,
+ int pos, int len) {
+ if (len + pos > l.numChars() || pos < 0) {
+ return false;
+ }
+ return l.substring(pos, pos + len).toLowerCase().equals(r);
Review Comment:
Another problem with this is that we are trying to match a known number of characters `len`, which you want it to be `r.numChars()`. I don't think that the overall logic here works.
I believe what you want is something like (this the concept, I am sure it can be optimized):
```java
public static boolean lowercaseMatchAt(final UTF8String l, final UTF8String r, int pos) {
assert(pos > 0);
UTF8String lsubstr = l.substring(pos);
UTF8String r1 = r.toLowerCase();
for (int i = 1; i <= lsubstr.numChars(); ++i) {
if (lsubstr.substring(0, i).toLowerCase().equals(r1)) return true;
}
return false;
}
```
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -279,20 +303,36 @@ public void testEndsWith() throws SparkException {
assertEndsWith("ab世De", "AB世dE", "UNICODE_CI", true);
assertEndsWith("äbćδe", "ÄbćδE", "UNICODE_CI", true);
assertEndsWith("äbćδe", "ÄBcΔÉ", "UNICODE_CI", false);
- // Case-variable character length
- assertEndsWith("The İo", "i̇o", "UNICODE_CI", true);
- assertEndsWith("The i̇o", "İo", "UNICODE_CI", true);
- assertEndsWith("the İodine", "the i̇odine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("the i̇odine", "the İodine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("The İodiNe", "i̇oDine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("The İodiNe", "i̇oDin", "UTF8_BINARY_LCASE", false);
// Characters with the same binary lowercase representation
assertEndsWith("The Kelvin", "Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The Kelvin", "Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The KKelvin", "KKelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The 2 Kelvin", "2 Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The 2 Kelvin", "2 Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The KKelvin", "KKelvin,", "UTF8_BINARY_LCASE", false);
+ // Case-variable character length
+ assertEndsWith("i̇", "i", "UNICODE_CI", false);
+ assertEndsWith("i̇", "İ", "UNICODE_CI", true);
+ assertEndsWith("İ", "i", "UNICODE_CI", false);
+ assertEndsWith("the i̇o", "io", "UNICODE_CI", false);
+ assertEndsWith("the i̇o", "Io", "UNICODE_CI", false);
+ assertEndsWith("the i̇o", "i̇o", "UNICODE_CI", true);
+ assertEndsWith("the i̇o", "İo", "UNICODE_CI", true);
+ assertEndsWith("the İo", "io", "UNICODE_CI", false);
+ assertEndsWith("the İo", "Io", "UNICODE_CI", false);
+ assertEndsWith("the İo", "i̇o", "UNICODE_CI", true);
+ assertEndsWith("the İo", "İo", "UNICODE_CI", true);
+ assertEndsWith("i̇", "i", "UTF8_BINARY_LCASE", false);
+ assertEndsWith("i̇", "İ", "UTF8_BINARY_LCASE", false); // note: different from UNICODE_CI
Review Comment:
This should be true. The two strings are lowercase-equal so both startswith and endswith should return true.
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1600495650
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -183,6 +204,19 @@ public static int findInSet(final UTF8String match, final UTF8String set, int co
return 0;
}
+ public static int lowercaseIndexOf(final UTF8String target, final UTF8String pattern,
+ final int start) {
+ if (pattern.numChars() == 0) return 0;
Review Comment:
why do you think it would not be expected? `indexOf` uses 0-based index
- -1 means "not found"
- 0 means "found at beginning"
an empty pattern string is (by definition) found at the beginning of any given target string
I don't think there are any issues there - seems pretty expected to me
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation [spark]
Posted by "mkaravel (via GitHub)" <gi...@apache.org>.
mkaravel commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1599455730
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,27 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ public static boolean lowercaseMatchFrom(final UTF8String l, final UTF8String r, int pos) {
+ if (pos < 0) return false;
+ for (int len = 0; len <= l.numChars() - pos; len++) {
Review Comment:
```suggestion
for (int len = 0; len <= l.numChars() - pos; ++len) {
```
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,27 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ public static boolean lowercaseMatchFrom(final UTF8String l, final UTF8String r, int pos) {
+ if (pos < 0) return false;
Review Comment:
Why do we return false in this case?
Also why not make this an assert and make sure we never call this method with a negative `pos`?
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,27 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ public static boolean lowercaseMatchFrom(final UTF8String l, final UTF8String r, int pos) {
+ if (pos < 0) return false;
+ for (int len = 0; len <= l.numChars() - pos; len++) {
+ if (l.substring(pos, pos + len).toLowerCase().equals(r)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ public static boolean lowercaseMatchUntil(final UTF8String l, final UTF8String r, int pos) {
+ if (pos > l.numChars()) return false;
Review Comment:
Same comment regarding the bounds for `pos`.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -183,6 +204,19 @@ public static int findInSet(final UTF8String match, final UTF8String set, int co
return 0;
}
+ public static int lowercaseIndexOf(final UTF8String target, final UTF8String pattern,
+ final int start) {
+ if (pattern.numChars() == 0) return 0;
Review Comment:
I would expect `start` to be returned here, not `0`.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,27 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ public static boolean lowercaseMatchFrom(final UTF8String l, final UTF8String r, int pos) {
+ if (pos < 0) return false;
+ for (int len = 0; len <= l.numChars() - pos; len++) {
+ if (l.substring(pos, pos + len).toLowerCase().equals(r)) {
Review Comment:
```suggestion
if (l.substring(pos, pos + len).toLowerCase().equals(r.toLowerCase())) {
```
Somehow we need to state that `r` is already in lowercase (comment and/or change the name of the variable).
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -183,6 +204,19 @@ public static int findInSet(final UTF8String match, final UTF8String set, int co
return 0;
}
+ public static int lowercaseIndexOf(final UTF8String target, final UTF8String pattern,
+ final int start) {
+ if (pattern.numChars() == 0) return 0;
+ int lenHaystack = target.numChars(), lenNeedle = pattern.numChars();
+ final UTF8String needle = pattern.toLowerCase();
+ for (int i = start; i <= (lenHaystack - lenNeedle); i++) {
Review Comment:
```suggestion
for (int i = start; i <= (lenHaystack - lenNeedle); ++i) {
```
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -118,7 +118,8 @@ public static boolean execBinary(final UTF8String l, final UTF8String r) {
return l.contains(r);
}
public static boolean execLowercase(final UTF8String l, final UTF8String r) {
- return l.containsInLowerCase(r);
+ if (r.numBytes() == 0) return true;
Review Comment:
Do you need this? There is special case inside `lowercaseIndexOf` that handles it.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -156,7 +157,8 @@ public static boolean execBinary(final UTF8String l, final UTF8String r) {
return l.startsWith(r);
}
public static boolean execLowercase(final UTF8String l, final UTF8String r) {
- return l.startsWithInLowerCase(r);
+ if (r.numBytes() == 0) return true;
Review Comment:
Same here.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,27 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ public static boolean lowercaseMatchFrom(final UTF8String l, final UTF8String r, int pos) {
+ if (pos < 0) return false;
+ for (int len = 0; len <= l.numChars() - pos; len++) {
+ if (l.substring(pos, pos + len).toLowerCase().equals(r)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ public static boolean lowercaseMatchUntil(final UTF8String l, final UTF8String r, int pos) {
+ if (pos > l.numChars()) return false;
+ for (int len = 1; len <= pos; len++) {
Review Comment:
```suggestion
for (int len = 1; len <= pos; ++len) {
```
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -183,6 +204,19 @@ public static int findInSet(final UTF8String match, final UTF8String set, int co
return 0;
}
+ public static int lowercaseIndexOf(final UTF8String target, final UTF8String pattern,
Review Comment:
Please add documentation comments to all the functions, specifically regarding the indices (1-based) and the return values (here for example we want to say that in case we don't have a match we return `-1`).
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -193,7 +195,8 @@ public static boolean execBinary(final UTF8String l, final UTF8String r) {
return l.endsWith(r);
}
public static boolean execLowercase(final UTF8String l, final UTF8String r) {
- return l.endsWithInLowerCase(r);
+ if (r.numBytes() == 0) return true;
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-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-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1609894240
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,155 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found when searching for a pattern
+ * string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix, starting from the
+ * specified position (0-based index referring to character position in UTF8String), with respect
+ * to the UTF8_BINARY_LCASE collation. The method assumes that the prefix is already lowercased
+ * prior to method call to avoid the overhead of calling .toLowerCase() multiple times on the
+ * same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with the specified
+ * prefix, starting from the specified position (0-based index referring to character position
+ * in UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * prefix is already lowercased. The method only considers the part of target string that
+ * starts from the specified (inclusive) position (that is, the method does not look at UTF8
+ * characters of the target string at or after position `endPos`). If the prefix is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return length of the target substring that ends with the specified prefix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the first occurrence of the pattern string in the target string,
+ * starting from the specified position (0-based index referring to character position in
+ * UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * pattern string is already lowercased prior to call. If the pattern is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return the position of the first occurrence of pattern in target
+ */
+ public static int lowercaseFind(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int i = startPos; i <= target.numChars(); ++i) {
+ if (lowercaseMatchFrom(target, lowercasePattern, i)) {
+ return i;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns whether the target string ends with the specified suffix, ending at the specified
+ * position (0-based index referring to character position in UTF8String), with respect to the
+ * UTF8_BINARY_LCASE collation. The method assumes that the suffix is already lowercased prior
+ * to method call to avoid the overhead of calling .toLowerCase() multiple times on the same
+ * suffix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return whether the target string ends with the specified suffix in lowercase
+ */
+ public static boolean lowercaseMatchUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ return lowercaseMatchLengthUntil(target, lowercasePattern, endPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that ends with the specified
+ * suffix, ending at the specified position (0-based index referring to character position in
+ * UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * suffix is already lowercased. The method only considers the part of target string that ends
+ * at the specified (non-inclusive) position (that is, the method does not look at UTF8
+ * characters of the target string at or after position `endPos`). If the suffix is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ assert endPos <= target.numChars();
+ for (int len = 0; len <= endPos; ++len) {
+ if (target.substring(endPos - len, endPos).toLowerCase().equals(lowercasePattern)) {
Review Comment:
> Is it the same if we lower case the entire target string first
no, it's not - consider the example in the PR description: `contains("İ", "i")` where `haystack = "İ"` & `needle = "i"`
`lower(haystack) = lower("İ") = "i\u0307"` would produce `substr(lower("İ"), 1, 1) = "i" = needle`
however, we can see that these is no `substr(haystack, start, len)` such that `lower(substr(haystack, start, len)) == needle` - so these 2 behaviours are not actually equivalent (as we first assumed), and the reason for this discrepancy lies in conditional one-to-many case mapping (certain characters have lowercase equivalents consisted of multiple characters)
--
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-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1612011134
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -430,7 +430,7 @@ public static int execBinary(final UTF8String string, final UTF8String substring
}
public static int execLowercase(final UTF8String string, final UTF8String substring,
final int start) {
- return string.toLowerCase().indexOf(substring.toLowerCase(), start);
Review Comment:
so as part of this PR, we actually changed the core definition of string-searching in UTF8_BINARY_LCASE, i.e. what it means for one substring (_pattern_) to be found in another string (_target_) under UTF8_BINARY_LCASE
in the old implementation, `contains("İ", "i")` would return `true` - however, this behaviour is incorrect because it relies on the fact that `substr(lower("İ"))` == `"i"` (incorrect, old implementation), instead of `lower(substr("İ"))` == `"i"` (correct, new implementation)
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "mkaravel (via GitHub)" <gi...@apache.org>.
mkaravel commented on PR #46511:
URL: https://github.com/apache/spark/pull/46511#issuecomment-2121384473
Please fill in the PR description.
--
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-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1612011134
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -430,7 +430,7 @@ public static int execBinary(final UTF8String string, final UTF8String substring
}
public static int execLowercase(final UTF8String string, final UTF8String substring,
final int start) {
- return string.toLowerCase().indexOf(substring.toLowerCase(), start);
Review Comment:
so as part of this PR, we actually changed the core definition of string-searching in UTF8_BINARY_LCASE, i.e. what it means for one substring (_pattern_) to be found in another string (_target_) under UTF8_BINARY_LCASE
in the old implementation, `contains("İ", "i")` would return `true` - however, this behaviour is incorrect because it relies on the fact that `substr(lower("İ"), 1, 1)` == `"i"` (incorrect, old implementation), instead of `lower(substr("İ", 1, 1))` != `"i"` (correct, new implementation)
--
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-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1612011134
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -430,7 +430,7 @@ public static int execBinary(final UTF8String string, final UTF8String substring
}
public static int execLowercase(final UTF8String string, final UTF8String substring,
final int start) {
- return string.toLowerCase().indexOf(substring.toLowerCase(), start);
Review Comment:
so as part of this PR, we actually changed the core definition of string-searching in UTF8_BINARY_LCASE, i.e. what it means for one substring (_pattern_) to be found in another string (_target_) under UTF8_BINARY_LCASE
in the old implementation, `contains("İ", "i")` would return `true` - however, this behaviour is incorrect because it relies on the fact that `substr(lower("İ"), 1, 1)` == `"i"` (incorrect, old implementation), instead of `lower(substr("İ"substr))` == `"i"` (correct, new implementation)
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1596342527
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,15 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ public static boolean lowercaseMatchAt(final UTF8String l, final UTF8String r,
+ int pos, int len) {
Review Comment:
no, `len` is used for `l` and we don't know in advance whether `len` will be equal to `r.numChars()` (it actually won't be in these special cases)
however, this is irrelevant because I need to change the approach overall, so `len` will likely be removed from this API either way
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1596326073
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,15 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ public static boolean lowercaseMatchAt(final UTF8String l, final UTF8String r,
+ int pos, int len) {
+ if (len + pos > l.numChars() || pos < 0) {
+ return false;
+ }
Review Comment:
after all, this method is part of a public interface (CollationAwareUTF8String.java) - so I think this check is in order (we don't know who or how may use this method exactly, although we currently only pass positive `pos` values to it)
--
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-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1609894240
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,155 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found when searching for a pattern
+ * string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix, starting from the
+ * specified position (0-based index referring to character position in UTF8String), with respect
+ * to the UTF8_BINARY_LCASE collation. The method assumes that the prefix is already lowercased
+ * prior to method call to avoid the overhead of calling .toLowerCase() multiple times on the
+ * same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with the specified
+ * prefix, starting from the specified position (0-based index referring to character position
+ * in UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * prefix is already lowercased. The method only considers the part of target string that
+ * starts from the specified (inclusive) position (that is, the method does not look at UTF8
+ * characters of the target string at or after position `endPos`). If the prefix is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return length of the target substring that ends with the specified prefix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the first occurrence of the pattern string in the target string,
+ * starting from the specified position (0-based index referring to character position in
+ * UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * pattern string is already lowercased prior to call. If the pattern is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return the position of the first occurrence of pattern in target
+ */
+ public static int lowercaseFind(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int i = startPos; i <= target.numChars(); ++i) {
+ if (lowercaseMatchFrom(target, lowercasePattern, i)) {
+ return i;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns whether the target string ends with the specified suffix, ending at the specified
+ * position (0-based index referring to character position in UTF8String), with respect to the
+ * UTF8_BINARY_LCASE collation. The method assumes that the suffix is already lowercased prior
+ * to method call to avoid the overhead of calling .toLowerCase() multiple times on the same
+ * suffix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return whether the target string ends with the specified suffix in lowercase
+ */
+ public static boolean lowercaseMatchUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ return lowercaseMatchLengthUntil(target, lowercasePattern, endPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that ends with the specified
+ * suffix, ending at the specified position (0-based index referring to character position in
+ * UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * suffix is already lowercased. The method only considers the part of target string that ends
+ * at the specified (non-inclusive) position (that is, the method does not look at UTF8
+ * characters of the target string at or after position `endPos`). If the suffix is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ assert endPos <= target.numChars();
+ for (int len = 0; len <= endPos; ++len) {
+ if (target.substring(endPos - len, endPos).toLowerCase().equals(lowercasePattern)) {
Review Comment:
> Is it the same if we lower case the entire target string first
no, it's not - consider the example in the PR description: `contains("İ", "i")` where `haystack = "İ"` & `needle = "i"`; then we have: `lower(haystack) = lower("İ") = "i\u0307"` would produce `substr(lower("İ"), 1, 1) = "i" = needle`
however, we can see that these is no `substr(haystack, start, len)` such that `lower(substr(haystack, start, len)) == needle` - so these 2 behaviours are not actually equivalent (as we first assumed), and the reason for this discrepancy lies in conditional one-to-many case mapping (certain characters have lowercase equivalents consisted of multiple characters)
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1596734094
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -118,7 +118,9 @@ public static boolean execBinary(final UTF8String l, final UTF8String r) {
return l.contains(r);
}
public static boolean execLowercase(final UTF8String l, final UTF8String r) {
- return l.containsInLowerCase(r);
+ if (r.numChars() == 0) return true;
+ if (l.numChars() < r.numChars()) return false;
Review Comment:
update: resolved in latest commit
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,15 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ public static boolean lowercaseMatchAt(final UTF8String l, final UTF8String r,
+ int pos, int len) {
Review Comment:
update: removed in latest commit
--
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-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1609842094
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,155 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found when searching for a pattern
+ * string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix, starting from the
+ * specified position (0-based index referring to character position in UTF8String), with respect
+ * to the UTF8_BINARY_LCASE collation. The method assumes that the prefix is already lowercased
+ * prior to method call to avoid the overhead of calling .toLowerCase() multiple times on the
+ * same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with the specified
+ * prefix, starting from the specified position (0-based index referring to character position
+ * in UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * prefix is already lowercased. The method only considers the part of target string that
+ * starts from the specified (inclusive) position (that is, the method does not look at UTF8
+ * characters of the target string at or after position `endPos`). If the prefix is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return length of the target substring that ends with the specified prefix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the first occurrence of the pattern string in the target string,
+ * starting from the specified position (0-based index referring to character position in
+ * UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * pattern string is already lowercased prior to call. If the pattern is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return the position of the first occurrence of pattern in target
+ */
+ public static int lowercaseFind(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int i = startPos; i <= target.numChars(); ++i) {
+ if (lowercaseMatchFrom(target, lowercasePattern, i)) {
+ return i;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns whether the target string ends with the specified suffix, ending at the specified
+ * position (0-based index referring to character position in UTF8String), with respect to the
+ * UTF8_BINARY_LCASE collation. The method assumes that the suffix is already lowercased prior
+ * to method call to avoid the overhead of calling .toLowerCase() multiple times on the same
+ * suffix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return whether the target string ends with the specified suffix in lowercase
+ */
+ public static boolean lowercaseMatchUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ return lowercaseMatchLengthUntil(target, lowercasePattern, endPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that ends with the specified
+ * suffix, ending at the specified position (0-based index referring to character position in
+ * UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * suffix is already lowercased. The method only considers the part of target string that ends
+ * at the specified (non-inclusive) position (that is, the method does not look at UTF8
+ * characters of the target string at or after position `endPos`). If the suffix is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ assert endPos <= target.numChars();
+ for (int len = 0; len <= endPos; ++len) {
+ if (target.substring(endPos - len, endPos).toLowerCase().equals(lowercasePattern)) {
Review Comment:
is there a way to do this comparison with only one memory copy? Right now it's two: `substring` and `toLowerCase`
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "mkaravel (via GitHub)" <gi...@apache.org>.
mkaravel commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1603739103
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with
+ * the specified prefix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the prefix is already lowercased. The method only
+ * considers the part of target string that starts from the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
Review Comment:
```suggestion
if (target.substring(startPos, startPos + len, /*IsEphemeral=*/true).toLowerCase().equals(lowercasePattern)) {
```
There is no need to make a copy here.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with
+ * the specified prefix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the prefix is already lowercased. The method only
+ * considers the part of target string that starts from the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the first occurrence of the pattern string
+ * in the target string from the specified position (0-based index),
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the pattern string is already lowercased prior to method call.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return the position of the first occurrence of pattern in target, if not found, -1 returned.
+ */
+ public static int lowercaseFind(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ for (int i = startPos; i <= target.numChars(); ++i) {
+ if (lowercaseMatchFrom(target, lowercasePattern, i)) {
+ return i;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns whether the target string ends with the specified suffix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the suffix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same suffix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return whether the target string ends with the specified suffix in lowercase
+ */
+ public static boolean lowercaseMatchUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ return lowercaseMatchLengthUntil(target, lowercasePattern, endPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that ends with
+ * the specified suffix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the suffix is already lowercased. The method only
+ * considers the part of target string that ends at the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ assert endPos <= target.numChars();
+ for (int len = 0; len <= endPos; ++len) {
+ if (target.substring(endPos - len, endPos).toLowerCase().equals(lowercasePattern)) {
Review Comment:
```suggestion
if (target.substring(endPos - len, endPos, /*isEphemeral=*/true).toLowerCase().equals(lowercasePattern)) {
```
There is no need to make a copy.
--
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-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #46511:
URL: https://github.com/apache/spark/pull/46511#issuecomment-2135733655
thanks, merging to master!
--
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-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1609839724
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -430,7 +430,7 @@ public static int execBinary(final UTF8String string, final UTF8String substring
}
public static int execLowercase(final UTF8String string, final UTF8String substring,
final int start) {
- return string.toLowerCase().indexOf(substring.toLowerCase(), start);
Review Comment:
to confirm, the previous implementation here is correct, right?
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "mkaravel (via GitHub)" <gi...@apache.org>.
mkaravel commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1603716669
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
Review Comment:
Let's specify that this is 0-based.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with
+ * the specified prefix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the prefix is already lowercased. The method only
+ * considers the part of target string that starts from the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the first occurrence of the pattern string
+ * in the target string from the specified position (0-based index),
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the pattern string is already lowercased prior to method call.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return the position of the first occurrence of pattern in target, if not found, -1 returned.
Review Comment:
```suggestion
* @return the position of the first occurrence of pattern in target, if not found, `MATCH_NOT_FOUND` returned.
```
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with
+ * the specified prefix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the prefix is already lowercased. The method only
+ * considers the part of target string that starts from the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the first occurrence of the pattern string
+ * in the target string from the specified position (0-based index),
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the pattern string is already lowercased prior to method call.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return the position of the first occurrence of pattern in target, if not found, -1 returned.
+ */
+ public static int lowercaseFind(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ for (int i = startPos; i <= target.numChars(); ++i) {
+ if (lowercaseMatchFrom(target, lowercasePattern, i)) {
+ return i;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns whether the target string ends with the specified suffix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the suffix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same suffix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return whether the target string ends with the specified suffix in lowercase
+ */
+ public static boolean lowercaseMatchUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ return lowercaseMatchLengthUntil(target, lowercasePattern, endPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that ends with
+ * the specified suffix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the suffix is already lowercased. The method only
+ * considers the part of target string that ends at the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ assert endPos <= target.numChars();
+ for (int len = 0; len <= endPos; ++len) {
+ if (target.substring(endPos - len, endPos).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
Review Comment:
Same thing here for creating a JIRA for using the pattern-length heuristic.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with
+ * the specified prefix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the prefix is already lowercased. The method only
+ * considers the part of target string that starts from the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the end position for searching (in the target string)
Review Comment:
You probably mean end position here. Also let's specify that `startPos` is 0-based.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with
+ * the specified prefix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the prefix is already lowercased. The method only
+ * considers the part of target string that starts from the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the end position for searching (in the target string)
Review Comment:
Also that `startPos` refers to UTF8 characters not bytes.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with
+ * the specified prefix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the prefix is already lowercased. The method only
+ * considers the part of target string that starts from the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the first occurrence of the pattern string
+ * in the target string from the specified position (0-based index),
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the pattern string is already lowercased prior to method call.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return the position of the first occurrence of pattern in target, if not found, -1 returned.
+ */
+ public static int lowercaseFind(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ for (int i = startPos; i <= target.numChars(); ++i) {
+ if (lowercaseMatchFrom(target, lowercasePattern, i)) {
+ return i;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns whether the target string ends with the specified suffix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the suffix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same suffix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
Review Comment:
We also need to explain/specify that `endPos` refers to UTF8 characters.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with
+ * the specified prefix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the prefix is already lowercased. The method only
+ * considers the part of target string that starts from the specified position.
Review Comment:
Let's also say that if a match is not found, then `MATCH_NOT_FOUND` is returned.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
Review Comment:
nit: Please try to use the full extent of the 100 characters per line for the comments.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
Review Comment:
Shouldn't we mention that we are interested in a match after `startPos`?
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with
+ * the specified prefix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the prefix is already lowercased. The method only
+ * considers the part of target string that starts from the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the first occurrence of the pattern string
+ * in the target string from the specified position (0-based index),
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the pattern string is already lowercased prior to method call.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return the position of the first occurrence of pattern in target, if not found, -1 returned.
+ */
+ public static int lowercaseFind(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ for (int i = startPos; i <= target.numChars(); ++i) {
+ if (lowercaseMatchFrom(target, lowercasePattern, i)) {
+ return i;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns whether the target string ends with the specified suffix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the suffix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same suffix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
Review Comment:
Let's specify that this is 0-based.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with
+ * the specified prefix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the prefix is already lowercased. The method only
+ * considers the part of target string that starts from the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the first occurrence of the pattern string
+ * in the target string from the specified position (0-based index),
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the pattern string is already lowercased prior to method call.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return the position of the first occurrence of pattern in target, if not found, -1 returned.
+ */
+ public static int lowercaseFind(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ for (int i = startPos; i <= target.numChars(); ++i) {
Review Comment:
Assert that startPos is non-negative?
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with
+ * the specified prefix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the prefix is already lowercased. The method only
+ * considers the part of target string that starts from the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the first occurrence of the pattern string
+ * in the target string from the specified position (0-based index),
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the pattern string is already lowercased prior to method call.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return the position of the first occurrence of pattern in target, if not found, -1 returned.
+ */
+ public static int lowercaseFind(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ for (int i = startPos; i <= target.numChars(); ++i) {
+ if (lowercaseMatchFrom(target, lowercasePattern, i)) {
+ return i;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns whether the target string ends with the specified suffix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the suffix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same suffix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return whether the target string ends with the specified suffix in lowercase
+ */
+ public static boolean lowercaseMatchUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ return lowercaseMatchLengthUntil(target, lowercasePattern, endPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that ends with
+ * the specified suffix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the suffix is already lowercased. The method only
+ * considers the part of target string that ends at the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
Review Comment:
```suggestion
* @param endPos the 0-based end position for searching (in the target string)
```
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with
+ * the specified prefix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the prefix is already lowercased. The method only
+ * considers the part of target string that starts from the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
Review Comment:
Could we please create a ticket to re-implement this using the heuristic of starting with a length that is equal to the pattern, which also means that we probably need to pass this length as an argument since the length of the pattern and the length of the lowercase pattern may be different. Actually, even using the length of the lowercase pattern should be okay since this is meant to act as a starting point for the search.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with
+ * the specified prefix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the prefix is already lowercased. The method only
+ * considers the part of target string that starts from the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the first occurrence of the pattern string
+ * in the target string from the specified position (0-based index),
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the pattern string is already lowercased prior to method call.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return the position of the first occurrence of pattern in target, if not found, -1 returned.
+ */
+ public static int lowercaseFind(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ for (int i = startPos; i <= target.numChars(); ++i) {
+ if (lowercaseMatchFrom(target, lowercasePattern, i)) {
+ return i;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns whether the target string ends with the specified suffix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the suffix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same suffix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return whether the target string ends with the specified suffix in lowercase
+ */
+ public static boolean lowercaseMatchUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ return lowercaseMatchLengthUntil(target, lowercasePattern, endPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that ends with
+ * the specified suffix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the suffix is already lowercased. The method only
+ * considers the part of target string that ends at the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ assert endPos <= target.numChars();
+ for (int len = 0; len <= endPos; ++len) {
+ if (target.substring(endPos - len, endPos).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the last occurrence of the pattern string
+ * in the target string until the specified position (0-based index),
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the pattern string is already lowercased prior to method call.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return the position of the last occurrence of pattern in target, if not found, -1 returned.
Review Comment:
```suggestion
* @return the position of the last occurrence of pattern in target, if not found, `MATCH_NOT_FOUND` returned.
```
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with
+ * the specified prefix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the prefix is already lowercased. The method only
+ * considers the part of target string that starts from the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the first occurrence of the pattern string
+ * in the target string from the specified position (0-based index),
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the pattern string is already lowercased prior to method call.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return the position of the first occurrence of pattern in target, if not found, -1 returned.
+ */
+ public static int lowercaseFind(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ for (int i = startPos; i <= target.numChars(); ++i) {
+ if (lowercaseMatchFrom(target, lowercasePattern, i)) {
+ return i;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns whether the target string ends with the specified suffix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the suffix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same suffix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return whether the target string ends with the specified suffix in lowercase
+ */
+ public static boolean lowercaseMatchUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ return lowercaseMatchLengthUntil(target, lowercasePattern, endPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that ends with
+ * the specified suffix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the suffix is already lowercased. The method only
+ * considers the part of target string that ends at the specified position.
Review Comment:
```suggestion
* The method assumes that the suffix is already lowercased. The method only
* considers the part of target string that ends at the specified (non-inclusive) position (that is the method does not look at UTF8 characters of the target string at or after position `endPos`).
```
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with
+ * the specified prefix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the prefix is already lowercased. The method only
+ * considers the part of target string that starts from the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the first occurrence of the pattern string
+ * in the target string from the specified position (0-based index),
Review Comment:
Specify that "position" refers to UTF8 characters not bytes?
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with
+ * the specified prefix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the prefix is already lowercased. The method only
+ * considers the part of target string that starts from the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the first occurrence of the pattern string
+ * in the target string from the specified position (0-based index),
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the pattern string is already lowercased prior to method call.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return the position of the first occurrence of pattern in target, if not found, -1 returned.
+ */
+ public static int lowercaseFind(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ for (int i = startPos; i <= target.numChars(); ++i) {
+ if (lowercaseMatchFrom(target, lowercasePattern, i)) {
+ return i;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns whether the target string ends with the specified suffix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the suffix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same suffix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return whether the target string ends with the specified suffix in lowercase
+ */
+ public static boolean lowercaseMatchUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ return lowercaseMatchLengthUntil(target, lowercasePattern, endPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that ends with
+ * the specified suffix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the suffix is already lowercased. The method only
+ * considers the part of target string that ends at the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ assert endPos <= target.numChars();
+ for (int len = 0; len <= endPos; ++len) {
+ if (target.substring(endPos - len, endPos).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the last occurrence of the pattern string
+ * in the target string until the specified position (0-based index),
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the pattern string is already lowercased prior to method call.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return the position of the last occurrence of pattern in target, if not found, -1 returned.
+ */
+ public static int lowercaseRFind(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ for (int i = endPos; i >= 0; --i) {
Review Comment:
Assert that endPos is non-negative?
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with
+ * the specified prefix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the prefix is already lowercased. The method only
+ * considers the part of target string that starts from the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the first occurrence of the pattern string
+ * in the target string from the specified position (0-based index),
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the pattern string is already lowercased prior to method call.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return the position of the first occurrence of pattern in target, if not found, -1 returned.
+ */
+ public static int lowercaseFind(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ for (int i = startPos; i <= target.numChars(); ++i) {
+ if (lowercaseMatchFrom(target, lowercasePattern, i)) {
+ return i;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns whether the target string ends with the specified suffix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the suffix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same suffix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return whether the target string ends with the specified suffix in lowercase
+ */
+ public static boolean lowercaseMatchUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ return lowercaseMatchLengthUntil(target, lowercasePattern, endPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that ends with
+ * the specified suffix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the suffix is already lowercased. The method only
+ * considers the part of target string that ends at the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ assert endPos <= target.numChars();
+ for (int len = 0; len <= endPos; ++len) {
+ if (target.substring(endPos - len, endPos).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the last occurrence of the pattern string
+ * in the target string until the specified position (0-based index),
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the pattern string is already lowercased prior to method call.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return the position of the last occurrence of pattern in target, if not found, -1 returned.
+ */
+ public static int lowercaseRFind(
Review Comment:
Maybe rename this to `lowercaseReverseFind`?
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -183,6 +320,22 @@ public static int findInSet(final UTF8String match, final UTF8String set, int co
return 0;
}
+ /**
+ * Returns the position of the first occurrence of the pattern string
+ * in the target string from the specified position (0-based index),
+ * with respect to the UTF8_BINARY_LCASE collation.
+ *
+ * @param target the string to be searched in
+ * @param pattern the string to be searched for
+ * @param start the start position for searching (in the target string)
+ * @return the position of the first occurrence of pattern in target, if not found, -1 returned.
Review Comment:
```suggestion
* @return the position of the first occurrence of pattern in target, if not found `MATCH_NOT_FOUND` is returned.
```
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -23,7 +23,7 @@
import static org.junit.jupiter.api.Assertions.*;
-
+// checkstyle.off: AvoidEscapedUnicodeCharacters
Review Comment:
Nice!
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -279,20 +320,44 @@ public void testEndsWith() throws SparkException {
assertEndsWith("ab世De", "AB世dE", "UNICODE_CI", true);
assertEndsWith("äbćδe", "ÄbćδE", "UNICODE_CI", true);
assertEndsWith("äbćδe", "ÄBcΔÉ", "UNICODE_CI", false);
- // Case-variable character length
- assertEndsWith("The İo", "i̇o", "UNICODE_CI", true);
- assertEndsWith("The i̇o", "İo", "UNICODE_CI", true);
- assertEndsWith("the İodine", "the i̇odine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("the i̇odine", "the İodine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("The İodiNe", "i̇oDine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("The İodiNe", "i̇oDin", "UTF8_BINARY_LCASE", false);
// Characters with the same binary lowercase representation
assertEndsWith("The Kelvin", "Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The Kelvin", "Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The KKelvin", "KKelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The 2 Kelvin", "2 Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The 2 Kelvin", "2 Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The KKelvin", "KKelvin,", "UTF8_BINARY_LCASE", false);
+ // Case-variable character length
+ assertEndsWith("i̇", "\u0307", "UNICODE_CI", false);
+ assertEndsWith("i̇", "İ", "UNICODE_CI", true);
+ assertEndsWith("İ", "i", "UNICODE_CI", false);
+ assertEndsWith("İİİ", "i̇i̇", "UNICODE_CI", true);
+ assertEndsWith("İİİ", "ii̇", "UNICODE_CI", false);
+ assertEndsWith("İi̇İ", "İi̇", "UNICODE_CI", true);
+ assertEndsWith("i̇İi̇i̇", "\u0307İi̇İ", "UNICODE_CI", false);
+ assertEndsWith("the i̇o", "io", "UNICODE_CI", false);
+ assertEndsWith("the i̇o", "Io", "UNICODE_CI", false);
+ assertEndsWith("the i̇o", "i̇o", "UNICODE_CI", true);
+ assertEndsWith("the i̇o", "İo", "UNICODE_CI", true);
+ assertEndsWith("the İo", "io", "UNICODE_CI", false);
+ assertEndsWith("the İo", "Io", "UNICODE_CI", false);
+ assertEndsWith("the İo", "i̇o", "UNICODE_CI", true);
+ assertEndsWith("the İo", "İo", "UNICODE_CI", true);
+ assertEndsWith("i̇", "\u0307", "UTF8_BINARY_LCASE", true); // != UNICODE_CI
Review Comment:
Can we also add?
```
assertEndsWith("İ", "\u0307", "UTF8_BINARY_LCASE", false);
```
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with
+ * the specified prefix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the prefix is already lowercased. The method only
+ * considers the part of target string that starts from the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
Review Comment:
```suggestion
if (target.substring(startPos, startPos + len, /*IsEphemeral=*/true).toLowerCase().equals(lowercasePattern)) {
```
There is no need to make a copy here.
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -279,20 +320,44 @@ public void testEndsWith() throws SparkException {
assertEndsWith("ab世De", "AB世dE", "UNICODE_CI", true);
assertEndsWith("äbćδe", "ÄbćδE", "UNICODE_CI", true);
assertEndsWith("äbćδe", "ÄBcΔÉ", "UNICODE_CI", false);
- // Case-variable character length
- assertEndsWith("The İo", "i̇o", "UNICODE_CI", true);
- assertEndsWith("The i̇o", "İo", "UNICODE_CI", true);
- assertEndsWith("the İodine", "the i̇odine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("the i̇odine", "the İodine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("The İodiNe", "i̇oDine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("The İodiNe", "i̇oDin", "UTF8_BINARY_LCASE", false);
// Characters with the same binary lowercase representation
assertEndsWith("The Kelvin", "Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The Kelvin", "Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The KKelvin", "KKelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The 2 Kelvin", "2 Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The 2 Kelvin", "2 Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The KKelvin", "KKelvin,", "UTF8_BINARY_LCASE", false);
+ // Case-variable character length
+ assertEndsWith("i̇", "\u0307", "UNICODE_CI", false);
+ assertEndsWith("i̇", "İ", "UNICODE_CI", true);
+ assertEndsWith("İ", "i", "UNICODE_CI", false);
+ assertEndsWith("İİİ", "i̇i̇", "UNICODE_CI", true);
+ assertEndsWith("İİİ", "ii̇", "UNICODE_CI", false);
+ assertEndsWith("İi̇İ", "İi̇", "UNICODE_CI", true);
+ assertEndsWith("i̇İi̇i̇", "\u0307İi̇İ", "UNICODE_CI", false);
+ assertEndsWith("the i̇o", "io", "UNICODE_CI", false);
+ assertEndsWith("the i̇o", "Io", "UNICODE_CI", false);
+ assertEndsWith("the i̇o", "i̇o", "UNICODE_CI", true);
+ assertEndsWith("the i̇o", "İo", "UNICODE_CI", true);
+ assertEndsWith("the İo", "io", "UNICODE_CI", false);
+ assertEndsWith("the İo", "Io", "UNICODE_CI", false);
+ assertEndsWith("the İo", "i̇o", "UNICODE_CI", true);
+ assertEndsWith("the İo", "İo", "UNICODE_CI", true);
+ assertEndsWith("i̇", "\u0307", "UTF8_BINARY_LCASE", true); // != UNICODE_CI
+ assertEndsWith("i̇", "İ", "UTF8_BINARY_LCASE", true);
+ assertEndsWith("İ", "i", "UTF8_BINARY_LCASE", false);
+ assertEndsWith("İİİ", "i̇i̇", "UTF8_BINARY_LCASE", true);
+ assertEndsWith("İİİ", "ii̇", "UTF8_BINARY_LCASE", false);
Review Comment:
Also add?
```
assertEndsWith("İİİ", "\u0307i̇", "UTF8_BINARY_LCASE", false);
```
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with
+ * the specified prefix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the prefix is already lowercased. The method only
+ * considers the part of target string that starts from the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the first occurrence of the pattern string
+ * in the target string from the specified position (0-based index),
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the pattern string is already lowercased prior to method call.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return the position of the first occurrence of pattern in target, if not found, -1 returned.
+ */
+ public static int lowercaseFind(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ for (int i = startPos; i <= target.numChars(); ++i) {
+ if (lowercaseMatchFrom(target, lowercasePattern, i)) {
+ return i;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns whether the target string ends with the specified suffix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the suffix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same suffix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return whether the target string ends with the specified suffix in lowercase
+ */
+ public static boolean lowercaseMatchUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ return lowercaseMatchLengthUntil(target, lowercasePattern, endPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that ends with
+ * the specified suffix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the suffix is already lowercased. The method only
+ * considers the part of target string that ends at the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ assert endPos <= target.numChars();
+ for (int len = 0; len <= endPos; ++len) {
+ if (target.substring(endPos - len, endPos).toLowerCase().equals(lowercasePattern)) {
Review Comment:
```suggestion
if (target.substring(endPos - len, endPos, /*isEphemeral=*/true).toLowerCase().equals(lowercasePattern)) {
```
There is no need to make a copy.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
Review Comment:
Same for other comments below.
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -709,12 +774,24 @@ public void testLocate() throws SparkException {
assertLocate("大千", "test大千世界大千世界", 9, "UNICODE_CI", 9);
assertLocate("大千", "大千世界大千世界", 1, "UNICODE_CI", 1);
// Case-variable character length
+ assertLocate("\u0307", "i̇", 1, "UTF8_BINARY", 2);
Review Comment:
```suggestion
assertLocate("\u0307", "i̇", 1, "UTF8_BINARY_LCASE", 2);
```
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
Review Comment:
We also need to specify that `startPos` refers to UTF8 characters not bytes.
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -709,12 +774,24 @@ public void testLocate() throws SparkException {
assertLocate("大千", "test大千世界大千世界", 9, "UNICODE_CI", 9);
assertLocate("大千", "大千世界大千世界", 1, "UNICODE_CI", 1);
// Case-variable character length
+ assertLocate("\u0307", "i̇", 1, "UTF8_BINARY", 2);
+ assertLocate("\u0307", "İ", 1, "UTF8_BINARY_LCASE", 0); // != UTF8_BINARY
Review Comment:
I think the comment is wrong here.
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -709,12 +774,24 @@ public void testLocate() throws SparkException {
assertLocate("大千", "test大千世界大千世界", 9, "UNICODE_CI", 9);
assertLocate("大千", "大千世界大千世界", 1, "UNICODE_CI", 1);
// Case-variable character length
+ assertLocate("\u0307", "i̇", 1, "UTF8_BINARY", 2);
Review Comment:
I am not sure what the intent of this test case is here. If it is for UTF8_BINARY_LCASE then we have it again below.
--
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-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1609843913
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,155 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found when searching for a pattern
+ * string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix, starting from the
+ * specified position (0-based index referring to character position in UTF8String), with respect
+ * to the UTF8_BINARY_LCASE collation. The method assumes that the prefix is already lowercased
+ * prior to method call to avoid the overhead of calling .toLowerCase() multiple times on the
+ * same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with the specified
+ * prefix, starting from the specified position (0-based index referring to character position
+ * in UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * prefix is already lowercased. The method only considers the part of target string that
+ * starts from the specified (inclusive) position (that is, the method does not look at UTF8
+ * characters of the target string at or after position `endPos`). If the prefix is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return length of the target substring that ends with the specified prefix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the first occurrence of the pattern string in the target string,
+ * starting from the specified position (0-based index referring to character position in
+ * UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * pattern string is already lowercased prior to call. If the pattern is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return the position of the first occurrence of pattern in target
+ */
+ public static int lowercaseFind(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int i = startPos; i <= target.numChars(); ++i) {
+ if (lowercaseMatchFrom(target, lowercasePattern, i)) {
+ return i;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns whether the target string ends with the specified suffix, ending at the specified
+ * position (0-based index referring to character position in UTF8String), with respect to the
+ * UTF8_BINARY_LCASE collation. The method assumes that the suffix is already lowercased prior
+ * to method call to avoid the overhead of calling .toLowerCase() multiple times on the same
+ * suffix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return whether the target string ends with the specified suffix in lowercase
+ */
+ public static boolean lowercaseMatchUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ return lowercaseMatchLengthUntil(target, lowercasePattern, endPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that ends with the specified
+ * suffix, ending at the specified position (0-based index referring to character position in
+ * UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * suffix is already lowercased. The method only considers the part of target string that ends
+ * at the specified (non-inclusive) position (that is, the method does not look at UTF8
+ * characters of the target string at or after position `endPos`). If the suffix is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ assert endPos <= target.numChars();
+ for (int len = 0; len <= endPos; ++len) {
+ if (target.substring(endPos - len, endPos).toLowerCase().equals(lowercasePattern)) {
Review Comment:
I don't know the details very well. Is it the same if we lower case the entire target string first, and then call `substring` repeatedly?
--
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-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #46511: [SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate)
URL: https://github.com/apache/spark/pull/46511
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation [spark]
Posted by "mkaravel (via GitHub)" <gi...@apache.org>.
mkaravel commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1599447728
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -102,20 +102,30 @@ public void testContains() throws SparkException {
assertContains("äbćδe", "ÄbćδE", "UNICODE_CI", true);
assertContains("äbćδe", "ÄBcΔÉ", "UNICODE_CI", false);
// Case-variable character length
- assertContains("abİo12", "i̇o", "UNICODE_CI", true);
- assertContains("abi̇o12", "İo", "UNICODE_CI", true);
- assertContains("the İodine", "the i̇odine", "UTF8_BINARY_LCASE", true);
- assertContains("the i̇odine", "the İodine", "UTF8_BINARY_LCASE", true);
- assertContains("The İodiNe", " i̇oDin", "UTF8_BINARY_LCASE", true);
- assertContains("İodiNe", "i̇oDine", "UTF8_BINARY_LCASE", true);
- assertContains("İodiNe", " i̇oDin", "UTF8_BINARY_LCASE", false);
- // Characters with the same binary lowercase representation
- assertContains("The Kelvin.", "Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("The Kelvin.", "Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("The KKelvin.", "KKelvin", "UTF8_BINARY_LCASE", true);
- assertContains("2 Kelvin.", "2 Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("2 Kelvin.", "2 Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("The KKelvin.", "KKelvin,", "UTF8_BINARY_LCASE", false);
+ assertContains("i̇", "i", "UNICODE_CI", false);
+ assertContains("i̇", "̇", "UNICODE_CI", false);
Review Comment:
```suggestion
assertContains("i̇", "\u0307", "UNICODE_CI", false);
```
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -279,20 +313,44 @@ public void testEndsWith() throws SparkException {
assertEndsWith("ab世De", "AB世dE", "UNICODE_CI", true);
assertEndsWith("äbćδe", "ÄbćδE", "UNICODE_CI", true);
assertEndsWith("äbćδe", "ÄBcΔÉ", "UNICODE_CI", false);
- // Case-variable character length
- assertEndsWith("The İo", "i̇o", "UNICODE_CI", true);
- assertEndsWith("The i̇o", "İo", "UNICODE_CI", true);
- assertEndsWith("the İodine", "the i̇odine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("the i̇odine", "the İodine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("The İodiNe", "i̇oDine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("The İodiNe", "i̇oDin", "UTF8_BINARY_LCASE", false);
// Characters with the same binary lowercase representation
assertEndsWith("The Kelvin", "Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The Kelvin", "Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The KKelvin", "KKelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The 2 Kelvin", "2 Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The 2 Kelvin", "2 Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The KKelvin", "KKelvin,", "UTF8_BINARY_LCASE", false);
+ // Case-variable character length
+ assertEndsWith("i̇", "̇", "UNICODE_CI", false);
+ assertEndsWith("i̇", "İ", "UNICODE_CI", true);
+ assertEndsWith("İ", "i", "UNICODE_CI", false);
+ assertEndsWith("İİİ", "i̇i̇", "UNICODE_CI", true);
+ assertEndsWith("İİİ", "ii̇", "UNICODE_CI", false);
+ assertEndsWith("İi̇İ", "İi̇", "UNICODE_CI", true);
+ assertEndsWith("i̇İi̇i̇", "̇İi̇İ", "UNICODE_CI", false);
Review Comment:
```suggestion
assertEndsWith("i̇İi̇i̇", "\u0307İi̇İ", "UNICODE_CI", false);
```
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -279,20 +313,44 @@ public void testEndsWith() throws SparkException {
assertEndsWith("ab世De", "AB世dE", "UNICODE_CI", true);
assertEndsWith("äbćδe", "ÄbćδE", "UNICODE_CI", true);
assertEndsWith("äbćδe", "ÄBcΔÉ", "UNICODE_CI", false);
- // Case-variable character length
- assertEndsWith("The İo", "i̇o", "UNICODE_CI", true);
- assertEndsWith("The i̇o", "İo", "UNICODE_CI", true);
- assertEndsWith("the İodine", "the i̇odine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("the i̇odine", "the İodine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("The İodiNe", "i̇oDine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("The İodiNe", "i̇oDin", "UTF8_BINARY_LCASE", false);
// Characters with the same binary lowercase representation
assertEndsWith("The Kelvin", "Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The Kelvin", "Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The KKelvin", "KKelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The 2 Kelvin", "2 Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The 2 Kelvin", "2 Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The KKelvin", "KKelvin,", "UTF8_BINARY_LCASE", false);
+ // Case-variable character length
+ assertEndsWith("i̇", "̇", "UNICODE_CI", false);
+ assertEndsWith("i̇", "İ", "UNICODE_CI", true);
+ assertEndsWith("İ", "i", "UNICODE_CI", false);
+ assertEndsWith("İİİ", "i̇i̇", "UNICODE_CI", true);
+ assertEndsWith("İİİ", "ii̇", "UNICODE_CI", false);
+ assertEndsWith("İi̇İ", "İi̇", "UNICODE_CI", true);
+ assertEndsWith("i̇İi̇i̇", "̇İi̇İ", "UNICODE_CI", false);
+ assertEndsWith("the i̇o", "io", "UNICODE_CI", false);
+ assertEndsWith("the i̇o", "Io", "UNICODE_CI", false);
+ assertEndsWith("the i̇o", "i̇o", "UNICODE_CI", true);
+ assertEndsWith("the i̇o", "İo", "UNICODE_CI", true);
+ assertEndsWith("the İo", "io", "UNICODE_CI", false);
+ assertEndsWith("the İo", "Io", "UNICODE_CI", false);
+ assertEndsWith("the İo", "i̇o", "UNICODE_CI", true);
+ assertEndsWith("the İo", "İo", "UNICODE_CI", true);
+ assertEndsWith("i̇", "̇", "UTF8_BINARY_LCASE", true); // different from UNICODE_CI
+ assertEndsWith("i̇", "İ", "UTF8_BINARY_LCASE", true);
+ assertEndsWith("İ", "i", "UTF8_BINARY_LCASE", false);
+ assertEndsWith("İİİ", "i̇i̇", "UTF8_BINARY_LCASE", true);
+ assertEndsWith("İİİ", "ii̇", "UTF8_BINARY_LCASE", false);
+ assertEndsWith("İi̇İ", "İi̇", "UTF8_BINARY_LCASE", true);
+ assertEndsWith("i̇İi̇i̇", "̇İi̇İ", "UTF8_BINARY_LCASE", true); // different from UNICODE_CI
Review Comment:
```suggestion
assertEndsWith("i̇İi̇i̇", "\u0307İi̇İ", "UTF8_BINARY_LCASE", true); // different from UNICODE_CI
```
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -709,12 +767,24 @@ public void testLocate() throws SparkException {
assertLocate("大千", "test大千世界大千世界", 9, "UNICODE_CI", 9);
assertLocate("大千", "大千世界大千世界", 1, "UNICODE_CI", 1);
// Case-variable character length
+ assertLocate("̇", "i̇", 1, "UTF8_BINARY", 2);
+ assertLocate("̇", "İ", 1, "UTF8_BINARY_LCASE", 0); // different from UTF8_BINARY
+ assertLocate("i", "i̇", 1, "UNICODE_CI", 0);
+ assertLocate("̇", "i̇", 1, "UNICODE_CI", 0);
+ assertLocate("i̇", "i", 1, "UNICODE_CI", 0);
+ assertLocate("İ", "i̇", 1, "UNICODE_CI", 1);
+ assertLocate("İ", "i", 1, "UNICODE_CI", 0);
+ assertLocate("i", "i̇", 1, "UTF8_BINARY_LCASE", 1); // different from UNICODE_CI
+ assertLocate("̇", "i̇", 1, "UTF8_BINARY_LCASE", 2); // different from UNICODE_CI
Review Comment:
```suggestion
assertLocate("\u0307", "i̇", 1, "UTF8_BINARY_LCASE", 2); // different from UNICODE_CI
```
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -709,12 +767,24 @@ public void testLocate() throws SparkException {
assertLocate("大千", "test大千世界大千世界", 9, "UNICODE_CI", 9);
assertLocate("大千", "大千世界大千世界", 1, "UNICODE_CI", 1);
// Case-variable character length
+ assertLocate("̇", "i̇", 1, "UTF8_BINARY", 2);
+ assertLocate("̇", "İ", 1, "UTF8_BINARY_LCASE", 0); // different from UTF8_BINARY
Review Comment:
```suggestion
assertLocate("\u0307", "İ", 1, "UTF8_BINARY_LCASE", 0); // different from UTF8_BINARY
```
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -102,20 +102,30 @@ public void testContains() throws SparkException {
assertContains("äbćδe", "ÄbćδE", "UNICODE_CI", true);
assertContains("äbćδe", "ÄBcΔÉ", "UNICODE_CI", false);
// Case-variable character length
- assertContains("abİo12", "i̇o", "UNICODE_CI", true);
- assertContains("abi̇o12", "İo", "UNICODE_CI", true);
- assertContains("the İodine", "the i̇odine", "UTF8_BINARY_LCASE", true);
- assertContains("the i̇odine", "the İodine", "UTF8_BINARY_LCASE", true);
- assertContains("The İodiNe", " i̇oDin", "UTF8_BINARY_LCASE", true);
- assertContains("İodiNe", "i̇oDine", "UTF8_BINARY_LCASE", true);
- assertContains("İodiNe", " i̇oDin", "UTF8_BINARY_LCASE", false);
- // Characters with the same binary lowercase representation
- assertContains("The Kelvin.", "Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("The Kelvin.", "Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("The KKelvin.", "KKelvin", "UTF8_BINARY_LCASE", true);
- assertContains("2 Kelvin.", "2 Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("2 Kelvin.", "2 Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("The KKelvin.", "KKelvin,", "UTF8_BINARY_LCASE", false);
+ assertContains("i̇", "i", "UNICODE_CI", false);
+ assertContains("i̇", "̇", "UNICODE_CI", false);
+ assertContains("i̇", "İ", "UNICODE_CI", true);
+ assertContains("İ", "i", "UNICODE_CI", false);
+ assertContains("adi̇os", "io", "UNICODE_CI", false);
+ assertContains("adi̇os", "Io", "UNICODE_CI", false);
+ assertContains("adi̇os", "i̇o", "UNICODE_CI", true);
+ assertContains("adi̇os", "İo", "UNICODE_CI", true);
+ assertContains("adİos", "io", "UNICODE_CI", false);
+ assertContains("adİos", "Io", "UNICODE_CI", false);
+ assertContains("adİos", "i̇o", "UNICODE_CI", true);
+ assertContains("adİos", "İo", "UNICODE_CI", true);
+ assertContains("i̇", "i", "UTF8_BINARY_LCASE", true); // different from UNICODE_CI
+ assertContains("i̇", "̇", "UTF8_BINARY_LCASE", true); // different from UNICODE_CI
Review Comment:
```suggestion
assertContains("i̇", "\u0307", "UTF8_BINARY_LCASE", true); // different from UNICODE_CI
```
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -279,20 +313,44 @@ public void testEndsWith() throws SparkException {
assertEndsWith("ab世De", "AB世dE", "UNICODE_CI", true);
assertEndsWith("äbćδe", "ÄbćδE", "UNICODE_CI", true);
assertEndsWith("äbćδe", "ÄBcΔÉ", "UNICODE_CI", false);
- // Case-variable character length
- assertEndsWith("The İo", "i̇o", "UNICODE_CI", true);
- assertEndsWith("The i̇o", "İo", "UNICODE_CI", true);
- assertEndsWith("the İodine", "the i̇odine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("the i̇odine", "the İodine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("The İodiNe", "i̇oDine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("The İodiNe", "i̇oDin", "UTF8_BINARY_LCASE", false);
// Characters with the same binary lowercase representation
assertEndsWith("The Kelvin", "Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The Kelvin", "Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The KKelvin", "KKelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The 2 Kelvin", "2 Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The 2 Kelvin", "2 Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The KKelvin", "KKelvin,", "UTF8_BINARY_LCASE", false);
+ // Case-variable character length
+ assertEndsWith("i̇", "̇", "UNICODE_CI", false);
Review Comment:
```suggestion
assertEndsWith("i̇", "\u0307", "UNICODE_CI", false);
```
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -279,20 +313,44 @@ public void testEndsWith() throws SparkException {
assertEndsWith("ab世De", "AB世dE", "UNICODE_CI", true);
assertEndsWith("äbćδe", "ÄbćδE", "UNICODE_CI", true);
assertEndsWith("äbćδe", "ÄBcΔÉ", "UNICODE_CI", false);
- // Case-variable character length
- assertEndsWith("The İo", "i̇o", "UNICODE_CI", true);
- assertEndsWith("The i̇o", "İo", "UNICODE_CI", true);
- assertEndsWith("the İodine", "the i̇odine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("the i̇odine", "the İodine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("The İodiNe", "i̇oDine", "UTF8_BINARY_LCASE", true);
- assertEndsWith("The İodiNe", "i̇oDin", "UTF8_BINARY_LCASE", false);
// Characters with the same binary lowercase representation
assertEndsWith("The Kelvin", "Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The Kelvin", "Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The KKelvin", "KKelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The 2 Kelvin", "2 Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The 2 Kelvin", "2 Kelvin", "UTF8_BINARY_LCASE", true);
assertEndsWith("The KKelvin", "KKelvin,", "UTF8_BINARY_LCASE", false);
+ // Case-variable character length
+ assertEndsWith("i̇", "̇", "UNICODE_CI", false);
+ assertEndsWith("i̇", "İ", "UNICODE_CI", true);
+ assertEndsWith("İ", "i", "UNICODE_CI", false);
+ assertEndsWith("İİİ", "i̇i̇", "UNICODE_CI", true);
+ assertEndsWith("İİİ", "ii̇", "UNICODE_CI", false);
+ assertEndsWith("İi̇İ", "İi̇", "UNICODE_CI", true);
+ assertEndsWith("i̇İi̇i̇", "̇İi̇İ", "UNICODE_CI", false);
+ assertEndsWith("the i̇o", "io", "UNICODE_CI", false);
+ assertEndsWith("the i̇o", "Io", "UNICODE_CI", false);
+ assertEndsWith("the i̇o", "i̇o", "UNICODE_CI", true);
+ assertEndsWith("the i̇o", "İo", "UNICODE_CI", true);
+ assertEndsWith("the İo", "io", "UNICODE_CI", false);
+ assertEndsWith("the İo", "Io", "UNICODE_CI", false);
+ assertEndsWith("the İo", "i̇o", "UNICODE_CI", true);
+ assertEndsWith("the İo", "İo", "UNICODE_CI", true);
+ assertEndsWith("i̇", "̇", "UTF8_BINARY_LCASE", true); // different from UNICODE_CI
Review Comment:
```suggestion
assertEndsWith("i̇", "\u0307", "UTF8_BINARY_LCASE", true); // different from UNICODE_CI
```
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -102,20 +102,30 @@ public void testContains() throws SparkException {
assertContains("äbćδe", "ÄbćδE", "UNICODE_CI", true);
assertContains("äbćδe", "ÄBcΔÉ", "UNICODE_CI", false);
// Case-variable character length
- assertContains("abİo12", "i̇o", "UNICODE_CI", true);
- assertContains("abi̇o12", "İo", "UNICODE_CI", true);
- assertContains("the İodine", "the i̇odine", "UTF8_BINARY_LCASE", true);
- assertContains("the i̇odine", "the İodine", "UTF8_BINARY_LCASE", true);
- assertContains("The İodiNe", " i̇oDin", "UTF8_BINARY_LCASE", true);
- assertContains("İodiNe", "i̇oDine", "UTF8_BINARY_LCASE", true);
- assertContains("İodiNe", " i̇oDin", "UTF8_BINARY_LCASE", false);
- // Characters with the same binary lowercase representation
- assertContains("The Kelvin.", "Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("The Kelvin.", "Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("The KKelvin.", "KKelvin", "UTF8_BINARY_LCASE", true);
- assertContains("2 Kelvin.", "2 Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("2 Kelvin.", "2 Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("The KKelvin.", "KKelvin,", "UTF8_BINARY_LCASE", false);
Review Comment:
I think we should keep these test cases. They do not do any harm.
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -709,12 +767,24 @@ public void testLocate() throws SparkException {
assertLocate("大千", "test大千世界大千世界", 9, "UNICODE_CI", 9);
assertLocate("大千", "大千世界大千世界", 1, "UNICODE_CI", 1);
// Case-variable character length
+ assertLocate("̇", "i̇", 1, "UTF8_BINARY", 2);
Review Comment:
```suggestion
assertLocate("\u0307", "i̇", 1, "UTF8_BINARY", 2);
```
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -709,12 +767,24 @@ public void testLocate() throws SparkException {
assertLocate("大千", "test大千世界大千世界", 9, "UNICODE_CI", 9);
assertLocate("大千", "大千世界大千世界", 1, "UNICODE_CI", 1);
// Case-variable character length
+ assertLocate("̇", "i̇", 1, "UTF8_BINARY", 2);
+ assertLocate("̇", "İ", 1, "UTF8_BINARY_LCASE", 0); // different from UTF8_BINARY
+ assertLocate("i", "i̇", 1, "UNICODE_CI", 0);
+ assertLocate("̇", "i̇", 1, "UNICODE_CI", 0);
Review Comment:
```suggestion
assertLocate("\u0307", "i̇", 1, "UNICODE_CI", 0);
```
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1599873181
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -102,20 +102,30 @@ public void testContains() throws SparkException {
assertContains("äbćδe", "ÄbćδE", "UNICODE_CI", true);
assertContains("äbćδe", "ÄBcΔÉ", "UNICODE_CI", false);
// Case-variable character length
- assertContains("abİo12", "i̇o", "UNICODE_CI", true);
- assertContains("abi̇o12", "İo", "UNICODE_CI", true);
- assertContains("the İodine", "the i̇odine", "UTF8_BINARY_LCASE", true);
- assertContains("the i̇odine", "the İodine", "UTF8_BINARY_LCASE", true);
- assertContains("The İodiNe", " i̇oDin", "UTF8_BINARY_LCASE", true);
- assertContains("İodiNe", "i̇oDine", "UTF8_BINARY_LCASE", true);
- assertContains("İodiNe", " i̇oDin", "UTF8_BINARY_LCASE", false);
- // Characters with the same binary lowercase representation
- assertContains("The Kelvin.", "Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("The Kelvin.", "Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("The KKelvin.", "KKelvin", "UTF8_BINARY_LCASE", true);
- assertContains("2 Kelvin.", "2 Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("2 Kelvin.", "2 Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("The KKelvin.", "KKelvin,", "UTF8_BINARY_LCASE", false);
Review Comment:
added these back
--
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-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1609894240
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,155 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found when searching for a pattern
+ * string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix, starting from the
+ * specified position (0-based index referring to character position in UTF8String), with respect
+ * to the UTF8_BINARY_LCASE collation. The method assumes that the prefix is already lowercased
+ * prior to method call to avoid the overhead of calling .toLowerCase() multiple times on the
+ * same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with the specified
+ * prefix, starting from the specified position (0-based index referring to character position
+ * in UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * prefix is already lowercased. The method only considers the part of target string that
+ * starts from the specified (inclusive) position (that is, the method does not look at UTF8
+ * characters of the target string at or after position `endPos`). If the prefix is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return length of the target substring that ends with the specified prefix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the first occurrence of the pattern string in the target string,
+ * starting from the specified position (0-based index referring to character position in
+ * UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * pattern string is already lowercased prior to call. If the pattern is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return the position of the first occurrence of pattern in target
+ */
+ public static int lowercaseFind(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int i = startPos; i <= target.numChars(); ++i) {
+ if (lowercaseMatchFrom(target, lowercasePattern, i)) {
+ return i;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns whether the target string ends with the specified suffix, ending at the specified
+ * position (0-based index referring to character position in UTF8String), with respect to the
+ * UTF8_BINARY_LCASE collation. The method assumes that the suffix is already lowercased prior
+ * to method call to avoid the overhead of calling .toLowerCase() multiple times on the same
+ * suffix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return whether the target string ends with the specified suffix in lowercase
+ */
+ public static boolean lowercaseMatchUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ return lowercaseMatchLengthUntil(target, lowercasePattern, endPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that ends with the specified
+ * suffix, ending at the specified position (0-based index referring to character position in
+ * UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * suffix is already lowercased. The method only considers the part of target string that ends
+ * at the specified (non-inclusive) position (that is, the method does not look at UTF8
+ * characters of the target string at or after position `endPos`). If the suffix is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ assert endPos <= target.numChars();
+ for (int len = 0; len <= endPos; ++len) {
+ if (target.substring(endPos - len, endPos).toLowerCase().equals(lowercasePattern)) {
Review Comment:
> Is it the same if we lower case the entire target string first
no, it's not - the changes in this PR (and some follow-up PRs) are related to changes
consider the example in the PR description: `contains("İ", "i")` where `haystack = "İ"` & `needle = "i"`
`lower(haystack) = lower("İ") = "i\u0307"` would produce `substr(lower("İ"), 1, 1) = "i" = needle`
however, we can see that these is no `substr(haystack, start, len)` such that `lower(substr(haystack, start, len)) == needle` - so these 2 behaviours are not actually equivalent (as we first assumed), and the reason for this discrepancy lies in conditional one-to-many case mapping (certain characters have lowercase equivalents consisted of multiple characters)
--
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-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1612004770
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -430,7 +430,7 @@ public static int execBinary(final UTF8String string, final UTF8String substring
}
public static int execLowercase(final UTF8String string, final UTF8String substring,
final int start) {
- return string.toLowerCase().indexOf(substring.toLowerCase(), start);
Review Comment:
no, unfortunately it's not - while it works fine for ASCII, it actually gives wrong results in some special cases featuring conditional case mapping, when a character has a lowercase equivalent that consists of multiple characters, or is found at a particular place in the string (context-awareness)
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1596334015
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java:
##########
@@ -215,43 +215,6 @@ public void contains() {
assertFalse(fromString("大千世界").contains(fromString("大千世界好")));
}
- @Test
- public void containsInLowerCase() {
Review Comment:
no need to run duplicate tests anyway, `CollationAwareUTF8String` is used only in `CollationSupport`
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1599736471
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -183,6 +204,19 @@ public static int findInSet(final UTF8String match, final UTF8String set, int co
return 0;
}
+ public static int lowercaseIndexOf(final UTF8String target, final UTF8String pattern,
+ final int start) {
+ if (pattern.numChars() == 0) return 0;
Review Comment:
that's not expected behaviour for `indexOf` in general, so I think it should stay 0 for `lowercaseIndexOf`
UTF8String `public int indexOf`:
```
if (v.numBytes() == 0) {
return 0;
}
```
CollationAwareUTF8String `public static int indexOf`:
```
if (pattern.numBytes() == 0) {
return 0;
}
```
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1604437739
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,143 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found
+ * when searching for a pattern string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix,
+ * with respect to the UTF8_BINARY_LCASE collation. The method assumes
+ * that the prefix is already lowercased prior to method call to avoid the
+ * overhead of calling .toLowerCase() multiple times on the same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with
+ * the specified prefix, with respect to the UTF8_BINARY_LCASE collation.
+ * The method assumes that the prefix is already lowercased. The method only
+ * considers the part of target string that starts from the specified position.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
Review Comment:
Yes, we'll address perf optimizations separately - so I'll create a ticket for that
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1604482878
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -709,12 +774,24 @@ public void testLocate() throws SparkException {
assertLocate("大千", "test大千世界大千世界", 9, "UNICODE_CI", 9);
assertLocate("大千", "大千世界大千世界", 1, "UNICODE_CI", 1);
// Case-variable character length
+ assertLocate("\u0307", "i̇", 1, "UTF8_BINARY", 2);
Review Comment:
the intent is: show that "\u0307" is found in "i̇" on the binary level (with respect to UTF8_BINARY)
but "\u0307" is not found in "İ" on the character level (with respect to UTF8_BINARY_LCASE)
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1599689228
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,27 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ public static boolean lowercaseMatchFrom(final UTF8String l, final UTF8String r, int pos) {
+ if (pos < 0) return false;
+ for (int len = 0; len <= l.numChars() - pos; len++) {
Review Comment:
actually, I think "lowercaseMatchFrom" should be able to match empty substring of `l` (len=0) to `r` (if `r` is an empty string itself)
otherwise, I would need to add a special case before the loop to cover this behaviour - I think there's no need to do that, since the first iteration of this loop will cover that scenario
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1596323254
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -118,7 +118,9 @@ public static boolean execBinary(final UTF8String l, final UTF8String r) {
return l.contains(r);
}
public static boolean execLowercase(final UTF8String l, final UTF8String r) {
- return l.containsInLowerCase(r);
+ if (r.numChars() == 0) return true;
+ if (l.numChars() < r.numChars()) return false;
Review Comment:
let me check if this is what you mean here, considering `haystack="İİİ"` and `needle="i̇i̇"`
there exists `len>0` such that `start=0` and `len=2` satisfy the condition `lower(substring(haystack, start, len)) == lower(needle)` i.e. `lower(substring("İİİ", 0, 2)) == lower("İİ") == "i̇i̇" == lower("i̇i̇")`
hence, `startsWith("İİİ", "i̇i̇")` should actually return `true`
in addition, `contains("İİİ", "i̇i̇")` should also return `true`
and `locate("İİİ", "i̇i̇")` should return `1` (needle found at the beginning of the haystack)
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1596739181
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -183,6 +192,22 @@ public static int findInSet(final UTF8String match, final UTF8String set, int co
return 0;
}
+ public static int lowercaseIndexOf(final UTF8String target, final UTF8String pattern,
+ final int start) {
+ if (pattern.numChars() == 0) return 0;
+ int lenHaystack = target.numChars(), lenNeedle = pattern.numChars();
+ final UTF8String firstLower = pattern.substring(0, 1).toLowerCase();
+ final UTF8String needle = pattern.toLowerCase();
+ for (int i = start; i <= (lenHaystack - lenNeedle); i++) {
+ if (target.substring(i, i + 1).toLowerCase().equals(firstLower)) {
Review Comment:
agreed, `firstLower` concept unfortunately doesn't hold here - removed in latest commit
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1596332638
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java:
##########
@@ -215,43 +215,6 @@ public void contains() {
assertFalse(fromString("大千世界").contains(fromString("大千世界好")));
}
- @Test
- public void containsInLowerCase() {
Review Comment:
starting at https://issues.apache.org/jira/browse/SPARK-47410, we've separated collation-related UTF8String logic away from `UTF8String.java` and into `CollationSupport.java`
these changes for UTF8_BINARY_LCASE have nothing to do with the original UTF8String implementation, so all tests are now found only in CollationSupportSuite (instead of UTF8StringSuite) - we don't have a UTF8LCASEStringSuite, and I don't think we need one, so CollationSupportSuite should satisfy
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1599689228
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,27 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ public static boolean lowercaseMatchFrom(final UTF8String l, final UTF8String r, int pos) {
+ if (pos < 0) return false;
+ for (int len = 0; len <= l.numChars() - pos; len++) {
Review Comment:
actually, I think "lowercaseMatchFrom" should be able to match empty substring of `l` (len=0) to `r` (if `r` is an empty string itself)
otherwise, I would need to add a special case before the loop to cover this behaviour - I think there's no need to do that, since the first iteration of this loop will cover that scenario
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation [spark]
Posted by "mkaravel (via GitHub)" <gi...@apache.org>.
mkaravel commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1600292083
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -183,6 +204,19 @@ public static int findInSet(final UTF8String match, final UTF8String set, int co
return 0;
}
+ public static int lowercaseIndexOf(final UTF8String target, final UTF8String pattern,
+ final int start) {
+ if (pattern.numChars() == 0) return 0;
Review Comment:
I have to admit this is a bit strange. Can we check with the OSS team that the UTF8String behavior is the expected one?
--
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-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1612011134
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -430,7 +430,7 @@ public static int execBinary(final UTF8String string, final UTF8String substring
}
public static int execLowercase(final UTF8String string, final UTF8String substring,
final int start) {
- return string.toLowerCase().indexOf(substring.toLowerCase(), start);
Review Comment:
so as part of this PR, we actually changed the core definition of string-searching in UTF8_BINARY_LCASE, i.e. what it means for one substring (_pattern_) to be found in another string (_target_) under UTF8_BINARY_LCASE
in the old implementation, `contains("İ", "i")` would return `true` - however, this behaviour is incorrect because it relies on the fact that `substr(lower("İ"), 1, 1)` == `"i"` (incorrect, old implementation), instead of `lower(substr("İ", 1, 1))` != `"i"` (correct, new implementation)
and this is all due to the fact that `lower("İ") = "i\u0307"` (1 uppercase character -> 2 lowercase characters)
--
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-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1612011134
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -430,7 +430,7 @@ public static int execBinary(final UTF8String string, final UTF8String substring
}
public static int execLowercase(final UTF8String string, final UTF8String substring,
final int start) {
- return string.toLowerCase().indexOf(substring.toLowerCase(), start);
Review Comment:
so as part of this PR, we actually changed the core definition of string-searching in UTF8_BINARY_LCASE, i.e. what it means for one substring (_pattern_) to be found in another string (_target_) under UTF8_BINARY_LCASE
in the old implementation, `contains("İ", "i")` would return `true` - however, this behaviour is incorrect because it relies on the fact that `substr(lower("İ"), 1, 1)` == `"i"` (incorrect, old implementation), instead of `lower(substr("İ"substr))` != `"i"` (correct, new implementation)
--
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-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1609894240
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,155 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found when searching for a pattern
+ * string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix, starting from the
+ * specified position (0-based index referring to character position in UTF8String), with respect
+ * to the UTF8_BINARY_LCASE collation. The method assumes that the prefix is already lowercased
+ * prior to method call to avoid the overhead of calling .toLowerCase() multiple times on the
+ * same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with the specified
+ * prefix, starting from the specified position (0-based index referring to character position
+ * in UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * prefix is already lowercased. The method only considers the part of target string that
+ * starts from the specified (inclusive) position (that is, the method does not look at UTF8
+ * characters of the target string at or after position `endPos`). If the prefix is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return length of the target substring that ends with the specified prefix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the first occurrence of the pattern string in the target string,
+ * starting from the specified position (0-based index referring to character position in
+ * UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * pattern string is already lowercased prior to call. If the pattern is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return the position of the first occurrence of pattern in target
+ */
+ public static int lowercaseFind(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int i = startPos; i <= target.numChars(); ++i) {
+ if (lowercaseMatchFrom(target, lowercasePattern, i)) {
+ return i;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns whether the target string ends with the specified suffix, ending at the specified
+ * position (0-based index referring to character position in UTF8String), with respect to the
+ * UTF8_BINARY_LCASE collation. The method assumes that the suffix is already lowercased prior
+ * to method call to avoid the overhead of calling .toLowerCase() multiple times on the same
+ * suffix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return whether the target string ends with the specified suffix in lowercase
+ */
+ public static boolean lowercaseMatchUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ return lowercaseMatchLengthUntil(target, lowercasePattern, endPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that ends with the specified
+ * suffix, ending at the specified position (0-based index referring to character position in
+ * UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * suffix is already lowercased. The method only considers the part of target string that ends
+ * at the specified (non-inclusive) position (that is, the method does not look at UTF8
+ * characters of the target string at or after position `endPos`). If the suffix is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ assert endPos <= target.numChars();
+ for (int len = 0; len <= endPos; ++len) {
+ if (target.substring(endPos - len, endPos).toLowerCase().equals(lowercasePattern)) {
Review Comment:
> Is it the same if we lower case the entire target string first
no, it's not - the changes in this PR (and some follow-up PRs) are related to changes
consider the example in the PR description: `contains("İ", "i")` where `haystack = "İ"` & `needle = "i"`
`lower(haystack) = lower("İ") = "i\u0307"` would produce `substr(lower("İ"), 1, 1) = "i" = needle`
however, we can see that these is no `substr(haystack, start, len)` such that `lower(substr(haystack, start, len)) == needle` - so these 2 behaviours are not actually equivalent (as we first assumed), and the reason for this discrepancy lies in conditional one-to-many case mapping (certain characters have lowercase equivalents consisted of multiple characters)
--
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-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1609894240
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,155 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ /**
+ * The constant value to indicate that the match is not found when searching for a pattern
+ * string in a target string.
+ */
+ private static final int MATCH_NOT_FOUND = -1;
+
+ /**
+ * Returns whether the target string starts with the specified prefix, starting from the
+ * specified position (0-based index referring to character position in UTF8String), with respect
+ * to the UTF8_BINARY_LCASE collation. The method assumes that the prefix is already lowercased
+ * prior to method call to avoid the overhead of calling .toLowerCase() multiple times on the
+ * same prefix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return whether the target string starts with the specified prefix in UTF8_BINARY_LCASE
+ */
+ public static boolean lowercaseMatchFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ return lowercaseMatchLengthFrom(target, lowercasePattern, startPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that starts with the specified
+ * prefix, starting from the specified position (0-based index referring to character position
+ * in UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * prefix is already lowercased. The method only considers the part of target string that
+ * starts from the specified (inclusive) position (that is, the method does not look at UTF8
+ * characters of the target string at or after position `endPos`). If the prefix is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return length of the target substring that ends with the specified prefix in lowercase
+ */
+ public static int lowercaseMatchLengthFrom(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int len = 0; len <= target.numChars() - startPos; ++len) {
+ if (target.substring(startPos, startPos + len).toLowerCase().equals(lowercasePattern)) {
+ return len;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the position of the first occurrence of the pattern string in the target string,
+ * starting from the specified position (0-based index referring to character position in
+ * UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * pattern string is already lowercased prior to call. If the pattern is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param startPos the start position for searching (in the target string)
+ * @return the position of the first occurrence of pattern in target
+ */
+ public static int lowercaseFind(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int startPos) {
+ assert startPos >= 0;
+ for (int i = startPos; i <= target.numChars(); ++i) {
+ if (lowercaseMatchFrom(target, lowercasePattern, i)) {
+ return i;
+ }
+ }
+ return MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns whether the target string ends with the specified suffix, ending at the specified
+ * position (0-based index referring to character position in UTF8String), with respect to the
+ * UTF8_BINARY_LCASE collation. The method assumes that the suffix is already lowercased prior
+ * to method call to avoid the overhead of calling .toLowerCase() multiple times on the same
+ * suffix string.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return whether the target string ends with the specified suffix in lowercase
+ */
+ public static boolean lowercaseMatchUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ return lowercaseMatchLengthUntil(target, lowercasePattern, endPos) != MATCH_NOT_FOUND;
+ }
+
+ /**
+ * Returns the length of the substring of the target string that ends with the specified
+ * suffix, ending at the specified position (0-based index referring to character position in
+ * UTF8String), with respect to the UTF8_BINARY_LCASE collation. The method assumes that the
+ * suffix is already lowercased. The method only considers the part of target string that ends
+ * at the specified (non-inclusive) position (that is, the method does not look at UTF8
+ * characters of the target string at or after position `endPos`). If the suffix is not found,
+ * MATCH_NOT_FOUND is returned.
+ *
+ * @param target the string to be searched in
+ * @param lowercasePattern the string to be searched for
+ * @param endPos the end position for searching (in the target string)
+ * @return length of the target substring that ends with the specified suffix in lowercase
+ */
+ public static int lowercaseMatchLengthUntil(
+ final UTF8String target,
+ final UTF8String lowercasePattern,
+ int endPos) {
+ assert endPos <= target.numChars();
+ for (int len = 0; len <= endPos; ++len) {
+ if (target.substring(endPos - len, endPos).toLowerCase().equals(lowercasePattern)) {
Review Comment:
> Is it the same if we lower case the entire target string first
no, it's not - the changes in this PR (and some follow-up PRs) are related to changes
consider the example in the PR description: `contains("İ", "i")` where `haystack = "İ"` & `needle = "i"`
`lower(haystack) = lower("İ") = "i\u0307"` would produce `substr(lower("İ"), 1, 1) = "i" = needle`
however, we can see that these is no `substr(haystack, start, len)` such that `lower(substr(haystack, start, len)) == needle` - so these 2 behaviours are not actually equivalent (as we first assumed), and the reason for this discrepancy lies in conditional one-to-many case mapping (certain characters have lowercase equivalents consisted of multiple)
--
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-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation (Contains, StartsWith, EndsWith, StringLocate) [spark]
Posted by "mkaravel (via GitHub)" <gi...@apache.org>.
mkaravel commented on PR #46511:
URL: https://github.com/apache/spark/pull/46511#issuecomment-2123824322
@uros-db
I am reading this in the PR description:
> Why are the changes needed?
Fix functions that give unexpected results due to the conditional case mapping when performing string comparisons under UTF8_BINARY_LCASE.
This is not really the reason we we are making this change (conditional case mapping is the reason why we want to change the comparator for UTF8_BINARY_LCASE, but not related to string search directly).
The real reason is that we need to do string search at the UT8 char-by-char level because otherwise indices returned or considered during string search can return strange and unusable results due to the fact that case mappings are one-to-many, that is one UTF8 character can map to two or more UTF8 characters under the case mapping (both for upper and lower casing).
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1596735843
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,15 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ public static boolean lowercaseMatchAt(final UTF8String l, final UTF8String r,
+ int pos, int len) {
+ if (len + pos > l.numChars() || pos < 0) {
+ return false;
+ }
+ return l.substring(pos, pos + len).toLowerCase().equals(r);
Review Comment:
update: resolved in latest commit (we're no longer trying to match a known number of characters)
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1596329450
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -102,20 +102,28 @@ public void testContains() throws SparkException {
assertContains("äbćδe", "ÄbćδE", "UNICODE_CI", true);
assertContains("äbćδe", "ÄBcΔÉ", "UNICODE_CI", false);
// Case-variable character length
- assertContains("abİo12", "i̇o", "UNICODE_CI", true);
- assertContains("abi̇o12", "İo", "UNICODE_CI", true);
- assertContains("the İodine", "the i̇odine", "UTF8_BINARY_LCASE", true);
- assertContains("the i̇odine", "the İodine", "UTF8_BINARY_LCASE", true);
- assertContains("The İodiNe", " i̇oDin", "UTF8_BINARY_LCASE", true);
- assertContains("İodiNe", "i̇oDine", "UTF8_BINARY_LCASE", true);
- assertContains("İodiNe", " i̇oDin", "UTF8_BINARY_LCASE", false);
- // Characters with the same binary lowercase representation
- assertContains("The Kelvin.", "Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("The Kelvin.", "Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("The KKelvin.", "KKelvin", "UTF8_BINARY_LCASE", true);
- assertContains("2 Kelvin.", "2 Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("2 Kelvin.", "2 Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("The KKelvin.", "KKelvin,", "UTF8_BINARY_LCASE", false);
+ assertContains("i̇", "i", "UNICODE_CI", false);
+ assertContains("i̇", "İ", "UNICODE_CI", true);
+ assertContains("İ", "i", "UNICODE_CI", false);
+ assertContains("adi̇os", "io", "UNICODE_CI", false);
+ assertContains("adi̇os", "Io", "UNICODE_CI", false);
+ assertContains("adi̇os", "i̇o", "UNICODE_CI", true);
+ assertContains("adi̇os", "İo", "UNICODE_CI", true);
+ assertContains("adİos", "io", "UNICODE_CI", false);
+ assertContains("adİos", "Io", "UNICODE_CI", false);
+ assertContains("adİos", "i̇o", "UNICODE_CI", true);
+ assertContains("adİos", "İo", "UNICODE_CI", true);
+ assertContains("i̇", "i", "UTF8_BINARY_LCASE", true); // note: different from UNICODE_CI
+ assertContains("i̇", "İ", "UTF8_BINARY_LCASE", false); // note: different from UNICODE_CI
+ assertContains("İ", "i", "UTF8_BINARY_LCASE", false);
+ assertContains("adi̇os", "io", "UTF8_BINARY_LCASE", false);
+ assertContains("adi̇os", "Io", "UTF8_BINARY_LCASE", false);
+ assertContains("adi̇os", "i̇o", "UTF8_BINARY_LCASE", true);
+ assertContains("adi̇os", "İo", "UTF8_BINARY_LCASE", false); // note: different from UNICODE_CI
+ assertContains("adİos", "io", "UTF8_BINARY_LCASE", false);
+ assertContains("adİos", "Io", "UTF8_BINARY_LCASE", false);
+ assertContains("adİos", "i̇o", "UTF8_BINARY_LCASE", false); // note: different from UNICODE_CI
Review Comment:
let's justify this, considering haystack="adİos" and needle="i̇o"
there exists `len>0` such that `start=3` and `len=2` satisfy the condition `lower(substring(haystack, start, len)) == lower(needle)` i.e. `lower(substring("adİos", 3, 2)) == lower("İo") == "i̇o" == lower("i̇o")`
hence, `contains("adİos", "i̇o")` should actually return `true`
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1596738413
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,15 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ public static boolean lowercaseMatchAt(final UTF8String l, final UTF8String r,
+ int pos, int len) {
+ if (len + pos > l.numChars() || pos < 0) {
+ return false;
+ }
+ return l.substring(pos, pos + len).toLowerCase().equals(r);
Review Comment:
> Shouldn't this be?
>
> return l.substring(pos, pos + len).toLowerCase().equals(r.toLowerCase());
not here, because we know needle is not gonna change (we won't take any substrings of needle), then we can lowercase the needle only once (before the function call) in order to avoid multiple lowercasing (this is only important for `contains` which turns out to be quadratic - just like ICU StringSearch algorithm, while `startsWith` and `endsWith` are linear in nature because one bound is known in advance)
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -102,20 +102,28 @@ public void testContains() throws SparkException {
assertContains("äbćδe", "ÄbćδE", "UNICODE_CI", true);
assertContains("äbćδe", "ÄBcΔÉ", "UNICODE_CI", false);
// Case-variable character length
- assertContains("abİo12", "i̇o", "UNICODE_CI", true);
- assertContains("abi̇o12", "İo", "UNICODE_CI", true);
- assertContains("the İodine", "the i̇odine", "UTF8_BINARY_LCASE", true);
- assertContains("the i̇odine", "the İodine", "UTF8_BINARY_LCASE", true);
- assertContains("The İodiNe", " i̇oDin", "UTF8_BINARY_LCASE", true);
- assertContains("İodiNe", "i̇oDine", "UTF8_BINARY_LCASE", true);
- assertContains("İodiNe", " i̇oDin", "UTF8_BINARY_LCASE", false);
- // Characters with the same binary lowercase representation
- assertContains("The Kelvin.", "Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("The Kelvin.", "Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("The KKelvin.", "KKelvin", "UTF8_BINARY_LCASE", true);
- assertContains("2 Kelvin.", "2 Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("2 Kelvin.", "2 Kelvin", "UTF8_BINARY_LCASE", true);
- assertContains("The KKelvin.", "KKelvin,", "UTF8_BINARY_LCASE", false);
+ assertContains("i̇", "i", "UNICODE_CI", false);
+ assertContains("i̇", "İ", "UNICODE_CI", true);
+ assertContains("İ", "i", "UNICODE_CI", false);
+ assertContains("adi̇os", "io", "UNICODE_CI", false);
+ assertContains("adi̇os", "Io", "UNICODE_CI", false);
+ assertContains("adi̇os", "i̇o", "UNICODE_CI", true);
+ assertContains("adi̇os", "İo", "UNICODE_CI", true);
+ assertContains("adİos", "io", "UNICODE_CI", false);
+ assertContains("adİos", "Io", "UNICODE_CI", false);
+ assertContains("adİos", "i̇o", "UNICODE_CI", true);
+ assertContains("adİos", "İo", "UNICODE_CI", true);
+ assertContains("i̇", "i", "UTF8_BINARY_LCASE", true); // note: different from UNICODE_CI
+ assertContains("i̇", "İ", "UTF8_BINARY_LCASE", false); // note: different from UNICODE_CI
+ assertContains("İ", "i", "UTF8_BINARY_LCASE", false);
+ assertContains("adi̇os", "io", "UTF8_BINARY_LCASE", false);
+ assertContains("adi̇os", "Io", "UTF8_BINARY_LCASE", false);
+ assertContains("adi̇os", "i̇o", "UTF8_BINARY_LCASE", true);
+ assertContains("adi̇os", "İo", "UTF8_BINARY_LCASE", false); // note: different from UNICODE_CI
+ assertContains("adİos", "io", "UTF8_BINARY_LCASE", false);
+ assertContains("adİos", "Io", "UTF8_BINARY_LCASE", false);
+ assertContains("adİos", "i̇o", "UTF8_BINARY_LCASE", false); // note: different from UNICODE_CI
Review Comment:
update: resolved in latest commit
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1600495650
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -183,6 +204,19 @@ public static int findInSet(final UTF8String match, final UTF8String set, int co
return 0;
}
+ public static int lowercaseIndexOf(final UTF8String target, final UTF8String pattern,
+ final int start) {
+ if (pattern.numChars() == 0) return 0;
Review Comment:
why do you think it would not be expected? `indexOf` uses 0-based index
- -1 means "not found"
- 0 means "found at beginning"
an empty pattern string is (by definition) found at the beginning of any given target string
I don't think there are any issues there - seems pretty expected to me
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1599693830
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -34,6 +34,27 @@
* Utility class for collation-aware UTF8String operations.
*/
public class CollationAwareUTF8String {
+
+ public static boolean lowercaseMatchFrom(final UTF8String l, final UTF8String r, int pos) {
+ if (pos < 0) return false;
Review Comment:
`lowercaseMatchFrom` here is part of a public interface, so I think we do need to establish some rules on how it will behave when given a negative position
the ways I see of doing this are:
- return false by default
- throw an exception
- assert that pos is not negative
In this case, I agree that assert is the best choice, so I'll implement that
--
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] [WIP][SPARK-48221][SQL] Alter string search logic for UTF8_BINARY_LCASE collation [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #46511:
URL: https://github.com/apache/spark/pull/46511#discussion_r1599772157
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -183,6 +204,19 @@ public static int findInSet(final UTF8String match, final UTF8String set, int co
return 0;
}
+ public static int lowercaseIndexOf(final UTF8String target, final UTF8String pattern,
Review Comment:
Added
--
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