Skip to content

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

Open
mkaravel wants to merge 1 commit into
apache:masterfrom
mkaravel:fix-unary-trim-icu-collations
Open

mkaravel wants to merge 1 commit into
apache:masterfrom
mkaravel:fix-unary-trim-icu-collations

Conversation

@mkaravel

@mkaravel mkaravel commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

This PR makes unary trim, ltrim, and rtrim use collation-aware matching for
case-insensitive ICU collations.

The expressions now pass their collation ID to CollationSupport. Interpreted execution
dispatches affected collations to the existing ICU implementation with an ASCII-space trim
string. Code generation selects either the ICU or binary implementation while generating code,
avoiding per-row collation dispatch in generated code.

Binary, lowercase, and case-sensitive ICU collations continue to use the existing binary
implementation.

Why are the changes needed?

Unary trimming currently removes only literal ASCII space, regardless of the input collation.
The explicit two-argument forms use ICU matching for ICU-collated operands. As a result, the
forms disagree under case-insensitive ICU collations, where Unicode space separators such as
NBSP compare as equivalent to ASCII space.

For example, these expressions currently produce different results:

WITH input AS (SELECT concat(chr(160), 'abc', chr(160)) AS value)
SELECT trim(value COLLATE UNICODE_CI),
       trim(value COLLATE UNICODE_CI, ' ' COLLATE UNICODE_CI)
FROM input;

The unary form preserves both NBSP characters while the explicit form returns abc. Both forms
should apply the same default trim string with the same collation semantics.

Does this PR introduce any user-facing change?

Yes. Unary trim, ltrim, and rtrim now remove characters that compare as equivalent to the
default ASCII-space trim string under case-insensitive ICU collations, matching their explicit
two-argument forms. Behavior is unchanged for other collations.

How was this patch tested?

Added tests covering:

  • Unary and explicit trim equivalence for every Unicode space-separator (Zs) character.
  • UNICODE_CI, UNICODE_CI_AI, and their _RTRIM variants.
  • Unchanged behavior for binary, lowercase, and case-sensitive ICU collations.
  • Negative cases for tab and zero-width space.
  • Interpreted and code-generated Catalyst execution.

Ran locally:

./build/sbt 'unsafe/testOnly org.apache.spark.unsafe.types.CollationSupportSuite -- *testDefaultStringTrimsUseCollation*'
./build/sbt 'catalyst/testOnly org.apache.spark.sql.catalyst.expressions.StringExpressionsSuite -- -z "default trim uses collation-aware space matching"'

Both passed. CollationSupportSuite#testDefaultStringTrimsUseCollation is a JUnit test, so it is
selected with a method glob rather than ScalaTest -z. The Catalyst test
default trim uses collation-aware space matching covers interpreted and generated code.

The fork's Build and test and Report test results workflows are enabled for CI validation.

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

Generated-by: Cursor Agent (Auto, 2026-09-18)

@mkaravel mkaravel changed the title [SQL] Make unary trim collation-aware for case-insensitive ICU collations [SPARK-59633][SQL] Make unary trim collation-aware for case-insensitive ICU collations Sep 18, 2026
@mkaravel
mkaravel marked this pull request as ready for review September 18, 2026 13:13

@uros-b uros-b left a comment

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.

Thank you @mkaravel, leaving a few comments below.


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

private static boolean useCollationAwareDefaultTrim(final int collationId) {

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

@@ -2757,11 +2757,11 @@ private void assertStringTrim(String collationName, String sourceString, String

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.

*/
public final class CollationSupport {

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

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.

collation, "\u200Babc\u200B", "\u200Babc\u200B", "\u200Babc\u200B", "\u200Babc\u200B");
}
}

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

return CollationFactory.isCaseInsensitive(collationId);
}

/**

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.

assertDefaultStringTrims(
collation, "\u200Babc\u200B", "\u200Babc\u200B", "\u200Babc\u200B", "\u200Babc\u200B");
}
}

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants