[SPARK-47296][SQL][COLLATION] Fail unsupported functions for non-binary collations by uros-db · Pull Request #45422 · apache/spark

uros-db · 2024-03-07T16:36:21Z

What changes were proposed in this pull request?

Why are the changes needed?

Currently, all StringType arguments passed to built-in string functions in Spark SQL get treated as binary strings. This behaviour is incorrect for almost all collationIds except the default (0), and we should instead warn the user if they try to use an unsupported collation for the given function. Over time, we should implement the appropriate support for these (function, collation) pairs, but until then - we should have a way to fail unsupported statements in query analysis.

Does this PR introduce any user-facing change?

Yes, users will now get appropriate errors when they try to use an unsupported collation with a given string function.

How was this patch tested?

Tests in CollationSuite to check if these functions work for binary collations and throw exceptions for others.

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

Yes.

cloud-fan · 2024-03-12T12:38:14Z

Without updating StringType.acceptsType, I'm not confident to find out all functions that expect StringType but do not support collation.

uros-db · 2024-03-12T12:55:49Z

@cloud-fan yes, that is a problem... should we settle only on string functions for now? I think these functions that are meant to work with Strings are more sensitive to this error

on a more important note, even if we were to update StringType.acceptsType, it would still not solve the problem of collation support level (introduced in this PR) that would prevent passing correctly matched arguments to a function that simply does not (yet) support that particular collation - we will be needing this much in the near future, and while we're at it - overriding checkInputTypes seems to solve both

while type coercion is a separate effort, and will probably cover other parts of the codebase, what do we think about implementing this for now? @dbatomic

a bit more context for readers: for now, everything in the codebase that supports StringType will take StringType(#) (any collation) and treat it as the default collation (UTF8_BINARY); this is especially problematic for string functions that essentially return incorrect results without warning

cloud-fan · 2024-03-12T13:51:26Z

I don't think it's safe to only handle expressions in regexpExpressions.scala. For example, Substring is not there. I don't know how to collect all functions that take StringType, unless we check all expressions one by one.

I'd prefer only updating functions that support collation to have more fine-grained collation check, which shouldn't be many right now.

uros-db · 2024-03-13T13:21:28Z

@cloud-fan that makes a lot of sense, now new case classes should handle this:

StringType no longer accepts all collationIds, but only the default collationId (0) - i.e. UTF8_BINARY
StringTypeBinary is added to allow binary collations (for now: UTF8_BINARY and UNICODE), but at this time we need this for full lockdown because casting is not ready (casting is a separate effort, and when it's done, we can have StringType accept all binary collations directly; for now, it's incorrect)
StringTypeBinaryLcase is added to allow binary & lowercase (UTF8_BINARY_LCASE, UTF8_BINARY, UNICODE) - this class is important because some expressions will support binary & lowercase, but not other collations at a given time
StringTypeAllCollations is added to allow all collations (for now this is supported only in StringPredicate expressions: Contains, StartsWith, EndsWith) - note that these expressions handle all collations, but can't guarantee that all string arguments have exactly the same collation type, so we still need checkCollationCompatibility in CollationTypeConstraints; once casting is ready, we will delete this

cloud-fan

mostly LGTM

dbatomic · 2024-03-15T12:08:50Z

   * equality and hashing).
   */
  def isBinaryCollation: Boolean = CollationFactory.fetchCollation(collationId).isBinaryCollation
+  def isLowercaseCollation: Boolean = collationId == CollationFactory.LOWERCASE_COLLATION_ID


Can you remove even this guy and push the check into StringTypeBinaryLcase?

I am not sure this is possible. StringTypeBinaryLcase does not extend StringType, and the point of this function for now is to call it on StringType object in acceptsType to check if we should let the function proceed with that input.

dbatomic · 2024-03-15T13:18:54Z

LGTM

As a follow up we should revisit error messages. IMO it is weird to expose message with "string_any_collation" type to customer. But I think that we can do that as a follow up.

cloud-fan · 2024-03-15T14:08:26Z

+      case (st: StringType, _: StringTypeCollated) => st
      // Cast any atomic type to string.
      case (any: AtomicType, StringType) if any != StringType => StringType
+      case (any: AtomicType, _: StringTypeCollated) if any != StringType => StringType


The code can be more readable if we call StringTypeCollated#defaultConcreteType, which is StringType(0)

Is this what you meant?

cloud-fan · 2024-03-19T16:00:56Z


+      // If a function expects a StringType, no StringType instance should be implicitly cast to
+      // StringType with a collation that's not accepted (aka. lockdown unsupported collations).
+      case (StringType, StringType) => None


isn't this case match covered by the first case match case _ if expectedType.acceptsType(inType) => Some(inType)?

I think this should be case (_: StringType, StringType) ...

cloud-fan · 2024-03-19T16:02:32Z


+      // "canANSIStoreAssign" doesn't account for targets extending StringTypeCollated, but
+      // ANSIStoreAssign is generally expected to return "true" for (AtomicType, StringType)
+      case (_: AtomicType, _: StringTypeCollated) => Some(StringType)


is this correct? StringType does not satisfy StringTypeCollated

Yes it is. canANSIStoreAssign has a rule for casting AtomicType to StringType, but since StringTypeCollated does not extend StringType, but only AbstractDataType, this cast rule will not be picked up. But I would say this rule has to be improved to check for all canANsiStoreAssign rules.

cloud-fan · 2024-03-19T16:03:21Z

      case (StringType, datetime: DatetimeType) => datetime
      case (StringType, AnyTimestampType) => AnyTimestampType.defaultConcreteType
      case (StringType, BinaryType) => BinaryType
+      case (st: StringType, StringType) => st


