Skip to content

Commit

Permalink
[SPARK-48435][SQL] UNICODE collation should not support binary equality
Browse files Browse the repository at this point in the history 
### What changes were proposed in this pull request?
CollationFactory has been updated to no longer mark UNICODE as collation that supportsBinaryCollation. To reflect these changes, various tests have been updated.

However, some tests have been (temporarily) removed because StringTrim no longer supports UNICODE collation given the new UNICODE definition in CollationFactory. At this time, StringTrim expression only supports UTF8_BINARY & UTF8_BINARY_LCASE, but not ICU collations. This work is in progress (#46762), so we'll ensure appropriate test coverage with those changes.

### Why are the changes needed?
UNICODE collation should not support binary collation. Note: in the future, we may want to consider a collation such as UNICODE_BINARY, which will support binary equality, but also maintain UNICODE ordering.

### Does this PR introduce _any_ user-facing change?
Yes, UNICODE is no longer treated as a binary collation. This affects how equality works for UNICODE, and also which codepath is taken for various collation-aware string expression given UNICODE collated string arguments.

### How was this patch tested?
Updated existing unit and e2e sql test for UNICODE collation.

### Was this patch authored or co-authored using generative AI tooling?
No.

Closes #46772 from uros-db/fix-unicode.

Authored-by: Uros Bojanic <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
  • Loading branch information
@uros-db @cloud-fan
uros-db authored and cloud-fan committed Jun 6, 2024
1 parent 3878b57 commit b5a4b32
Show file tree
Hide file tree
Showing 9 changed files with 86 additions and 111 deletions.
5 changes: 2 additions & 3 deletions common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -351,9 +351,8 @@ public static int lowercaseIndexOf(final UTF8String target, final UTF8String pat

public static int indexOf(final UTF8String target, final UTF8String pattern,
final int start, final int collationId) {
if (pattern.numBytes() == 0) {
return target.indexOfEmpty(start);
}
if (pattern.numBytes() == 0) return target.indexOfEmpty(start);
if (target.numBytes() == 0) return MATCH_NOT_FOUND;

StringSearch stringSearch = CollationFactory.getStringSearch(target, pattern, collationId);
stringSearch.setIndex(start);
Expand Down
2 changes: 1 addition & 1 deletion common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -671,7 +671,7 @@ protected Collation buildCollation() {
(s1, s2) -> collator.compare(s1.toString(), s2.toString()),
ICU_COLLATOR_VERSION,
s -> (long) collator.getCollationKey(s.toString()).hashCode(),
/* supportsBinaryEquality = */ collationId == UNICODE_COLLATION_ID,
/* supportsBinaryEquality = */ false,
/* supportsBinaryOrdering = */ false,
/* supportsLowercaseEquality = */ false);
}
Expand Down
36 changes: 2 additions & 34 deletions common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,8 @@ public void testStringInstr() throws SparkException {
assertStringInstr("", "xxxx", "UNICODE", 0);
assertStringInstr("test大千世界X大千世界", "界x", "UNICODE", 0);
assertStringInstr("test大千世界X大千世界", "界X", "UNICODE", 8);
assertStringInstr("xxxx", "", "UNICODE_CI", 1);
assertStringInstr("", "xxxx", "UNICODE_CI", 0);
assertStringInstr("aaads", "AD", "UNICODE_CI", 3);
assertStringInstr("aaads", "dS", "UNICODE_CI", 4);
assertStringInstr("test大千世界X大千世界", "界y", "UNICODE_CI", 0);
Expand Down Expand Up @@ -1040,20 +1042,6 @@ public void testStringTrim() throws SparkException {
assertStringTrim("UTF8_BINARY_LCASE", "xxasdxx", "x", "asd");
assertStringTrim("UTF8_BINARY_LCASE", "xa世ax", "x", "a世a");

assertStringTrimLeft("UNICODE", "asd", null, "asd");
assertStringTrimLeft("UNICODE", " asd ", null, "asd ");
assertStringTrimLeft("UNICODE", " a世a ", null, "a世a ");
assertStringTrimLeft("UNICODE", "asd", "x", "asd");
assertStringTrimLeft("UNICODE", "xxasdxx", "x", "asdxx");
assertStringTrimLeft("UNICODE", "xa世ax", "x", "a世ax");

assertStringTrimRight("UNICODE", "asd", null, "asd");
assertStringTrimRight("UNICODE", " asd ", null, " asd");
assertStringTrimRight("UNICODE", " a世a ", null, " a世a");
assertStringTrimRight("UNICODE", "asd", "x", "asd");
assertStringTrimRight("UNICODE", "xxasdxx", "x", "xxasd");
assertStringTrimRight("UNICODE", "xa世ax", "x", "xa世a");

// Test cases where trimString has more than one character
assertStringTrim("UTF8_BINARY", "ddsXXXaa", "asd", "XXX");
assertStringTrimLeft("UTF8_BINARY", "ddsXXXaa", "asd", "XXXaa");
Expand All @@ -1063,22 +1051,14 @@ public void testStringTrim() throws SparkException {
assertStringTrimLeft("UTF8_BINARY_LCASE", "ddsXXXaa", "asd", "XXXaa");
assertStringTrimRight("UTF8_BINARY_LCASE", "ddsXXXaa", "asd", "ddsXXX");

assertStringTrim("UNICODE", "ddsXXXaa", "asd", "XXX");
assertStringTrimLeft("UNICODE", "ddsXXXaa", "asd", "XXXaa");
assertStringTrimRight("UNICODE", "ddsXXXaa", "asd", "ddsXXX");

// Test cases specific to collation type
// uppercase trim, lowercase src
assertStringTrim("UTF8_BINARY", "asd", "A", "asd");
assertStringTrim("UTF8_BINARY_LCASE", "asd", "A", "sd");
assertStringTrim("UNICODE", "asd", "A", "asd");
assertStringTrim("UNICODE_CI", "asd", "A", "sd");

// lowercase trim, uppercase src
assertStringTrim("UTF8_BINARY", "ASD", "a", "ASD");
assertStringTrim("UTF8_BINARY_LCASE", "ASD", "a", "SD");
assertStringTrim("UNICODE", "ASD", "a", "ASD");
assertStringTrim("UNICODE_CI", "ASD", "a", "SD");

// uppercase and lowercase chars of different byte-length (utf8)
assertStringTrim("UTF8_BINARY", "ẞaaaẞ", "ß", "ẞaaaẞ");
Expand All @@ -1089,10 +1069,6 @@ public void testStringTrim() throws SparkException {
assertStringTrimLeft("UTF8_BINARY_LCASE", "ẞaaaẞ", "ß", "aaaẞ");
assertStringTrimRight("UTF8_BINARY_LCASE", "ẞaaaẞ", "ß", "ẞaaa");

assertStringTrim("UNICODE", "ẞaaaẞ", "ß", "ẞaaaẞ");
assertStringTrimLeft("UNICODE", "ẞaaaẞ", "ß", "ẞaaaẞ");
assertStringTrimRight("UNICODE", "ẞaaaẞ", "ß", "ẞaaaẞ");

assertStringTrim("UTF8_BINARY", "ßaaaß", "ẞ", "ßaaaß");
assertStringTrimLeft("UTF8_BINARY", "ßaaaß", "ẞ", "ßaaaß");
assertStringTrimRight("UTF8_BINARY", "ßaaaß", "ẞ", "ßaaaß");
Expand All @@ -1101,10 +1077,6 @@ public void testStringTrim() throws SparkException {
assertStringTrimLeft("UTF8_BINARY_LCASE", "ßaaaß", "ẞ", "aaaß");
assertStringTrimRight("UTF8_BINARY_LCASE", "ßaaaß", "ẞ", "ßaaa");

assertStringTrim("UNICODE", "ßaaaß", "ẞ", "ßaaaß");
assertStringTrimLeft("UNICODE", "ßaaaß", "ẞ", "ßaaaß");
assertStringTrimRight("UNICODE", "ßaaaß", "ẞ", "ßaaaß");

// different byte-length (utf8) chars trimmed
assertStringTrim("UTF8_BINARY", "Ëaaaẞ", "Ëẞ", "aaa");
assertStringTrimLeft("UTF8_BINARY", "Ëaaaẞ", "Ëẞ", "aaaẞ");
Expand All @@ -1113,10 +1085,6 @@ public void testStringTrim() throws SparkException {
assertStringTrim("UTF8_BINARY_LCASE", "Ëaaaẞ", "Ëẞ", "aaa");
assertStringTrimLeft("UTF8_BINARY_LCASE", "Ëaaaẞ", "Ëẞ", "aaaẞ");
assertStringTrimRight("UTF8_BINARY_LCASE", "Ëaaaẞ", "Ëẞ", "Ëaaa");

assertStringTrim("UNICODE", "Ëaaaẞ", "Ëẞ", "aaa");
assertStringTrimLeft("UNICODE", "Ëaaaẞ", "Ëẞ", "aaaẞ");
assertStringTrimRight("UNICODE", "Ëaaaẞ", "Ëẞ", "Ëaaa");
}

// TODO: Test more collation-aware string expressions.
Expand Down
10 changes: 8 additions & 2 deletions common/unsafe/src/test/scala/org/apache/spark/unsafe/types/CollationFactorySuite.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class CollationFactorySuite extends AnyFunSuite with Matchers { // scalastyle:ig
assert(UNICODE_COLLATION_ID == (1 << 29))
val unicode = fetchCollation(UNICODE_COLLATION_ID)
assert(unicode.collationName == "UNICODE")
assert(unicode.supportsBinaryEquality)
assert(!unicode.supportsBinaryEquality)

assert(UNICODE_CI_COLLATION_ID == ((1 << 29) | (1 << 17)))
val unicodeCi = fetchCollation(UNICODE_CI_COLLATION_ID)
Expand Down Expand Up @@ -131,18 +131,24 @@ class CollationFactorySuite extends AnyFunSuite with Matchers { // scalastyle:ig
CollationTestCase("UTF8_BINARY", "aaa", "aaa", true),
CollationTestCase("UTF8_BINARY", "aaa", "AAA", false),
CollationTestCase("UTF8_BINARY", "aaa", "bbb", false),
CollationTestCase("UTF8_BINARY", "å", "a\u030A", false),
CollationTestCase("UTF8_BINARY_LCASE", "aaa", "aaa", true),
CollationTestCase("UTF8_BINARY_LCASE", "aaa", "AAA", true),
CollationTestCase("UTF8_BINARY_LCASE", "aaa", "AaA", true),
CollationTestCase("UTF8_BINARY_LCASE", "aaa", "AaA", true),
CollationTestCase("UTF8_BINARY_LCASE", "aaa", "aa", false),
CollationTestCase("UTF8_BINARY_LCASE", "aaa", "bbb", false),
CollationTestCase("UTF8_BINARY_LCASE", "å", "a\u030A", false),
CollationTestCase("UNICODE", "aaa", "aaa", true),
CollationTestCase("UNICODE", "aaa", "AAA", false),
CollationTestCase("UNICODE", "aaa", "bbb", false),
CollationTestCase("UNICODE", "å", "a\u030A", true),
CollationTestCase("UNICODE_CI", "aaa", "aaa", true),
CollationTestCase("UNICODE_CI", "aaa", "AAA", true),
CollationTestCase("UNICODE_CI", "aaa", "bbb", false))
CollationTestCase("UNICODE_CI", "aaa", "bbb", false),
CollationTestCase("UNICODE_CI", "å", "a\u030A", true),
CollationTestCase("UNICODE_CI", "Å", "a\u030A", true)
)

checks.foreach(testCase => {
val collation = fetchCollation(testCase.collationName)
Expand Down
8 changes: 4 additions & 4 deletions ...t/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollationExpressionSuite.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,10 +174,10 @@ class CollationExpressionSuite extends SparkFunSuite with ExpressionEvalHelper {
("aa", "UTF8_BINARY_LCASE", UTF8String.fromString("aa").getBytes),
("AA", "UTF8_BINARY_LCASE", UTF8String.fromString("aa").getBytes),
("aA", "UTF8_BINARY_LCASE", UTF8String.fromString("aa").getBytes),
("", "UNICODE", UTF8String.fromString("").getBytes),
("aa", "UNICODE", UTF8String.fromString("aa").getBytes),
("AA", "UNICODE", UTF8String.fromString("AA").getBytes),
("aA", "UNICODE", UTF8String.fromString("aA").getBytes),
("", "UNICODE", Array[Byte](1, 1, 0)),
("aa", "UNICODE", Array[Byte](42, 42, 1, 6, 1, 6, 0)),
("AA", "UNICODE", Array[Byte](42, 42, 1, 6, 1, -36, -36, 0)),
("aA", "UNICODE", Array[Byte](42, 42, 1, 6, 1, -59, -36, 0)),
("", "UNICODE_CI", Array[Byte](1, 0)),
("aa", "UNICODE_CI", Array[Byte](42, 42, 1, 6, 0)),
("AA", "UNICODE_CI", Array[Byte](42, 42, 1, 6, 0)),
Expand Down
71 changes: 35 additions & 36 deletions ...est/scala/org/apache/spark/sql/catalyst/expressions/CollationRegexpExpressionsSuite.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,10 @@ class CollationRegexpExpressionsSuite extends SparkFunSuite with ExpressionEvalH
LikeTestCase("", "", "", "UTF8_BINARY_LCASE", true, true, true),
LikeTestCase("Foo", "", "", "UTF8_BINARY_LCASE", false, false, true),
LikeTestCase("", "%foo%", ".o.", "UTF8_BINARY_LCASE", false, false, false),
LikeTestCase("AbC", "%ABC%", ".B.", "UNICODE", false, true, false),
LikeTestCase(null, "%foo%", ".o.", "UNICODE", null, null, null),
LikeTestCase("Foo", null, null, "UNICODE", null, null, null),
LikeTestCase(null, null, null, "UNICODE", null, null, null)
LikeTestCase("AbC", "%ABC%", ".B.", "UTF8_BINARY", false, true, false),
LikeTestCase(null, "%foo%", ".o.", "UTF8_BINARY", null, null, null),
LikeTestCase("Foo", null, null, "UTF8_BINARY", null, null, null),
LikeTestCase(null, null, null, "UTF8_BINARY", null, null, null)
)
testCases.foreach(t => {
// Like
Expand All @@ -62,13 +62,13 @@ class CollationRegexpExpressionsSuite extends SparkFunSuite with ExpressionEvalH
StringSplitTestCase("1A2B3C", "[abc]", "UTF8_BINARY", Seq("1A2B3C")),
StringSplitTestCase("1A2B3C", "[ABC]", "UTF8_BINARY_LCASE", Seq("1", "2", "3", "")),
StringSplitTestCase("1A2B3C", "[abc]", "UTF8_BINARY_LCASE", Seq("1", "2", "3", "")),
StringSplitTestCase("1A2B3C", "[1-9]+", "UNICODE", Seq("", "A", "B", "C")),
StringSplitTestCase("", "", "UNICODE", Seq("")),
StringSplitTestCase("1A2B3C", "", "UNICODE", Seq("1", "A", "2", "B", "3", "C")),
StringSplitTestCase("", "[1-9]+", "UNICODE", Seq("")),
StringSplitTestCase(null, "[1-9]+", "UNICODE", null),
StringSplitTestCase("1A2B3C", null, "UNICODE", null),
StringSplitTestCase(null, null, "UNICODE", null)
StringSplitTestCase("1A2B3C", "[1-9]+", "UTF8_BINARY", Seq("", "A", "B", "C")),
StringSplitTestCase("", "", "UTF8_BINARY", Seq("")),
StringSplitTestCase("1A2B3C", "", "UTF8_BINARY", Seq("1", "A", "2", "B", "3", "C")),
StringSplitTestCase("", "[1-9]+", "UTF8_BINARY", Seq("")),
StringSplitTestCase(null, "[1-9]+", "UTF8_BINARY", null),
StringSplitTestCase("1A2B3C", null, "UTF8_BINARY", null),
StringSplitTestCase(null, null, "UTF8_BINARY", null)
)
testCases.foreach(t => {
// StringSplit
Expand All @@ -89,10 +89,10 @@ class CollationRegexpExpressionsSuite extends SparkFunSuite with ExpressionEvalH
RegexpTestCase("", "", "UTF8_BINARY_LCASE", "", Seq(""), 1),
RegexpTestCase("Foo", "", "UTF8_BINARY_LCASE", "", Seq("", "", "", ""), 4),
RegexpTestCase("", ".o.", "UTF8_BINARY_LCASE", "", Seq(), 0),
RegexpTestCase("Foo", ".O.", "UNICODE", "", Seq(), 0),
RegexpTestCase(null, ".O.", "UNICODE", null, null, null),
RegexpTestCase("Foo", null, "UNICODE", null, null, null),
RegexpTestCase(null, null, "UNICODE", null, null, null)
RegexpTestCase("Foo", ".O.", "UTF8_BINARY", "", Seq(), 0),
RegexpTestCase(null, ".O.", "UTF8_BINARY", null, null, null),
RegexpTestCase("Foo", null, "UTF8_BINARY", null, null, null),
RegexpTestCase(null, null, "UTF8_BINARY", null, null, null)
)
testCases.foreach(t => {
// RegExpExtract
Expand Down Expand Up @@ -124,47 +124,46 @@ class CollationRegexpExpressionsSuite extends SparkFunSuite with ExpressionEvalH
// Supported collations (StringTypeBinaryLcase)
val binaryCollation = StringType(CollationFactory.collationNameToId("UTF8_BINARY"))
val lowercaseCollation = StringType(CollationFactory.collationNameToId("UTF8_BINARY_LCASE"))
val unicodeCollation = StringType(CollationFactory.collationNameToId("UNICODE"))
// LikeAll
checkEvaluation(Literal.create("foo", binaryCollation).likeAll("%foo%", "%oo"), true)
checkEvaluation(Literal.create("foo", binaryCollation).likeAll("%foo%", "%bar%"), false)
checkEvaluation(Literal.create("Foo", lowercaseCollation).likeAll("%foo%", "%oo"), true)
checkEvaluation(Literal.create("Foo", lowercaseCollation).likeAll("%foo%", "%bar%"), false)
checkEvaluation(Literal.create("foo", unicodeCollation).likeAll("%foo%", "%oo"), true)
checkEvaluation(Literal.create("foo", unicodeCollation).likeAll("%foo%", "%bar%"), false)
checkEvaluation(Literal.create("foo", unicodeCollation).likeAll("%foo%", nullStr), null)
checkEvaluation(Literal.create("foo", unicodeCollation).likeAll("%feo%", nullStr), false)
checkEvaluation(Literal.create(null, unicodeCollation).likeAll("%foo%", "%oo"), null)
checkEvaluation(Literal.create("foo", binaryCollation).likeAll("%foo%", "%oo"), true)
checkEvaluation(Literal.create("foo", binaryCollation).likeAll("%foo%", "%bar%"), false)
checkEvaluation(Literal.create("foo", binaryCollation).likeAll("%foo%", nullStr), null)
checkEvaluation(Literal.create("foo", binaryCollation).likeAll("%feo%", nullStr), false)
checkEvaluation(Literal.create(null, binaryCollation).likeAll("%foo%", "%oo"), null)
// NotLikeAll
checkEvaluation(Literal.create("foo", binaryCollation).notLikeAll("%foo%", "%oo"), false)
checkEvaluation(Literal.create("foo", binaryCollation).notLikeAll("%goo%", "%bar%"), true)
checkEvaluation(Literal.create("Foo", lowercaseCollation).notLikeAll("%foo%", "%oo"), false)
checkEvaluation(Literal.create("Foo", lowercaseCollation).notLikeAll("%goo%", "%bar%"), true)
checkEvaluation(Literal.create("foo", unicodeCollation).notLikeAll("%foo%", "%oo"), false)
checkEvaluation(Literal.create("foo", unicodeCollation).notLikeAll("%goo%", "%bar%"), true)
checkEvaluation(Literal.create("foo", unicodeCollation).notLikeAll("%foo%", nullStr), false)
checkEvaluation(Literal.create("foo", unicodeCollation).notLikeAll("%feo%", nullStr), null)
checkEvaluation(Literal.create(null, unicodeCollation).notLikeAll("%foo%", "%oo"), null)
checkEvaluation(Literal.create("foo", binaryCollation).notLikeAll("%foo%", "%oo"), false)
checkEvaluation(Literal.create("foo", binaryCollation).notLikeAll("%goo%", "%bar%"), true)
checkEvaluation(Literal.create("foo", binaryCollation).notLikeAll("%foo%", nullStr), false)
checkEvaluation(Literal.create("foo", binaryCollation).notLikeAll("%feo%", nullStr), null)
checkEvaluation(Literal.create(null, binaryCollation).notLikeAll("%foo%", "%oo"), null)
// LikeAny
checkEvaluation(Literal.create("foo", binaryCollation).likeAny("%goo%", "%hoo"), false)
checkEvaluation(Literal.create("foo", binaryCollation).likeAny("%foo%", "%bar%"), true)
checkEvaluation(Literal.create("Foo", lowercaseCollation).likeAny("%goo%", "%hoo"), false)
checkEvaluation(Literal.create("Foo", lowercaseCollation).likeAny("%foo%", "%bar%"), true)
checkEvaluation(Literal.create("foo", unicodeCollation).likeAny("%goo%", "%hoo"), false)
checkEvaluation(Literal.create("foo", unicodeCollation).likeAny("%foo%", "%bar%"), true)
checkEvaluation(Literal.create("foo", unicodeCollation).likeAny("%foo%", nullStr), true)
checkEvaluation(Literal.create("foo", unicodeCollation).likeAny("%feo%", nullStr), null)
checkEvaluation(Literal.create(null, unicodeCollation).likeAny("%foo%", "%oo"), null)
checkEvaluation(Literal.create("foo", binaryCollation).likeAny("%goo%", "%hoo"), false)
checkEvaluation(Literal.create("foo", binaryCollation).likeAny("%foo%", "%bar%"), true)
checkEvaluation(Literal.create("foo", binaryCollation).likeAny("%foo%", nullStr), true)
checkEvaluation(Literal.create("foo", binaryCollation).likeAny("%feo%", nullStr), null)
checkEvaluation(Literal.create(null, binaryCollation).likeAny("%foo%", "%oo"), null)
// NotLikeAny
checkEvaluation(Literal.create("foo", binaryCollation).notLikeAny("%foo%", "%hoo"), true)
checkEvaluation(Literal.create("foo", binaryCollation).notLikeAny("%foo%", "%oo%"), false)
checkEvaluation(Literal.create("Foo", lowercaseCollation).notLikeAny("%Foo%", "%hoo"), true)
checkEvaluation(Literal.create("Foo", lowercaseCollation).notLikeAny("%foo%", "%oo%"), false)
checkEvaluation(Literal.create("foo", unicodeCollation).notLikeAny("%Foo%", "%hoo"), true)
checkEvaluation(Literal.create("foo", unicodeCollation).notLikeAny("%foo%", "%oo%"), false)
checkEvaluation(Literal.create("foo", unicodeCollation).notLikeAny("%foo%", nullStr), null)
checkEvaluation(Literal.create("foo", unicodeCollation).notLikeAny("%feo%", nullStr), true)
checkEvaluation(Literal.create(null, unicodeCollation).notLikeAny("%foo%", "%oo"), null)
checkEvaluation(Literal.create("foo", binaryCollation).notLikeAny("%Foo%", "%hoo"), true)
checkEvaluation(Literal.create("foo", binaryCollation).notLikeAny("%foo%", "%oo%"), false)
checkEvaluation(Literal.create("foo", binaryCollation).notLikeAny("%foo%", nullStr), null)
checkEvaluation(Literal.create("foo", binaryCollation).notLikeAny("%feo%", nullStr), true)
checkEvaluation(Literal.create(null, binaryCollation).notLikeAny("%foo%", "%oo"), null)
}

}
Loading

0 comments on commit b5a4b32

Please sign in to comment.