[Deprecated][SQL] Add support for collation for StringTrim type of functions/expressions by davidm-db · Pull Request #45749 · apache/spark

davidm-db · 2024-03-28T09:56:17Z

Left to do/discussion topics (to be removed by the PR completion)

Handling of a lowercase (utf8_binary_lcase) collation type seems a bit grueling in the long run. This PR handles it the same way as in every other place, since I think it is out of the scope of this PR, but should we maybe create a work item to generalize this collation type better (if we don't have it already)?

What changes were proposed in this pull request?

This PR is created to add support for collations to StringTrim family of functions/expressions, specifically:

StringTrim
StringTrimBoth
StringTrimLeft
StringTrimRight

Changes:

stringExpressions.scala
- Check input types to be of the same collation.
- Route to proper endpoints in UTF8String - doEval and doGenCode.
UTF8String.java
- Implement logic for all trim functions (logic should be well commented-out in the code and mostly matches already existing implementations of trimLeft and trimRight).

Other minor changes in this PR:

Change parameter names in CollationFactory.getStringSearch() to a more descriptive ones.

Why are the changes needed?

We are incrementally adding collation support to a built-in string functions in Spark.

Does this PR introduce any user-facing change?

Yes:

User should now be able to use non-default collations in string trim functions.
User will receive DATATYPE_MISMATCH.COLLATION_MISMATCH exception if collations of two function parameters do not match.

How was this patch tested?

Already existing tests + new unit/e2e tests.

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

No.

mihailomilosevic2001 · 2024-03-28T10:29:05Z

Also, please add tests to CollationStringExpressionSuite

davidm-db · 2024-03-28T10:34:36Z

@mihailom-db I'm adding tests to CollationStringExpressionSuite - I just pushed the changes so they can be reviewed while I'm adding the tests :)

davidm-db · 2024-03-29T11:22:21Z

@MaxGekk, @cloud-fan could someone please take a look at this PR?

Tagging the rest of Belgrade collation crew if anyone else would like to review additionally: @dbatomic, @nikolamand-db, @stefankandic, @uros-db

uros-db · 2024-04-01T13:05:38Z

