[SPARK-49334][SQL] `str_to_map` should check whether the `collation` values of all parameter types are the same #47825

panbingkun · 2024-08-21T09:07:39Z

What changes were proposed in this pull request?

From the implementation logic of expression str_to_map, we found that it only extracts collation of the first parameter text for the next logical processing, as follows:

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala

Line 588 in 7f1a0c6

    
           private final lazy val collationId: Int = text.dataType.asInstanceOf[StringType].collationId

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala

Lines 594 to 595 in 7f1a0c6

    
           val keyValues = CollationAwareUTF8String.splitSQL(inputString.asInstanceOf[UTF8String], 
        
             stringDelimiter.asInstanceOf[UTF8String], -1, collationId)

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala

Lines 600 to 601 in 7f1a0c6

    
           val keyValueArray = CollationAwareUTF8String.splitSQL( 
        
             keyValues(i), keyValueDelimiterUTF8String, 2, collationId)

I propose to perform consistency checks on the collation values of all parameters to avoid potential issues.

Why are the changes needed?

Strengthen the parameter data type check of expression str_to_map to avoid potential issues.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Add new UT.
Pass GA.

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

No.

…values of all parameter types are the same

panbingkun · 2024-08-21T09:15:25Z

cc @MaxGekk @cloud-fan

MaxGekk

It looks reasonable. cc @uros-db You touched the expression last time, please, take a look at the changes.

uros-db · 2024-08-21T12:46:26Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala

+  override def checkInputDataTypes(): TypeCheckResult = {
+    val defaultCheck = super.checkInputDataTypes()
+    if (defaultCheck.isFailure) {
+      defaultCheck
+    } else if (!TypeCoercion.haveSameType(children.map(_.dataType))) {
+      DataTypeMismatch(
+        errorSubClass = "DATA_DIFF_TYPES",
+        messageParameters = Map(
+          "functionName" -> toSQLId(prettyName),
+          "dataType" -> children.map(_.dataType).map(toSQLType).mkString("[", ", ", "]")
+        )
+      )
+    } else {
+      TypeCheckSuccess
+    }
+  }


this is not how we usually enforce collation type coercion

please see CollationTypeCasts.scala

Let me take a closer look, thank you!

uros-db · 2024-08-21T12:48:19Z

sql/core/src/test/scala/org/apache/spark/sql/CollationSQLExpressionsSuite.scala

+    val tableName = "t_diff_collation"
+    withTable(tableName) {
+      sql(s"CREATE TABLE $tableName (" +
+        s"text STRING COLLATE UTF8_BINARY, " +
+        s"pairDelim STRING COLLATE UTF8_LCASE, " +
+        s"keyValueDelim STRING COLLATE UTF8_BINARY) " +
+        s"USING parquet")
+      checkError(
+        exception = intercept[AnalysisException] {
+          sql(s"SELECT str_to_map(text, pairDelim, keyValueDelim) from $tableName")
+        },
+        errorClass = "DATATYPE_MISMATCH.DATA_DIFF_TYPES",
+        sqlState = "42K09",
+        parameters = Map(
+          "functionName" -> "`str_to_map`",
+          "dataType" -> "[\"STRING\", \"STRING COLLATE UTF8_LCASE\", \"STRING\"]",
+          "sqlExpr" -> "\"str_to_map(text, pairDelim, keyValueDelim)\""),
+        context = ExpectedContext(
+          fragment = "str_to_map(text, pairDelim, keyValueDelim)",
+          start = 7,
+          stop = 48)
+      )
+    }


we shouldn't use DATATYPE_MISMATCH.DATA_DIFF_TYPES for collation match verification

please see COLLATION_MISMATCH.EXPLICIT, there should be many tests across the codebase market with "// Collation mismatch"

uros-db · 2024-08-21T12:49:45Z

sql/core/src/test/scala/org/apache/spark/sql/CollationSQLExpressionsSuite.scala

