uros-b commented on code in PR #58908:
URL: https://github.com/apache/spark/pull/58908#discussion_r4047559407
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -30,6 +30,12 @@
*/
public final class CollationSupport {
+ private static final UTF8String DEFAULT_TRIM_STRING =
UTF8String.fromString(" ");
+
+ private static boolean useCollationAwareDefaultTrim(final int collationId) {
Review Comment:
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).
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -2757,11 +2757,11 @@ private void assertStringTrim(String collationName,
String sourceString, String
Review Comment:
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.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -30,6 +30,12 @@
*/
public final class CollationSupport {
+ private static final UTF8String DEFAULT_TRIM_STRING =
UTF8String.fromString(" ");
+
+ private static boolean useCollationAwareDefaultTrim(final int collationId) {
+ return CollationFactory.isCaseInsensitive(collationId);
+ }
+
/**
Review Comment:
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.
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -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");
+ }
+ }
+
Review Comment:
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
##########
common/unsafe/src/test/java/org/apache/spark/unsafe/types/CollationSupportSuite.java:
##########
@@ -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");
+ }
+ }
Review Comment:
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.
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -30,6 +30,12 @@
*/
public final class CollationSupport {
+ private static final UTF8String DEFAULT_TRIM_STRING =
UTF8String.fromString(" ");
Review Comment:
Reuse UTF8String.SPACE_UTF8 instead of a new DEFAULT_TRIM_STRING.
Same value, already a public constant on UTF8String.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
