You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "jovanm-db (via GitHub)" <gi...@apache.org> on 2024/10/21 12:06:04 UTC

[PR] [SPARK-48413][SQL][FOLLOW-UP] Fix ALTER COLUMN with collation [spark]

jovanm-db opened a new pull request, #48582:
URL: https://github.com/apache/spark/pull/48582

   ### What changes were proposed in this pull request?
   Add support for changing collation of a column with ALTER COLUMN command. Use existing support for ALTER COLUMN with type to enable changing collations of column. Syntax example:
   
   ```sql
   ALTER TABLE t1 ALTER COLUMN col TYPE STRING COLLATE UTF8_BINARY_LCASE
   ```
   
   
   ### Why are the changes needed?
   Enable changing collation on column.
   
   
   ### Does this PR introduce _any_ user-facing change?
   Yes, it adds support for changing collation of column.
   
   
   ### How was this patch tested?
   Added tests to `DDLSuite` and `DataTypeSuite`.
   
   
   ### 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-48413][SQL][FOLLOW-UP] Fix ALTER COLUMN with collation [spark]

Posted by "mihailom-db (via GitHub)" <gi...@apache.org>.
mihailom-db commented on code in PR #48582:
URL: https://github.com/apache/spark/pull/48582#discussion_r1810073569


##########
sql/core/src/test/scala/org/apache/spark/sql/CollationSuite.scala:
##########
@@ -516,6 +516,16 @@ class CollationSuite extends DatasourceV2SQLBase with AdaptiveSparkPlanHelper {
     }
   }
 
+  test("SPARK-48413: Alter column with collation") {
+    val tableName = "testcat.alter_column_tbl"
+    withTable(tableName) {
+      sql(s"CREATE TABLE $tableName (c1 STRING) USING PARQUET")
+      sql(s"ALTER TABLE $tableName ALTER COLUMN c1 TYPE STRING COLLATE UTF8_LCASE")
+      sql(s"INSERT INTO $tableName VALUES ('a')")

Review Comment:
   ```suggestion
         sql(s"INSERT INTO $tableName VALUES ('a')")
         sql(s"ALTER TABLE $tableName ALTER COLUMN c1 TYPE STRING COLLATE UTF8_LCASE")
   ```



-- 
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-48413][SQL][FOLLOW-UP] Fix ALTER COLUMN with collation [spark]

Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #48582: [SPARK-48413][SQL][FOLLOW-UP] Fix ALTER COLUMN with collation
URL: https://github.com/apache/spark/pull/48582


-- 
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-48413][SQL][FOLLOW-UP] Fix ALTER COLUMN with collation [spark]

Posted by "mihailom-db (via GitHub)" <gi...@apache.org>.
mihailom-db commented on code in PR #48582:
URL: https://github.com/apache/spark/pull/48582#discussion_r1808694526


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -1546,7 +1546,10 @@ trait CheckAnalysis extends PredicateHelper with LookupCatalog with QueryErrorsB
             .map(dt => col.field.copy(dataType = dt))
             .getOrElse(col.field)
           val newDataType = a.dataType.get
+          val sameTypeExceptCollations =
+            DataType.equalsIgnoreCompatibleCollation(field.dataType, newDataType)
           newDataType match {
+            case _ if sameTypeExceptCollations =>

Review Comment:
   Add comment saying as to why this is allowed. We allow it as we want to be able to change collations.



##########
sql/core/src/test/scala/org/apache/spark/sql/CollationSuite.scala:
##########
@@ -516,6 +516,14 @@ class CollationSuite extends DatasourceV2SQLBase with AdaptiveSparkPlanHelper {
     }
   }
 
+  test("SPARK-48413: Alter column with collation") {
+    val tableName = "testcat.alter_column_tbl"
+    withTable(tableName) {
+      sql(s"CREATE TABLE $tableName (c1 STRING) USING PARQUET")
+      sql(s"ALTER TABLE $tableName ALTER COLUMN c1 TYPE STRING COLLATE UTF8_LCASE")
+    }

Review Comment:
   Could you just for readability and completeness check that the collation of column c1 changed? Knowing that we passed the change, but not checking what we actually did is not sufficient.



-- 
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-48413][SQL][FOLLOW-UP] Fix ALTER COLUMN with collation [spark]

Posted by "mihailom-db (via GitHub)" <gi...@apache.org>.
mihailom-db commented on code in PR #48582:
URL: https://github.com/apache/spark/pull/48582#discussion_r1810074209


##########
sql/core/src/test/scala/org/apache/spark/sql/CollationSuite.scala:
##########
@@ -516,6 +516,16 @@ class CollationSuite extends DatasourceV2SQLBase with AdaptiveSparkPlanHelper {
     }
   }
 
+  test("SPARK-48413: Alter column with collation") {
+    val tableName = "testcat.alter_column_tbl"
+    withTable(tableName) {
+      sql(s"CREATE TABLE $tableName (c1 STRING) USING PARQUET")
+      sql(s"ALTER TABLE $tableName ALTER COLUMN c1 TYPE STRING COLLATE UTF8_LCASE")
+      sql(s"INSERT INTO $tableName VALUES ('a')")

Review Comment:
   Let's first insert and then alter, seems a bit nicer for the test.



-- 
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-48413][SQL][FOLLOW-UP] Fix ALTER COLUMN with collation [spark]

Posted by "stefankandic (via GitHub)" <gi...@apache.org>.
stefankandic commented on code in PR #48582:
URL: https://github.com/apache/spark/pull/48582#discussion_r1810462624


##########
sql/core/src/test/scala/org/apache/spark/sql/CollationSuite.scala:
##########
@@ -516,6 +516,16 @@ class CollationSuite extends DatasourceV2SQLBase with AdaptiveSparkPlanHelper {
     }
   }
 
+  test("SPARK-48413: Alter column with collation") {

Review Comment:
   can we also add tests for complex types (struct, map, array)?



-- 
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-48413][SQL][FOLLOW-UP] Fix ALTER COLUMN with collation [spark]

Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #48582:
URL: https://github.com/apache/spark/pull/48582#issuecomment-2429596554

   +1, LGTM. Merging to master.
   Thank you, @jovanm-db and @mihailom-db @stefankandic 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