You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "stefankandic (via GitHub)" <gi...@apache.org> on 2024/12/11 13:56:02 UTC
[PR] [SPARK-50546] Add subquery cast support to collation type coercion [spark]
stefankandic opened a new pull request, #49147:
URL: https://github.com/apache/spark/pull/49147
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
Adding proper support for implicit casting of subqueries in collation type coercion. Currently I only added support for `Project` and `Aggregate` plans in subquery expression, but we might need to add more later.
### Why are the changes needed?
Since `SubqueryExpression` is a special type of expression which can't simply be cast to a new type, we need to add this as a special case to avoid errors when trying to change the data type in these types of queries.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Added new unit tests.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50546][SQL] Add subquery cast support to collation type coercion [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #49147:
URL: https://github.com/apache/spark/pull/49147#discussion_r1882744085
##########
sql/core/src/test/scala/org/apache/spark/sql/collation/CollationTypePrecedenceSuite.scala:
##########
@@ -175,6 +175,115 @@ class CollationTypePrecedenceSuite extends QueryTest with SharedSparkSession {
)
}
+ test("in subquery expression") {
+ val tableName = "subquery_tbl"
+ withTable(tableName) {
+ sql(
+ s"""
+ |CREATE TABLE $tableName (
+ | c1 STRING COLLATE UTF8_LCASE,
+ | c2 STRING COLLATE UNICODE
+ |) USING $dataSource
+ |""".stripMargin)
+
+ sql(s"INSERT INTO $tableName VALUES ('a', 'A')")
+
+ assertImplicitMismatch(
+ sql(
+ s"""
+ |SELECT * FROM $tableName
+ |WHERE c1 IN (SELECT c2 FROM $tableName)
+ |""".stripMargin))
Review Comment:
please, fix indentation in all places
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50546][SQL] Add subquery cast support to collation type coercion [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #49147:
URL: https://github.com/apache/spark/pull/49147#issuecomment-2542121956
+1, LGTM. Merging to master.
Thank you, @stefankandic and @mihailom-db for review.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50546][SQL] Add subquery cast support to collation type coercion [spark]
Posted by "stefankandic (via GitHub)" <gi...@apache.org>.
stefankandic commented on code in PR #49147:
URL: https://github.com/apache/spark/pull/49147#discussion_r1882758341
##########
sql/core/src/test/scala/org/apache/spark/sql/collation/CollationTypePrecedenceSuite.scala:
##########
@@ -175,6 +175,115 @@ class CollationTypePrecedenceSuite extends QueryTest with SharedSparkSession {
)
}
+ test("in subquery expression") {
+ val tableName = "subquery_tbl"
+ withTable(tableName) {
+ sql(
+ s"""
+ |CREATE TABLE $tableName (
+ | c1 STRING COLLATE UTF8_LCASE,
+ | c2 STRING COLLATE UNICODE
+ |) USING $dataSource
+ |""".stripMargin)
+
+ sql(s"INSERT INTO $tableName VALUES ('a', 'A')")
+
+ assertImplicitMismatch(
+ sql(
+ s"""
+ |SELECT * FROM $tableName
+ |WHERE c1 IN (SELECT c2 FROM $tableName)
+ |""".stripMargin))
Review Comment:
Why do we have to do this? This is the way intellij formats multi line strings by default, and looking at the code base it seems like aligning the `|` with the third quote is more often used anyways
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50546][SQL] Add subquery cast support to collation type coercion [spark]
Posted by "mihailom-db (via GitHub)" <gi...@apache.org>.
mihailom-db commented on code in PR #49147:
URL: https://github.com/apache/spark/pull/49147#discussion_r1881737978
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CollationTypeCoercion.scala:
##########
@@ -180,6 +184,36 @@ object CollationTypeCoercion {
}
}
+ /**
+ * Changes the data type of the expression in the subquery to the given `newType`.
+ * Currently only supports subqueries with [[Project]] and [[Aggregate]] plan.
+ */
+ private def changeTypeInSubquery(
+ subqueryExpression: SubqueryExpression,
+ newType: DataType): SubqueryExpression = {
+
+ def transformNamedExpressions(ex: NamedExpression): NamedExpression = {
+ changeType(ex, newType) match {
+ case named: NamedExpression => named
+ case other => Alias(other, ex.name)()
+ }
+ }
+
+ val newPlan = subqueryExpression.plan match {
+ case project: Project =>
+ val newProjectList = project.projectList.map(transformNamedExpressions)
+ project.copy(projectList = newProjectList)
+
+ case agg: Aggregate =>
+ val newAggregateExpressions = agg.aggregateExpressions.map(transformNamedExpressions)
+ agg.copy(aggregateExpressions = newAggregateExpressions)
+
Review Comment:
Do we need SubqueryAlias here?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50546][SQL] Add subquery cast support to collation type coercion [spark]
Posted by "stefankandic (via GitHub)" <gi...@apache.org>.
stefankandic commented on PR #49147:
URL: https://github.com/apache/spark/pull/49147#issuecomment-2536060263
@stevomitric @mihailom-db please take a look
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50546][SQL] Add subquery cast support to collation type coercion [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #49147:
URL: https://github.com/apache/spark/pull/49147#discussion_r1882685998
##########
sql/core/src/test/scala/org/apache/spark/sql/collation/CollationTypePrecedenceSuite.scala:
##########
@@ -175,6 +175,115 @@ class CollationTypePrecedenceSuite extends QueryTest with SharedSparkSession {
)
}
+ test("in subquery expression") {
+ val tableName = "subquery_tbl"
+ withTable(tableName) {
+ sql(
+ s"""
+ |CREATE TABLE $tableName (
+ | c1 STRING COLLATE UTF8_LCASE,
+ | c2 STRING COLLATE UNICODE
+ |) USING $dataSource
+ |""".stripMargin)
Review Comment:
indentation should be 2 spaces not 3. Please, fix here and below:
```suggestion
|CREATE TABLE $tableName (
| c1 STRING COLLATE UTF8_LCASE,
| c2 STRING COLLATE UNICODE
|) USING $dataSource
|""".stripMargin)
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50546][SQL] Add subquery cast support to collation type coercion [spark]
Posted by "stefankandic (via GitHub)" <gi...@apache.org>.
stefankandic commented on PR #49147:
URL: https://github.com/apache/spark/pull/49147#issuecomment-2539585395
@MaxGekk can you also take a look?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50546][SQL] Add subquery cast support to collation type coercion [spark]
Posted by "stefankandic (via GitHub)" <gi...@apache.org>.
stefankandic commented on code in PR #49147:
URL: https://github.com/apache/spark/pull/49147#discussion_r1881832315
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CollationTypeCoercion.scala:
##########
@@ -180,6 +184,36 @@ object CollationTypeCoercion {
}
}
+ /**
+ * Changes the data type of the expression in the subquery to the given `newType`.
+ * Currently only supports subqueries with [[Project]] and [[Aggregate]] plan.
+ */
+ private def changeTypeInSubquery(
+ subqueryExpression: SubqueryExpression,
+ newType: DataType): SubqueryExpression = {
+
+ def transformNamedExpressions(ex: NamedExpression): NamedExpression = {
+ changeType(ex, newType) match {
+ case named: NamedExpression => named
+ case other => Alias(other, ex.name)()
+ }
+ }
+
+ val newPlan = subqueryExpression.plan match {
+ case project: Project =>
+ val newProjectList = project.projectList.map(transformNamedExpressions)
+ project.copy(projectList = newProjectList)
+
+ case agg: Aggregate =>
+ val newAggregateExpressions = agg.aggregateExpressions.map(transformNamedExpressions)
+ agg.copy(aggregateExpressions = newAggregateExpressions)
+
Review Comment:
<img width="570" alt="image" src="https://github.com/user-attachments/assets/54645a39-edbd-4376-b95f-6b0d74d28bbc" />
I don't think so, this is how the plan will look like if we have an alias for the subquery
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CollationTypeCoercion.scala:
##########
@@ -180,6 +184,36 @@ object CollationTypeCoercion {
}
}
+ /**
+ * Changes the data type of the expression in the subquery to the given `newType`.
+ * Currently only supports subqueries with [[Project]] and [[Aggregate]] plan.
+ */
+ private def changeTypeInSubquery(
+ subqueryExpression: SubqueryExpression,
+ newType: DataType): SubqueryExpression = {
+
+ def transformNamedExpressions(ex: NamedExpression): NamedExpression = {
+ changeType(ex, newType) match {
+ case named: NamedExpression => named
+ case other => Alias(other, ex.name)()
+ }
+ }
+
+ val newPlan = subqueryExpression.plan match {
+ case project: Project =>
+ val newProjectList = project.projectList.map(transformNamedExpressions)
+ project.copy(projectList = newProjectList)
+
+ case agg: Aggregate =>
+ val newAggregateExpressions = agg.aggregateExpressions.map(transformNamedExpressions)
+ agg.copy(aggregateExpressions = newAggregateExpressions)
+
Review Comment:
<img width="570" alt="image" src="https://github.com/user-attachments/assets/54645a39-edbd-4376-b95f-6b0d74d28bbc" />
I don't think so, this is how the plan will look like if we have an alias for the subquery
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50546][SQL] Add subquery cast support to collation type coercion [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #49147:
URL: https://github.com/apache/spark/pull/49147#discussion_r1882955753
##########
sql/core/src/test/scala/org/apache/spark/sql/collation/CollationTypePrecedenceSuite.scala:
##########
@@ -175,6 +175,115 @@ class CollationTypePrecedenceSuite extends QueryTest with SharedSparkSession {
)
}
+ test("in subquery expression") {
+ val tableName = "subquery_tbl"
+ withTable(tableName) {
+ sql(
+ s"""
+ |CREATE TABLE $tableName (
+ | c1 STRING COLLATE UTF8_LCASE,
+ | c2 STRING COLLATE UNICODE
+ |) USING $dataSource
+ |""".stripMargin)
+
+ sql(s"INSERT INTO $tableName VALUES ('a', 'A')")
+
+ assertImplicitMismatch(
+ sql(
+ s"""
+ |SELECT * FROM $tableName
+ |WHERE c1 IN (SELECT c2 FROM $tableName)
+ |""".stripMargin))
Review Comment:
> This is the way intellij formats multi line strings by default
Just tune IDE once according to the style guide, and the issue will be fixed forever.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50546][SQL] Add subquery cast support to collation type coercion [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #49147: [SPARK-50546][SQL] Add subquery cast support to collation type coercion
URL: https://github.com/apache/spark/pull/49147
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org