You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "stevomitric (via GitHub)" <gi...@apache.org> on 2024/08/26 11:57:53 UTC

[PR] [SPARK-49204][SQL][FOLLOWUP] Fix IndexOutOfBoundsException when dealing with surrogate pairs [spark]

stevomitric opened a new pull request, #47871:
URL: https://github.com/apache/spark/pull/47871

   ### What changes were proposed in this pull request?
   Modified the `StringLocate` ICU codepath and added a codepoint count check for the provided offset.
   
   
   ### Why are the changes needed?
   Currently, doing the following throws an `java.lang.IndexOutOfBoundsException`.
   ```sql
   select position('asd' COLLATE UNICODE, 'a', 10)
   ```
   while the correct behavior is to return an false match result (0).
   
   ### Does this PR introduce _any_ user-facing change?
   Yes, fixes the `java.lang.IndexOutOfBoundsException` error.
   
   
   ### How was this patch tested?
   New test cases in this PR.
   
   
   ### Was this patch authored or co-authored using generative AI tooling?
   No
   


-- 
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-49204][SQL][FOLLOWUP] Fix IndexOutOfBoundsException when dealing with surrogate pairs [spark]

Posted by "stevomitric (via GitHub)" <gi...@apache.org>.
stevomitric commented on PR #47871:
URL: https://github.com/apache/spark/pull/47871#issuecomment-2310115006

   I think you misunderstood the nature of the fix here.
   
   As stated in the description, the addressed issue is only for UNICODE code path (for non empty 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] [SPARK-49204][SQL][FOLLOWUP] Fix IndexOutOfBoundsException when dealing with surrogate pairs [spark]

Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #47871:
URL: https://github.com/apache/spark/pull/47871#issuecomment-2311961909

   @stevomitric Just to double check, only master suffers from the issue, correct?


-- 
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-49204][SQL][FOLLOWUP] Fix IndexOutOfBoundsException when dealing with surrogate pairs [spark]

Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on PR #47871:
URL: https://github.com/apache/spark/pull/47871#issuecomment-2310120077

   yes, I see the fix in this PR is regarding non-empty patterns - however, please add these tests with empty patterns as well


-- 
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-49204][SQL][FOLLOWUP] Fix IndexOutOfBoundsException when dealing with surrogate pairs [spark]

Posted by "stevomitric (via GitHub)" <gi...@apache.org>.
stevomitric commented on PR #47871:
URL: https://github.com/apache/spark/pull/47871#issuecomment-2310031712

   Adding @uros-db to take a look.


-- 
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-49204][SQL][FOLLOWUP] Fix IndexOutOfBoundsException when dealing with surrogate pairs [spark]

Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #47871:
URL: https://github.com/apache/spark/pull/47871#discussion_r1731421102


##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -2329,6 +2329,19 @@ public void testStringLocate() throws SparkException {
     assertStringLocate("b", "a🙃x🙃b", 4, "UTF8_LCASE", 5);
     assertStringLocate("b", "a🙃x🙃b", 4, "UNICODE", 5);
     assertStringLocate("b", "a🙃x🙃b", 4, "UNICODE_CI", 5);
+    // Out of bounds test cases.
+    assertStringLocate("a", "asd", 4, "UTF8_BINARY", 0);
+    assertStringLocate("a", "asd", 4, "UTF8_LCASE", 0);
+    assertStringLocate("a", "asd", 4, "UNICODE", 0);
+    assertStringLocate("a", "asd", 4, "UNICODE_CI", 0);

Review Comment:
   thanks, sorry - yes, "1" is what I meant



-- 
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-49204][SQL][FOLLOWUP] Fix IndexOutOfBoundsException when dealing with surrogate pairs [spark]

Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #47871:
URL: https://github.com/apache/spark/pull/47871#issuecomment-2311823465

   @stevomitric Highly likely this GA [Run / Protobuf breaking change detection and Python CodeGen check](https://github.com/stevomitric/spark/actions/runs/10562522322/job/29260715190#logs) is not related to your changes. Could you re-run this failed GA **only**.


-- 
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-49204][SQL][FOLLOWUP] Fix IndexOutOfBoundsException when dealing with surrogate pairs [spark]

Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #47871:
URL: https://github.com/apache/spark/pull/47871#discussion_r1731176914


