Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SPARK-48440][SQL] Fix StringTranslate behaviour for non-UTF8_BINARY collations #46761

Closed
wants to merge 18 commits into from
Closed

[SPARK-48440][SQL] Fix StringTranslate behaviour for non-UTF8_BINARY collations #46761

wants to merge 18 commits into from

Conversation

uros-db
Copy link
Contributor

@uros-db uros-db commented May 27, 2024 •
edited
Loading

What changes were proposed in this pull request?

String searching in UTF8_LCASE now works on character-level, rather than on byte-level. For example: translate("İ", "i") now returns "İ", because there exists no single character in "İ" such that lowercased version of that character equals to "i". Note, however, that there is a byte subsequence of "İ" such that lowercased version of that UTF-8 byte sequence equals to "i" (so the new behaviour is different than the old behaviour).

Also, translation for ICU collations works by repeatedly translating the longest possible substring that matches a key in the dictionary (under the specified collation), starting from the left side of the input string, until the entire string is translated.

Why are the changes needed?

Fix functions that give unusable results due to one-to-many case mapping when performing string search under UTF8_BINARY_LCASE (see example above).

Does this PR introduce any user-facing change?

Yes, behaviour of translate expression is changed for edge cases with one-to-many case mapping.

How was this patch tested?

New unit tests in CollationStringExpressionsSuite.

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

No.

@github-actions github-actions bot added the SQL label May 27, 2024
@uros-db uros-db changed the title [WIP][SQL] Fix StringTranslate behaviour for non-UTF8_BINARY collations [SPARK-48440][SQL] Fix StringTranslate behaviour for non-UTF8_BINARY collations May 28, 2024
@uros-db uros-db changed the title [SPARK-48440][SQL] Fix StringTranslate behaviour for non-UTF8_BINARY collations [WIP][SPARK-48440][SQL] Fix StringTranslate behaviour for non-UTF8_BINARY collations May 28, 2024
@uros-db uros-db changed the title [WIP][SPARK-48440][SQL] Fix StringTranslate behaviour for non-UTF8_BINARY collations [SPARK-48440][SQL] Fix StringTranslate behaviour for non-UTF8_BINARY collations Jun 10, 2024
@uros-db uros-db mentioned this pull request Jun 10, 2024
Closed
cloud-fan pushed a commit that referenced this pull request Jun 11, 2024
@uros-db @cloud-fan
[SPARK-48576][SQL] Rename UTF8_BINARY_LCASE to UTF8_LCASE
aad6771 
### What changes were proposed in this pull request?
Renaming `UTF8_BINARY_LCASE` collation to `UTF8_LCASE`.

### Why are the changes needed?
As part of the collation effort in Spark, we've moved away from byte-by-byte logic towards character-by-character logic, so what we used to call `UTF8_BINARY_LCASE` is now more precisely `UTF8_LCASE`. For example, string searching in UTF8_LCASE now works on character-level (rather than on byte-level), which is reflected in this PRs: #46511, #46589, #46682, #46761, #46762. In addition, string comparison also works on character-level now, as per the changes introduced in this PR: #46700.

### Does this PR introduce _any_ user-facing change?
Yes, what was previously named `UTF8_BINARY_LCASE` collation, will from now on be named `UTF8_LCASE`.

### How was this patch tested?
Existing tests.

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

Closes #46924 from uros-db/rename-lcase.

Authored-by: Uros Bojanic <157381213+uros-db@users.noreply.github.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
@uros-db uros-db requested a review from mkaravel July 8, 2024 13:04
mkaravel
mkaravel reviewed Jul 8, 2024
View reviewed changes
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java Outdated Show resolved Hide resolved
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java Outdated Show resolved Hide resolved
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java Outdated Show resolved Hide resolved
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java Show resolved Hide resolved
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java Outdated Show resolved Hide resolved
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java Show resolved Hide resolved
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java Outdated Show resolved Hide resolved
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java Show resolved Hide resolved
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationFactory.java Outdated Show resolved Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala Outdated Show resolved Hide resolved
@uros-db uros-db requested a review from mkaravel July 9, 2024 18:24
mkaravel
mkaravel approved these changes Jul 12, 2024
View reviewed changes
Copy link
Contributor

@mkaravel mkaravel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor nit.
PR looks good to me. Thank you for working on this!

common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java Outdated Show resolved Hide resolved
@cloud-fan
Copy link
Contributor

The CI failure is unrelated, thanks, merging to master!