reading the code around here, I think null means no implicit cast.

cloud-fan · 2024-03-19T16:06:57Z

+      // If a function expects a StringType, no StringType instance should be implicitly cast to
+      // StringType with a collation that's not accepted (aka. lockdown unsupported collations).
+      case (StringType, StringType) => None
+      case (StringType, _: StringTypeCollated) => None


This case should be put before case (StringType, a: AtomicType) =>, otherwise it's useless

cloud-fan · 2024-03-19T16:07:44Z

      case (StringType, AnyTimestampType) => AnyTimestampType.defaultConcreteType
      case (StringType, BinaryType) => BinaryType
+      case (st: StringType, StringType) => st
+      case (st: StringType, _: StringTypeCollated) => st


I think this will be covered by the last default case match?

Yes and no. The following two lines would have made a cast, but I changed them so they doesn't.

mihailomilosevic2001 · 2024-03-19T20:50:17Z

LGTM

As a follow up we should revisit error messages. IMO it is weird to expose message with "string_any_collation" type to customer. But I think that we can do that as a follow up.

Could this be an onboarding task?

cloud-fan · 2024-03-20T14:57:12Z

thanks, merging to master!

…ypeCollated ### What changes were proposed in this pull request? Renaming simpleString in StringTypeAnyCollation. This PR should only be merged after #45383 is merged. ### Why are the changes needed? [SPARK-47296](#45422) introduced a change to fail all unsupported functions. Because of this change expected inputTypes in ExpectsInputTypes had to be changed. This change introduced a change on user side which will print "STRING_ANY_COLLATION" in places where before we printed "STRING" when an error occurred. Concretely if we get an input of Int where StringTypeAnyCollation was expected, we will throw this faulty message for users. ### Does this PR introduce _any_ user-facing change? Yes ### How was this patch tested? Existing tests were changed back to "STRING" notation. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #45694 from mihailom-db/SPARK-47504. Authored-by: Mihailo Milosevic <mihailo.milosevic@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

Fail all 10 regexp expressions for non-binary collations

e0485fc

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

dbatomic reviewed Mar 7, 2024

View reviewed changes

HyukjinKwon reviewed Mar 8, 2024

View reviewed changes

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

Reformat code

355eee9

MaxGekk requested changes Mar 9, 2024

View reviewed changes

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

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

Refactoring and better test coverage

cde9eff

uros-db requested review from MaxGekk and dbatomic March 12, 2024 12:12

uros-db added 5 commits March 13, 2024 09:08

Lockdown using StringType

616335b

Merge branch 'apache:master' into regexp-functions

e6bec45

Lockdown for regexpExpressions

77e606c

Renaming and small fixes

ee72ed3

Code style fixes

8bc10f5

uros-db requested a review from HyukjinKwon March 13, 2024 13:23

uros-db added 6 commits March 13, 2024 15:02

Move StringType lockdown condition to case object

8ee0f58

Fix checkInputDataTypes

d3a9e70

Implicit cast for new StringTypes

c11c458

Implicit cast for new StringTypes

4778951

Separate suite for stringExpressions

1582f24

Collation test fix

8d75846

cloud-fan reviewed Mar 15, 2024

View reviewed changes

Comment thread sql/api/src/main/scala/org/apache/spark/sql/types/StringType.scala Outdated

cloud-fan reviewed Mar 15, 2024

View reviewed changes

Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala Outdated

cloud-fan approved these changes Mar 15, 2024

View reviewed changes

uros-db added 2 commits March 15, 2024 09:33

Reformat

63caef6

Remove extra changes

38c3906

uros-db requested a review from cloud-fan March 15, 2024 10:51

dbatomic reviewed Mar 15, 2024

View reviewed changes

dbatomic approved these changes Mar 15, 2024

View reviewed changes

Fix type coercion

638c6d4

cloud-fan reviewed Mar 15, 2024

View reviewed changes

uros-db and others added 3 commits March 17, 2024 21:06

Fix ANSI type coercion

af7611f

Merge branch 'apache:master' into regexp-functions

ceabb4a

Fix failing tests

0f4a6b1

mihailomilosevic2001 mentioned this pull request Mar 19, 2024

[SPARK-47210][SQL] Addition of implicit casting without indeterminate support #45383

Closed

cloud-fan reviewed Mar 19, 2024

View reviewed changes

Comment thread sql/core/src/test/scala/org/apache/spark/sql/CollationRegexpExpressionsSuite.scala

cloud-fan reviewed Mar 19, 2024

View reviewed changes

Comment thread sql/core/src/test/scala/org/apache/spark/sql/CollationStringExpressionsSuite.scala

Incorporate requested changes

9b24fba

mihailomilosevic2001 added 4 commits March 20, 2024 08:27

Merge branch 'apache:master' into regexp-functions

831197a

Refactor code and fix comments

99b36b0

Fix newlines

9051923

Add back newlines

6986b8b

cloud-fan approved these changes Mar 20, 2024

View reviewed changes

cloud-fan closed this in 8762e25 Mar 20, 2024

mihailomilosevic2001 mentioned this pull request Mar 25, 2024

[SPARK-47504][SQL] Resolve AbstractDataType simpleStrings for StringTypeCollated #45694

Closed

Conversation

uros-db commented Mar 7, 2024

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cloud-fan commented Mar 12, 2024

Uh oh!

uros-db commented Mar 12, 2024

Uh oh!

cloud-fan commented Mar 12, 2024

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cloud-fan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbatomic commented Mar 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mihailomilosevic2001 commented Mar 19, 2024

Uh oh!

cloud-fan commented Mar 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

uros-db commented Mar 13, 2024 •

edited

Loading