+    withTable(tableName) {
+      sql(s"CREATE TABLE $tableName (" +
+        s"text STRING COLLATE UTF8_BINARY, " +
+        s"pairDelim STRING COLLATE UTF8_LCASE, " +
+        s"keyValueDelim STRING COLLATE UTF8_BINARY) " +
+        s"USING parquet")


also, there is an ongoing effort to move such tests to collations.sql golden file

please see: #47828 and https://issues.apache.org/jira/browse/SPARK-48779

Updated, I have moved the related tests to file collations.sql.

uros-db

please try to include collation-related tickets (such as https://issues.apache.org/jira/browse/SPARK-49334) in the corresponding Collation support epic: https://issues.apache.org/jira/browse/SPARK-46830

even better, we have a String function support parent under this epic: https://issues.apache.org/jira/browse/SPARK-46837

this way, we can all see and discuss the ongoing collation tickets

uros-db

to sum up, I do agree that type coercion should be enforced here

the reasoning is: pairDelim and keyValueDelim are treated as (collation-dependent) delimiters, rather than (collation-unaware) regular expressions

please make the requsted changes, and feel free to request a re-review as needed

panbingkun · 2024-08-22T03:19:03Z

please try to include collation-related tickets (such as https://issues.apache.org/jira/browse/SPARK-49334) in the corresponding Collation support epic: https://issues.apache.org/jira/browse/SPARK-46830

even better, we have a String function support parent under this epic: https://issues.apache.org/jira/browse/SPARK-46837

this way, we can all see and discuss the ongoing collation tickets

I have moved this pr(SPARK-49334) as a subtask to SPARK-46837

panbingkun · 2024-08-22T03:28:26Z

to sum up, I do agree that type coercion should be enforced here

the reasoning is: pairDelim and keyValueDelim are treated as (collation-dependent) delimiters, rather than (collation-unaware) regular expressions

please make the requsted changes, and feel free to request a re-review as needed

This PR has been updated according to the requsted changes,
please review it again when you have free time, thank you very much!

uros-db · 2024-08-22T06:30:30Z

sql/core/src/test/resources/sql-tests/inputs/collations.sql

@@ -90,3 +90,14 @@ select 'a' collate en_ci_ai = 'Å';
 select 'Kypper' collate sv < 'Köpfe';
 select 'Kypper' collate de > 'Köpfe';
 select 'I' collate tr_ci = 'ı';
+
+-- create table for str_to_map
+create table t1 (text string collate utf8_binary, pairDelim string collate utf8_lcase, keyValueDelim string collate utf8_binary) using parquet;


Suggested change

create table t1 (text string collate utf8_binary, pairDelim string collate utf8_lcase, keyValueDelim string collate utf8_binary) using parquet;

create table t4 (text string collate utf8_binary, pairDelim string collate utf8_lcase, keyValueDelim string collate utf8_binary) using parquet;

let's just use another table name here, so that we don't confuse it with the three tables above in the future (I know that the naming is not currently consistent, but we do have an ongoing effort to revamp this golden file - https://issues.apache.org/jira/browse/SPARK-48779)

I highly agree, it has been updated!

uros-db

lgtm, just one small nit

MaxGekk · 2024-08-22T09:14:27Z

+1, LGTM. Merging to master.
Thank you, @panbingkun and @uros-db for review.

panbingkun · 2024-08-22T11:54:13Z

@uros-db @MaxGekk
I have checked all expressions with spark input data type StringTypeAnyCollation, and I believe that split_part and levenshtein should also need similar checks and enhancements. Therefore, I have submitted the following two PRs to complete it.

split_part: [SPARK-49354][SQL] split_part should check whether the collation values of all parameter types are the same #47845
levenshtein: [SPARK-49355][SQL] levenshtein should check whether the collation values of all parameter types are the same #47847

please help review it when you have free time, thank you very much!

…values of all parameter types are the same ### What changes were proposed in this pull request? The same principle as #47825 (review), the parameter `delimiter` in expression `split_part` are treated as (`collation-dependent`) delimiters, rather than (`collation-unaware`) regular expressions. ### Why are the changes needed? Strengthen the parameter data type check of expression `split_part` to avoid potential issues. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - Add some `test case` to `collations.sql`. - Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #47845 from panbingkun/SPARK-49354. Lead-authored-by: panbingkun <panbingkun@baidu.com> Co-authored-by: panbingkun <pbk1982@gmail.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

… values of all parameter types are the same ### What changes were proposed in this pull request? The same principle as #47825 (review), the parameters (`left` and `right`) in expression `levenshtein` are `collation-dependent`, rather than `collation-unaware`. ### Why are the changes needed? Strengthen the parameter data type check of expression `levenshtein` to avoid potential issues. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - Add some `test case` to `collations.sql`. - Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #47847 from panbingkun/SPARK-49355. Authored-by: panbingkun <panbingkun@baidu.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

…values of all parameter types are the same ### What changes were proposed in this pull request? From the implementation logic of expression `str_to_map`, we found that it only extracts `collation` of the first parameter `text` for the `next` logical processing, as follows: https://github.com/apache/spark/blob/7f1a0c62a03c0d6498503987c4855327c0b6cae1/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala#L588 https://github.com/apache/spark/blob/7f1a0c62a03c0d6498503987c4855327c0b6cae1/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala#L594-L595 https://github.com/apache/spark/blob/7f1a0c62a03c0d6498503987c4855327c0b6cae1/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala#L600-L601 I propose to perform `consistency checks` on the `collation` values of all parameters to avoid potential issues. ### Why are the changes needed? Strengthen the parameter data type check of expression `str_to_map` to avoid potential issues. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - Add new UT. - Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47825 from panbingkun/SPARK-49334. Authored-by: panbingkun <panbingkun@baidu.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

…values of all parameter types are the same ### What changes were proposed in this pull request? The same principle as apache#47825 (review), the parameter `delimiter` in expression `split_part` are treated as (`collation-dependent`) delimiters, rather than (`collation-unaware`) regular expressions. ### Why are the changes needed? Strengthen the parameter data type check of expression `split_part` to avoid potential issues. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - Add some `test case` to `collations.sql`. - Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47845 from panbingkun/SPARK-49354. Lead-authored-by: panbingkun <panbingkun@baidu.com> Co-authored-by: panbingkun <pbk1982@gmail.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

… values of all parameter types are the same ### What changes were proposed in this pull request? The same principle as apache#47825 (review), the parameters (`left` and `right`) in expression `levenshtein` are `collation-dependent`, rather than `collation-unaware`. ### Why are the changes needed? Strengthen the parameter data type check of expression `levenshtein` to avoid potential issues. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - Add some `test case` to `collations.sql`. - Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47847 from panbingkun/SPARK-49355. Authored-by: panbingkun <panbingkun@baidu.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

…values of all parameter types are the same ### What changes were proposed in this pull request? From the implementation logic of expression `str_to_map`, we found that it only extracts `collation` of the first parameter `text` for the `next` logical processing, as follows: https://github.com/apache/spark/blob/7f1a0c62a03c0d6498503987c4855327c0b6cae1/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala#L588 https://github.com/apache/spark/blob/7f1a0c62a03c0d6498503987c4855327c0b6cae1/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala#L594-L595 https://github.com/apache/spark/blob/7f1a0c62a03c0d6498503987c4855327c0b6cae1/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala#L600-L601 I propose to perform `consistency checks` on the `collation` values of all parameters to avoid potential issues. ### Why are the changes needed? Strengthen the parameter data type check of expression `str_to_map` to avoid potential issues. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - Add new UT. - Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47825 from panbingkun/SPARK-49334. Authored-by: panbingkun <panbingkun@baidu.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

…values of all parameter types are the same ### What changes were proposed in this pull request? The same principle as apache#47825 (review), the parameter `delimiter` in expression `split_part` are treated as (`collation-dependent`) delimiters, rather than (`collation-unaware`) regular expressions. ### Why are the changes needed? Strengthen the parameter data type check of expression `split_part` to avoid potential issues. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - Add some `test case` to `collations.sql`. - Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47845 from panbingkun/SPARK-49354. Lead-authored-by: panbingkun <panbingkun@baidu.com> Co-authored-by: panbingkun <pbk1982@gmail.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

… values of all parameter types are the same ### What changes were proposed in this pull request? The same principle as apache#47825 (review), the parameters (`left` and `right`) in expression `levenshtein` are `collation-dependent`, rather than `collation-unaware`. ### Why are the changes needed? Strengthen the parameter data type check of expression `levenshtein` to avoid potential issues. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - Add some `test case` to `collations.sql`. - Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47847 from panbingkun/SPARK-49355. Authored-by: panbingkun <panbingkun@baidu.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

[SPARK-49334][SQL] str_to_map should check whether the collation …

fed4a7e

…values of all parameter types are the same

github-actions bot added the SQL label Aug 21, 2024

panbingkun marked this pull request as ready for review August 21, 2024 09:15

panbingkun marked this pull request as draft August 21, 2024 11:12

fix ut

1973bbc

panbingkun marked this pull request as ready for review August 21, 2024 11:35

MaxGekk reviewed Aug 21, 2024

View reviewed changes

uros-db reviewed Aug 21, 2024

View reviewed changes

uros-db suggested changes Aug 21, 2024

View reviewed changes

panbingkun added 3 commits August 22, 2024 10:35

update

e8be51b

Merge branch 'master' into SPARK-49334

a611128

update

899fd22

panbingkun requested a review from uros-db August 22, 2024 03:20

uros-db reviewed Aug 22, 2024

View reviewed changes

uros-db approved these changes Aug 22, 2024

View reviewed changes

panbingkun added 2 commits August 22, 2024 14:40

Merge branch 'master' into SPARK-49334

109af97

replace t1 with t4

4d1ac6b

panbingkun requested a review from MaxGekk August 22, 2024 06:47

MaxGekk approved these changes Aug 22, 2024

View reviewed changes

MaxGekk closed this in 309c543 Aug 22, 2024

This was referenced Aug 22, 2024

[SPARK-49354][SQL] split_part should check whether the collation values of all parameter types are the same #47845

Closed

[SPARK-49355][SQL] levenshtein should check whether the collation values of all parameter types are the same #47847

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-49334][SQL] `str_to_map` should check whether the `collation` values of all parameter types are the same #47825

[SPARK-49334][SQL] `str_to_map` should check whether the `collation` values of all parameter types are the same #47825

panbingkun commented Aug 21, 2024 •

edited

Loading

panbingkun commented Aug 21, 2024

MaxGekk left a comment •

edited

Loading

uros-db Aug 21, 2024

panbingkun Aug 22, 2024

uros-db Aug 21, 2024 •

edited

Loading

uros-db Aug 21, 2024

panbingkun Aug 22, 2024

uros-db left a comment

uros-db left a comment

panbingkun commented Aug 22, 2024

panbingkun commented Aug 22, 2024 •

edited

Loading

uros-db Aug 22, 2024 •

edited

Loading

panbingkun Aug 22, 2024

uros-db left a comment

MaxGekk commented Aug 22, 2024

panbingkun commented Aug 22, 2024

	val keyValues = CollationAwareUTF8String.splitSQL(inputString.asInstanceOf[UTF8String],
	stringDelimiter.asInstanceOf[UTF8String], -1, collationId)

	val keyValueArray = CollationAwareUTF8String.splitSQL(
	keyValues(i), keyValueDelimiterUTF8String, 2, collationId)

	create table t1 (text string collate utf8_binary, pairDelim string collate utf8_lcase, keyValueDelim string collate utf8_binary) using parquet;
	create table t4 (text string collate utf8_binary, pairDelim string collate utf8_lcase, keyValueDelim string collate utf8_binary) using parquet;

[SPARK-49334][SQL] str_to_map should check whether the collation values of all parameter types are the same #47825

[SPARK-49334][SQL] str_to_map should check whether the collation values of all parameter types are the same #47825

Conversation

panbingkun commented Aug 21, 2024 • edited Loading

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?

panbingkun commented Aug 21, 2024

MaxGekk left a comment • edited Loading

Choose a reason for hiding this comment

uros-db Aug 21, 2024

Choose a reason for hiding this comment

panbingkun Aug 22, 2024

Choose a reason for hiding this comment

uros-db Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

uros-db Aug 21, 2024

Choose a reason for hiding this comment

panbingkun Aug 22, 2024

Choose a reason for hiding this comment

uros-db left a comment

Choose a reason for hiding this comment

uros-db left a comment

Choose a reason for hiding this comment

panbingkun commented Aug 22, 2024

panbingkun commented Aug 22, 2024 • edited Loading

uros-db Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

panbingkun Aug 22, 2024

Choose a reason for hiding this comment

uros-db left a comment

Choose a reason for hiding this comment

MaxGekk commented Aug 22, 2024

panbingkun commented Aug 22, 2024

[SPARK-49334][SQL] `str_to_map` should check whether the `collation` values of all parameter types are the same #47825

[SPARK-49334][SQL] `str_to_map` should check whether the `collation` values of all parameter types are the same #47825

panbingkun commented Aug 21, 2024 •

edited

Loading

MaxGekk left a comment •

edited

Loading

uros-db Aug 21, 2024 •

edited

Loading

panbingkun commented Aug 22, 2024 •

edited

Loading

uros-db Aug 22, 2024 •

edited

Loading