##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -2329,6 +2329,19 @@ public void testStringLocate() throws SparkException {
     assertStringLocate("b", "a🙃x🙃b", 4, "UTF8_LCASE", 5);
     assertStringLocate("b", "a🙃x🙃b", 4, "UNICODE", 5);
     assertStringLocate("b", "a🙃x🙃b", 4, "UNICODE_CI", 5);
+    // Out of bounds test cases.
+    assertStringLocate("a", "asd", 4, "UTF8_BINARY", 0);
+    assertStringLocate("a", "asd", 4, "UTF8_LCASE", 0);
+    assertStringLocate("a", "asd", 4, "UNICODE", 0);
+    assertStringLocate("a", "asd", 4, "UNICODE_CI", 0);

Review Comment:
   ```suggestion
       assertStringLocate("a", "asd", 4, "UTF8_BINARY", 0);
       assertStringLocate("a", "asd", 4, "UTF8_LCASE", 0);
       assertStringLocate("a", "asd", 4, "UNICODE", 0);
       assertStringLocate("a", "asd", 4, "UNICODE_CI", 0);
       assertStringLocate("", "asd", 100, "UTF8_BINARY", 100);
       assertStringLocate("", "asd", 100, "UTF8_LCASE", 100);
       assertStringLocate("", "asd", 100, "UNICODE", 100);
       assertStringLocate("", "asd", 100, "UNICODE_CI", 100);
   ```



-- 
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-49204][SQL][FOLLOWUP] Fix IndexOutOfBoundsException when dealing with surrogate pairs [spark]

Posted by "stevomitric (via GitHub)" <gi...@apache.org>.
stevomitric commented on PR #47871:
URL: https://github.com/apache/spark/pull/47871#issuecomment-2311950489

   Checks passed. @MaxGekk 


-- 
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-49204][SQL][FOLLOWUP] Fix IndexOutOfBoundsException when dealing with surrogate pairs [spark]

Posted by "stevomitric (via GitHub)" <gi...@apache.org>.
stevomitric commented on PR #47871:
URL: https://github.com/apache/spark/pull/47871#issuecomment-2311978766

   @MaxGekk yes. the problem was introduces by this PR 2 weeks ago https://github.com/apache/spark/pull/47711


-- 
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-49204][SQL][FOLLOWUP] Fix IndexOutOfBoundsException when dealing with surrogate pairs [spark]

Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #47871: [SPARK-49204][SQL][FOLLOWUP] Fix IndexOutOfBoundsException when dealing with surrogate pairs
URL: https://github.com/apache/spark/pull/47871


-- 
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-49204][SQL][FOLLOWUP] Fix IndexOutOfBoundsException when dealing with surrogate pairs [spark]

Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #47871:
URL: https://github.com/apache/spark/pull/47871#discussion_r1731165842


##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -671,6 +671,11 @@ public static int indexOf(final UTF8String target, final UTF8String pattern,
     // Initialize the string search with respect to the specified ICU collation.
     String targetStr = target.toValidString();
     String patternStr = pattern.toValidString();
+    // Check if `start` is out of bounds. The provided offset `start` is given in number of
+    // codepoints, so a simple `targetStr.length` check is not sufficient here. This check is
+    // needed because `String.offsetByCodePoints` throws an `IndexOutOfBoundsException`
+    // exception when the offset is out of bounds.
+    if (targetStr.codePointCount(0, targetStr.length()) <= start) return MATCH_NOT_FOUND;

Review Comment:
   ```suggestion
       if (targetStr.codePointCount(0, targetStr.length()) <= start) return start;
   ```



-- 
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-49204][SQL][FOLLOWUP] Fix IndexOutOfBoundsException when dealing with surrogate pairs [spark]

Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #47871:
URL: https://github.com/apache/spark/pull/47871#discussion_r1731426477


##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -671,6 +671,11 @@ public static int indexOf(final UTF8String target, final UTF8String pattern,
     // Initialize the string search with respect to the specified ICU collation.
     String targetStr = target.toValidString();
     String patternStr = pattern.toValidString();