@cloud-fan cloud-fan closed this in 8e4bbdf Jul 12, 2024
jingz-db pushed a commit to jingz-db/spark that referenced this pull request Jul 22, 2024
@uros-db @jingz-db
[SPARK-48440][SQL] Fix StringTranslate behaviour for non-UTF8_BINARY …
d055894 
…collations

### What changes were proposed in this pull request?
String searching in UTF8_LCASE now works on character-level, rather than on byte-level. For example: `translate("İ", "i")` now returns `"İ"`, because there exists no **single character** in `"İ"` such that lowercased version of that character equals to `"i"`. Note, however, that there _is_ a byte subsequence of `"İ"` such that lowercased version of that UTF-8 byte sequence equals to `"i"` (so the new behaviour is different than the old behaviour).

Also, translation for ICU collations works by repeatedly translating the longest possible substring that matches a key in the dictionary (under the specified collation), starting from the left side of the input string, until the entire string is translated.

### Why are the changes needed?
Fix functions that give unusable results due to one-to-many case mapping when performing string search under UTF8_BINARY_LCASE (see example above).

### Does this PR introduce _any_ user-facing change?
Yes, behaviour of `translate` expression is changed for edge cases with one-to-many case mapping.

### How was this patch tested?
New unit tests in `CollationStringExpressionsSuite`.

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

Closes apache#46761 from uros-db/alter-translate.

Authored-by: Uros Bojanic <157381213+uros-db@users.noreply.github.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
attilapiros pushed a commit to attilapiros/spark that referenced this pull request Oct 4, 2024
@uros-db @attilapiros
[SPARK-48440][SQL] Fix StringTranslate behaviour for non-UTF8_BINARY …
ca38eee 
…collations

### What changes were proposed in this pull request?
String searching in UTF8_LCASE now works on character-level, rather than on byte-level. For example: `translate("İ", "i")` now returns `"İ"`, because there exists no **single character** in `"İ"` such that lowercased version of that character equals to `"i"`. Note, however, that there _is_ a byte subsequence of `"İ"` such that lowercased version of that UTF-8 byte sequence equals to `"i"` (so the new behaviour is different than the old behaviour).

Also, translation for ICU collations works by repeatedly translating the longest possible substring that matches a key in the dictionary (under the specified collation), starting from the left side of the input string, until the entire string is translated.

### Why are the changes needed?
Fix functions that give unusable results due to one-to-many case mapping when performing string search under UTF8_BINARY_LCASE (see example above).

### Does this PR introduce _any_ user-facing change?
Yes, behaviour of `translate` expression is changed for edge cases with one-to-many case mapping.

### How was this patch tested?
New unit tests in `CollationStringExpressionsSuite`.

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

Closes apache#46761 from uros-db/alter-translate.

Authored-by: Uros Bojanic <157381213+uros-db@users.noreply.github.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
himadripal pushed a commit to himadripal/spark that referenced this pull request Oct 19, 2024
@uros-db
[SPARK-48440][SQL] Fix StringTranslate behaviour for non-UTF8_BINARY …
997fa23 
…collations

### What changes were proposed in this pull request?
String searching in UTF8_LCASE now works on character-level, rather than on byte-level. For example: `translate("İ", "i")` now returns `"İ"`, because there exists no **single character** in `"İ"` such that lowercased version of that character equals to `"i"`. Note, however, that there _is_ a byte subsequence of `"İ"` such that lowercased version of that UTF-8 byte sequence equals to `"i"` (so the new behaviour is different than the old behaviour).

Also, translation for ICU collations works by repeatedly translating the longest possible substring that matches a key in the dictionary (under the specified collation), starting from the left side of the input string, until the entire string is translated.

### Why are the changes needed?
Fix functions that give unusable results due to one-to-many case mapping when performing string search under UTF8_BINARY_LCASE (see example above).

### Does this PR introduce _any_ user-facing change?
Yes, behaviour of `translate` expression is changed for edge cases with one-to-many case mapping.

### How was this patch tested?
New unit tests in `CollationStringExpressionsSuite`.

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

Closes apache#46761 from uros-db/alter-translate.

Authored-by: Uros Bojanic <157381213+uros-db@users.noreply.github.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cloud-fan cloud-fan cloud-fan approved these changes

@mkaravel mkaravel mkaravel approved these changes

Assignees
No one assigned
Labels
SQL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@uros-db @cloud-fan @mkaravel