+    return collatedTrimLeft(trimString, collationId);
+  }
+
+  private UTF8String lowercaseTrimLeft(UTF8String trimString) {


Instead of implementing lowercaseTrimLeft and collatedTrimLeft separately (these functions look very similar to me), I think we could make use of new StringSearch(pattern, target) (with .toLowerCase() for both params, and no collationId for UTF8_BINARY_LCASE)

For more context, please take a look at: https://github.com/apache/spark/pull/45704/files#r1538624688

uros-db · 2024-04-01T13:20:39Z

+      if (evals.length == 1) {
+        ev.copy(code = code"""
+          |${srcString.code}
+          |boolean ${ev.isNull} = false;
+          |UTF8String ${ev.value} = null;
+          |if (${srcString.isNull}) {
+          |  ${ev.isNull} = true;
+          |} else {
+          |  ${ev.value} = ${srcString.value}.$trimMethod($collationId);
+          |}""".stripMargin)
+      } else {
+        val trimString = evals(1)
+        ev.copy(code = code"""
+          |${srcString.code}
+          |boolean ${ev.isNull} = false;
+          |UTF8String ${ev.value} = null;
+          |if (${srcString.isNull}) {
+          |  ${ev.isNull} = true;
+          |} else {
+          |  ${trimString.code}
+          |  if (${trimString.isNull}) {
+          |    ${ev.isNull} = true;
+          |  } else {
+          |    ${ev.value} =
+          |      ${srcString.value}.$trimMethod(${trimString.value}, $collationId);
+          |  }
+          |}""".stripMargin)
+      }


I think this code is very similar to the one above, consider rewriting it a bit more neatly in a way that incorporates $collationId

stefankandic · 2024-04-01T14:58:52Z

+        trimByteIdx -= stringCharLen[numChars - 1];
+        numChars--;
+      }
+      else {


nit: in jvm languages else is almost always on the same line as the closing brace of it's if

stefankandic · 2024-04-01T15:01:14Z

+    return copyUTF8String(trimByteIdx, numBytes - 1);
+  }
+
+  private UTF8String collatedTrimLeft(UTF8String trimString, int collationId) {


@uros-db I see a number of PRs from different people adding similar functionality so just wondering if you can start a discussion to standardize the names of these kinds of methods because I've seen multiple variants

my vote would be for collationAware...

collationAwareFunctionName sounds good to me, we can discuss consolidating the naming for all functions

uros-db · 2024-04-11T14:23:25Z

heads up: we’ve done some major code restructuring in #45978, so please sync these changes before moving on

@davidm-db you’ll likely need to rewrite the code in this PR, so please follow the guidelines outlined in https://issues.apache.org/jira/browse/SPARK-47410

mihailomilosevic2001 · 2024-04-15T07:49:30Z

Please add these expressions to CollationTypeCasts rules.

uros-db

please change the PR title so we can remove it from the corresponding ticket in favor of your new PR

…unctions/expressions (for UTF8_BINARY & LCASE) Recreating [original PR](#45749) because code has been reorganized in [this PR](#45978). ### What changes were proposed in this pull request? This PR is created to add support for collations to StringTrim family of functions/expressions, specifically: - `StringTrim` - `StringTrimBoth` - `StringTrimLeft` - `StringTrimRight` Changes: - `CollationSupport.java` - Add new `StringTrim`, `StringTrimLeft` and `StringTrimRight` classes with corresponding logic. - `CollationAwareUTF8String` - add new `trim`, `trimLeft` and `trimRight` methods that actually implement trim logic. - `UTF8String.java` - expose some of the methods publicly. - `stringExpressions.scala` - Change input types. - Change eval and code gen logic. - `CollationTypeCasts.scala` - add `StringTrim*` expressions to `CollationTypeCasts` rules. ### Why are the changes needed? We are incrementally adding collation support to a built-in string functions in Spark. ### Does this PR introduce _any_ user-facing change? Yes: - User should now be able to use non-default collations in string trim functions. ### How was this patch tested? Already existing tests + new unit/e2e tests. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #46206 from davidm-db/string-trim-functions. Authored-by: David Milicevic <david.milicevic@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

davidm-db added 3 commits March 27, 2024 16:57

Add all code paths for collation support to TRIM functions

0a32eac

Implement trim functions that support collations

ce8253e

Change param names for getStringSearch to a more descriptive ones

095748f

github-actions Bot added the SQL label Mar 28, 2024

mihailomilosevic2001 reviewed Mar 28, 2024

View reviewed changes

Comment thread common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationFactory.java Outdated

mihailomilosevic2001 reviewed Mar 28, 2024

View reviewed changes

Comment thread common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java Outdated

davidm-db added 2 commits March 28, 2024 18:33

Tests + revert file reformatting

804493a

Add doc comments

4c03363

uros-db reviewed Apr 1, 2024

View reviewed changes

Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala Outdated

uros-db reviewed Apr 1, 2024

View reviewed changes

Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala Outdated

uros-db reviewed Apr 1, 2024

View reviewed changes

Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala Outdated

uros-db reviewed Apr 1, 2024

View reviewed changes

Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala Outdated

davidm-db added 2 commits April 1, 2024 16:36

Revert unnecessary changes

edfa729

Simplify code gen

9dc8f39

stefankandic reviewed Apr 1, 2024

View reviewed changes

Comment thread common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java

stefankandic reviewed Apr 1, 2024

View reviewed changes

Comment thread common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java

This was referenced Apr 1, 2024

[SPARK-47566][SQL] Support SubstringIndex function to work with collated strings #45725

Closed

[SPARK-47567][SQL] Support LOCATE function to work with collated strings #45791

Closed

Addressing comments

337878f

davidm-db closed this Apr 24, 2024

davidm-db mentioned this pull request Apr 24, 2024

[SPARK-47409][SQL] Add support for collation for StringTrim type of functions/expressions (for UTF8_BINARY & LCASE) #46206

Closed

uros-db reviewed Apr 30, 2024

View reviewed changes

davidm-db changed the title ~~[SPARK-47409][SQL] Add support for collation for StringTrim type of functions/expressions~~ [Deprecated][SQL] Add support for collation for StringTrim type of functions/expressions Apr 30, 2024

Conversation

davidm-db commented Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Left to do/discussion topics (to be removed by the PR completion)

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

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

Uh oh!

Uh oh!

Uh oh!

mihailomilosevic2001 commented Mar 28, 2024

Uh oh!

davidm-db commented Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidm-db commented Mar 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

uros-db Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

uros-db Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stefankandic Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

stefankandic Apr 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uros-db Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

uros-db commented Apr 11, 2024

Uh oh!

mihailomilosevic2001 commented Apr 15, 2024

Uh oh!

uros-db left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

davidm-db commented Mar 28, 2024 •

edited

Loading

davidm-db commented Mar 28, 2024 •

edited

Loading

davidm-db commented Mar 29, 2024 •

edited

Loading

stefankandic Apr 1, 2024 •

edited

Loading