+    // Check if `start` is out of bounds. The provided offset `start` is given in number of
+    // codepoints, so a simple `targetStr.length` check is not sufficient here. This check is

Review Comment:
   Thank you for the explanation.



-- 
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-49204][SQL][FOLLOWUP] Fix IndexOutOfBoundsException when dealing with surrogate pairs [spark]

Posted by "stevomitric (via GitHub)" <gi...@apache.org>.
stevomitric commented on PR #47871:
URL: https://github.com/apache/spark/pull/47871#issuecomment-2311890524

   All tests passed. Pinging @MaxGekk 


-- 
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-49204][SQL][FOLLOWUP] Fix IndexOutOfBoundsException when dealing with surrogate pairs [spark]

Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #47871:
URL: https://github.com/apache/spark/pull/47871#discussion_r1731165842


##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java:
##########
@@ -671,6 +671,11 @@ public static int indexOf(final UTF8String target, final UTF8String pattern,
     // Initialize the string search with respect to the specified ICU collation.
     String targetStr = target.toValidString();
     String patternStr = pattern.toValidString();
+    // Check if `start` is out of bounds. The provided offset `start` is given in number of
+    // codepoints, so a simple `targetStr.length` check is not sufficient here. This check is
+    // needed because `String.offsetByCodePoints` throws an `IndexOutOfBoundsException`
+    // exception when the offset is out of bounds.
+    if (targetStr.codePointCount(0, targetStr.length()) <= start) return MATCH_NOT_FOUND;

Review Comment:
   ```suggestion
       if (targetStr.codePointCount(0, targetStr.length()) <= start) return start;
   ```



-- 
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-49204][SQL][FOLLOWUP] Fix IndexOutOfBoundsException when dealing with surrogate pairs [spark]

Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #47871:
URL: https://github.com/apache/spark/pull/47871#issuecomment-2311954687

   +1, LGTM. Merging to master.
   Thank you, @stevomitric and @uros-db for review.


-- 
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-49204][SQL][FOLLOWUP] Fix IndexOutOfBoundsException when dealing with surrogate pairs [spark]

Posted by "stevomitric (via GitHub)" <gi...@apache.org>.
stevomitric commented on code in PR #47871:
URL: https://github.com/apache/spark/pull/47871#discussion_r1731408947


##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -2329,6 +2329,19 @@ public void testStringLocate() throws SparkException {
     assertStringLocate("b", "a🙃x🙃b", 4, "UTF8_LCASE", 5);
     assertStringLocate("b", "a🙃x🙃b", 4, "UNICODE", 5);
     assertStringLocate("b", "a🙃x🙃b", 4, "UNICODE_CI", 5);
+    // Out of bounds test cases.
+    assertStringLocate("a", "asd", 4, "UTF8_BINARY", 0);
+    assertStringLocate("a", "asd", 4, "UTF8_LCASE", 0);
+    assertStringLocate("a", "asd", 4, "UNICODE", 0);
+    assertStringLocate("a", "asd", 4, "UNICODE_CI", 0);

Review Comment:
   Also the suggested edit here is wrong, the expected result should be 1 (as per the UTF8 Binary collation).



##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -2329,6 +2329,19 @@ public void testStringLocate() throws SparkException {
     assertStringLocate("b", "a🙃x🙃b", 4, "UTF8_LCASE", 5);
     assertStringLocate("b", "a🙃x🙃b", 4, "UNICODE", 5);
     assertStringLocate("b", "a🙃x🙃b", 4, "UNICODE_CI", 5);
+    // Out of bounds test cases.
+    assertStringLocate("a", "asd", 4, "UTF8_BINARY", 0);
+    assertStringLocate("a", "asd", 4, "UTF8_LCASE", 0);
+    assertStringLocate("a", "asd", 4, "UNICODE", 0);
+    assertStringLocate("a", "asd", 4, "UNICODE_CI", 0);

Review Comment:
   These tests (with empty strings) won't hit the codepath, but i will include them since it falls into the category i stated in the comment above.



-- 
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