Skip to content

[SPARK-59633][SQL] Make unary trim collation-aware for case-insensitive ICU collations #58908

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

Open
mkaravel wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[SPARK-59633][SQL] Make unary trim collation-aware for case-insensitive ICU collations #58908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@
*/
public final class CollationSupport {

private static final UTF8String DEFAULT_TRIM_STRING = UTF8String.fromString(" ");

@uros-b uros-b Sep 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reuse UTF8String.SPACE_UTF8 instead of a new DEFAULT_TRIM_STRING.

Same value, already a public constant on UTF8String.


private static boolean useCollationAwareDefaultTrim(final int collationId) {

@uros-b uros-b Sep 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Document why useCollationAwareDefaultTrim is only isCaseInsensitive.

That helper is the whole policy, and it is not obvious. A short comment should say:

CI/CI_AI ICU collations use primary/secondary strength, so UCA treats many Zs characters as equal to U+0020.
CS ICU stays binary because tertiary strength distinguishes them.
Non-ICU collations are already excluded by isCaseInsensitive.
CS_AI is not a valid trim input type.
Without that, the next change is likely to “fix” it into always-ICU (a UNICODE perf hit) or add isAccentInsensitive (dead for SQL trim).

return CollationFactory.isCaseInsensitive(collationId);
}

/**

@uros-b uros-b Sep 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: unary exec(src, collationId) could delegate to exec(src, SPACE_UTF8, collationId) for CI instead of calling execICU directly. Same result today; one dispatcher if trim collation policy changes.

* Collation-aware string expressions.
*/
Expand Down Expand Up @@ -506,6 +512,11 @@ public static class StringTrim {
public static UTF8String exec(final UTF8String srcString) {
return execBinary(srcString);
}
public static UTF8String exec(final UTF8String srcString, final int collationId) {
return useCollationAwareDefaultTrim(collationId) ?
execICU(srcString, collationId) :
execBinary(srcString);
}
public static UTF8String exec(
final UTF8String srcString,
final UTF8String trimString,
Expand All @@ -527,6 +538,13 @@ public static UTF8String exec(
public static String genCode(final String srcString) {
return String.format("CollationSupport.StringTrim.execBinary(%s)", srcString);
}
public static String genCode(final String srcString, final int collationId) {
if (useCollationAwareDefaultTrim(collationId)) {
return String.format(
"CollationSupport.StringTrim.execICU(%s, %d)", srcString, collationId);
}
return genCode(srcString);
}
public static String genCode(
final String srcString,
final String trimString,
Expand All @@ -553,6 +571,11 @@ public static UTF8String execLowercase(
final int collationId) {
return CollationAwareUTF8String.lowercaseTrim(srcString, trimString, collationId);
}
public static UTF8String execICU(
final UTF8String srcString,
final int collationId) {
return execICU(srcString, DEFAULT_TRIM_STRING, collationId);
}
public static UTF8String execICU(
final UTF8String srcString,
final UTF8String trimString,
Expand All @@ -571,6 +594,11 @@ public static class StringTrimLeft {
public static UTF8String exec(final UTF8String srcString) {
return execBinary(srcString);
}
public static UTF8String exec(final UTF8String srcString, final int collationId) {
return useCollationAwareDefaultTrim(collationId) ?
execICU(srcString, collationId) :
execBinary(srcString);
}
public static UTF8String exec(
final UTF8String srcString,
UTF8String trimString,
Expand All @@ -589,6 +617,13 @@ public static UTF8String exec(
public static String genCode(final String srcString) {
return String.format("CollationSupport.StringTrimLeft.execBinary(%s)", srcString);
}
public static String genCode(final String srcString, final int collationId) {
if (useCollationAwareDefaultTrim(collationId)) {
return String.format(
"CollationSupport.StringTrimLeft.execICU(%s, %d)", srcString, collationId);
}
return genCode(srcString);
}
public static String genCode(
final String srcString,
final String trimString,
Expand All @@ -613,6 +648,11 @@ public static UTF8String execLowercase(
final UTF8String trimString) {
return CollationAwareUTF8String.lowercaseTrimLeft(srcString, trimString);
}
public static UTF8String execICU(
final UTF8String srcString,
final int collationId) {
return execICU(srcString, DEFAULT_TRIM_STRING, collationId);
}
public static UTF8String execICU(
final UTF8String srcString,
final UTF8String trimString,
Expand All @@ -625,6 +665,11 @@ public static class StringTrimRight {
public static UTF8String exec(final UTF8String srcString) {
return execBinary(srcString);
}
public static UTF8String exec(final UTF8String srcString, final int collationId) {
return useCollationAwareDefaultTrim(collationId) ?
execICU(srcString, collationId) :
execBinary(srcString);
}
public static UTF8String exec(
final UTF8String srcString,
final UTF8String trimString,
Expand All @@ -646,6 +691,13 @@ public static UTF8String exec(
public static String genCode(final String srcString) {
return String.format("CollationSupport.StringTrimRight.execBinary(%s)", srcString);
}
public static String genCode(final String srcString, final int collationId) {
if (useCollationAwareDefaultTrim(collationId)) {
return String.format(
"CollationSupport.StringTrimRight.execICU(%s, %d)", srcString, collationId);
}
return genCode(srcString);
}
public static String genCode(
final String srcString,
final String trimString,
Expand All @@ -671,6 +723,11 @@ public static UTF8String execLowercase(
final int collationId) {
return CollationAwareUTF8String.lowercaseTrimRight(srcString, trimString, collationId);
}
public static UTF8String execICU(
final UTF8String srcString,
final int collationId) {
return execICU(srcString, DEFAULT_TRIM_STRING, collationId);
}
public static UTF8String execICU(
final UTF8String srcString,
final UTF8String trimString,
Expand Down
100 changes: 95 additions & 5 deletions common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2757,11 +2757,11 @@ private void assertStringTrim(String collationName, String sourceString, String

@uros-b uros-b Sep 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

assertStringTrimLeft / assertStringTrimRight still call the old 1-arg exec(src).

The PR updated assertStringTrim to pass collationId when the trim string is the default space, but Left/Right helpers still hit the binary-only overloads. Existing testStringTrimLeft / testStringTrimRight default-space cases therefore do not exercise the new product path. After this lands, those 1-arg methods are a footgun: they look like the public API but no longer match StringTrimLeft.doEval.

if (trimString == null) {
// Trim string is ASCII space.
result = CollationSupport.StringTrim.exec(src);
UTF8String trimLeft = CollationSupport.StringTrimLeft.exec(src);
resultTrimLeftRight = CollationSupport.StringTrimRight.exec(trimLeft);
UTF8String trimRight = CollationSupport.StringTrimRight.exec(src);
resultTrimRightLeft = CollationSupport.StringTrimLeft.exec(trimRight);
result = CollationSupport.StringTrim.exec(src, collationId);
UTF8String trimLeft = CollationSupport.StringTrimLeft.exec(src, collationId);
resultTrimLeftRight = CollationSupport.StringTrimRight.exec(trimLeft, collationId);
UTF8String trimRight = CollationSupport.StringTrimRight.exec(src, collationId);
resultTrimRightLeft = CollationSupport.StringTrimLeft.exec(trimRight, collationId);
} else {
// Trim string is specified.
result = CollationSupport.StringTrim.exec(src, trim, collationId);
Expand All @@ -2778,6 +2778,96 @@ private void assertStringTrim(String collationName, String sourceString, String
assertEquals(resultTrimRightLeft, result);
}

private void assertDefaultStringTrims(
String collationName,
String sourceString,
String expectedLeft,
String expectedRight,
String expectedBoth) throws SparkException {
int collationId = CollationFactory.collationNameToId(collationName);
UTF8String source = UTF8String.fromString(sourceString);
UTF8String defaultTrimString = UTF8String.fromString(" ");

UTF8String trimLeft = CollationSupport.StringTrimLeft.exec(source, collationId);
UTF8String trimRight = CollationSupport.StringTrimRight.exec(source, collationId);
UTF8String trimBoth = CollationSupport.StringTrim.exec(source, collationId);

assertEquals(UTF8String.fromString(expectedLeft), trimLeft);
assertEquals(UTF8String.fromString(expectedRight), trimRight);
assertEquals(UTF8String.fromString(expectedBoth), trimBoth);
assertEquals(
CollationSupport.StringTrimLeft.exec(source, defaultTrimString, collationId), trimLeft);
assertEquals(
CollationSupport.StringTrimRight.exec(source, defaultTrimString, collationId), trimRight);
assertEquals(
CollationSupport.StringTrim.exec(source, defaultTrimString, collationId), trimBoth);
}

@Test
public void testDefaultStringTrimsUseCollation() throws SparkException {
String[] spaceSeparators = {
"\u00A0", "\u1680", "\u2000", "\u2001", "\u2002", "\u2003", "\u2004", "\u2005",
"\u2006", "\u2007", "\u2008", "\u2009", "\u200A", "\u202F", "\u205F", "\u3000"
};
String[] unaffectedCollations = {
UTF8_BINARY,
"UTF8_BINARY_RTRIM",
UTF8_LCASE,
"UTF8_LCASE_RTRIM",
UNICODE,
"UNICODE_RTRIM"
};
String[] affectedCollations = {
UNICODE_CI,
"UNICODE_CI_RTRIM",
"UNICODE_CI_AI",
"UNICODE_CI_AI_RTRIM"
};

for (String collation : unaffectedCollations) {
int collationId = CollationFactory.collationNameToId(collation);
assertEquals(
"CollationSupport.StringTrim.execBinary(source)",
CollationSupport.StringTrim.genCode("source", collationId));
assertEquals(
"CollationSupport.StringTrimLeft.execBinary(source)",
CollationSupport.StringTrimLeft.genCode("source", collationId));
assertEquals(
"CollationSupport.StringTrimRight.execBinary(source)",
CollationSupport.StringTrimRight.genCode("source", collationId));
}
for (String collation : affectedCollations) {
int collationId = CollationFactory.collationNameToId(collation);
assertEquals(
String.format("CollationSupport.StringTrim.execICU(source, %d)", collationId),
CollationSupport.StringTrim.genCode("source", collationId));
assertEquals(
String.format("CollationSupport.StringTrimLeft.execICU(source, %d)", collationId),
CollationSupport.StringTrimLeft.genCode("source", collationId));
assertEquals(
String.format("CollationSupport.StringTrimRight.execICU(source, %d)", collationId),
CollationSupport.StringTrimRight.genCode("source", collationId));
}

for (String separator : spaceSeparators) {
String source = separator + "abc" + separator;
for (String collation : unaffectedCollations) {
assertDefaultStringTrims(collation, source, source, source, source);
}
for (String collation : affectedCollations) {
assertDefaultStringTrims(
collation, source, "abc" + separator, separator + "abc", "abc");
}
}

for (String collation : affectedCollations) {
assertDefaultStringTrims(collation, " abc ", "abc ", " abc", "abc");
assertDefaultStringTrims(collation, "\tabc\t", "\tabc\t", "\tabc\t", "\tabc\t");
assertDefaultStringTrims(
collation, "\u200Babc\u200B", "\u200Babc\u200B", "\u200Babc\u200B", "\u200Babc\u200B");
}
}

@uros-b uros-b Sep 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add the JIRA reproduction as a SQL test.

collations-padding-trim.sql already covers two-arg TRIM/LTRIM/RTRIM and has no unary TRIM(col COLLATE UNICODE_CI) case. A golden SQL test for

trim(concat(chr(160), 'abc', chr(160)) COLLATE UNICODE_CI)
versus the explicit ' ' form is what will catch a parser/analysis/codegen miss that the Java helper tests will not.


@uros-b uros-b Sep 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Test gaps - the Catalyst test only checks NBSP on UNICODE_CI vs UNICODE. Worth adding, still in checkEvaluation:

  • unary vs StringTrim(src, Literal(" ")) at the expression level (the Java suite does this, Catalyst does not)
  • mixed padding, e.g. "\u00A0 abc \u00A0" → "abc"
  • a string of only NBSP → empty
  • en_CI / other locale CI names are covered by the helper, but only UNICODE_* names are tested

@Test
public void testStringTrim() throws SparkException {
// Basic tests.
Expand Down
12 changes: 6 additions & 6 deletions ...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1490,11 +1490,11 @@ trait String2TrimExpression extends Expression with ImplicitCastInputTypes {
if (evals.length == 1) {
val stringTrimCode: String = this match {
case _: StringTrim =>
CollationSupport.StringTrim.genCode(srcString.value)
CollationSupport.StringTrim.genCode(srcString.value, collationId)
case _: StringTrimLeft =>
CollationSupport.StringTrimLeft.genCode(srcString.value)
CollationSupport.StringTrimLeft.genCode(srcString.value, collationId)
case _: StringTrimRight =>
CollationSupport.StringTrimRight.genCode(srcString.value)
CollationSupport.StringTrimRight.genCode(srcString.value, collationId)
}
ev.copy(code = code"""
|${srcString.code}
Expand Down Expand Up @@ -1622,7 +1622,7 @@ case class StringTrim(srcStr: Expression, trimStr: Option[Expression] = None)
override protected def direction: String = "BOTH"

override def doEval(srcString: UTF8String): UTF8String =
CollationSupport.StringTrim.exec(srcString)
CollationSupport.StringTrim.exec(srcString, collationId)

override def doEval(srcString: UTF8String, trimString: UTF8String): UTF8String =
CollationSupport.StringTrim.exec(srcString, trimString, collationId)
Expand Down Expand Up @@ -1739,7 +1739,7 @@ case class StringTrimLeft(srcStr: Expression, trimStr: Option[Expression] = None
override protected def direction: String = "LEADING"

override def doEval(srcString: UTF8String): UTF8String =
CollationSupport.StringTrimLeft.exec(srcString)
CollationSupport.StringTrimLeft.exec(srcString, collationId)

override def doEval(srcString: UTF8String, trimString: UTF8String): UTF8String =
CollationSupport.StringTrimLeft.exec(srcString, trimString, collationId)
Expand Down Expand Up @@ -1807,7 +1807,7 @@ case class StringTrimRight(srcStr: Expression, trimStr: Option[Expression] = Non
override protected def direction: String = "TRAILING"

override def doEval(srcString: UTF8String): UTF8String =
CollationSupport.StringTrimRight.exec(srcString)
CollationSupport.StringTrimRight.exec(srcString, collationId)

override def doEval(srcString: UTF8String, trimString: UTF8String): UTF8String =
CollationSupport.StringTrimRight.exec(srcString, trimString, collationId)
Expand Down
17 changes: 17 additions & 0 deletions ...yst/src/test/scala/org/apache/spark/sql/catalyst/expressions/StringExpressionsSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import org.apache.spark.sql.catalyst.dsl.expressions._
import org.apache.spark.sql.catalyst.expressions.Cast._
import org.apache.spark.sql.catalyst.expressions.codegen.GenerateUnsafeProjection
import org.apache.spark.sql.catalyst.util.CharsetProvider
import org.apache.spark.sql.catalyst.util.CollationFactory
import org.apache.spark.sql.errors.QueryExecutionErrors.toSQLId
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.types.StringTypeWithCollation
Expand Down Expand Up @@ -1060,6 +1061,22 @@ class StringExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
checkEvaluation(StringTrim(Literal("xxxbarxxx"), Literal("x")), "bar")
}

test("default trim uses collation-aware space matching") {
val unicodeCi = CollationFactory.collationNameToId("UNICODE_CI")
val unicode = CollationFactory.collationNameToId("UNICODE")
val source = "\u00A0abc\u00A0"
val unicodeCiSource = Literal.create(source, StringType(unicodeCi))
val unicodeSource = Literal.create(source, StringType(unicode))

checkEvaluation(StringTrimLeft(unicodeCiSource), "abc\u00A0")
checkEvaluation(StringTrimRight(unicodeCiSource), "\u00A0abc")
checkEvaluation(StringTrim(unicodeCiSource), "abc")

checkEvaluation(StringTrimLeft(unicodeSource), source)
checkEvaluation(StringTrimRight(unicodeSource), source)
checkEvaluation(StringTrim(unicodeSource), source)
}

test("LTRIM") {
val s = $"a".string.at(0)
checkEvaluation(StringTrimLeft(Literal(" aa ")), "aa ", create_row(" abdef "))
Expand Down