[SPARK-33095] Follow up, support alter table column rename. #30142

ScrapCodes · 2020-10-23T15:07:24Z

What changes were proposed in this pull request?

Support rename column for mysql dialect.

Why are the changes needed?

At the moment, it does not work for mysql version 5.x. So, we should throw proper exception for that case.

Does this PR introduce any user-facing change?

Yes, column rename with mysql dialect should work correctly.

How was this patch tested?

Added tests for rename column.
Ran the tests to pass with both versions of mysql.

export MYSQL_DOCKER_IMAGE_NAME=mysql:5.7.31
export MYSQL_DOCKER_IMAGE_NAME=mysql:8.0

ScrapCodes · 2020-10-23T15:09:27Z

While testing #30038, I found rename column needs some work for mysql dialect. More details added in the code comments.

cc @cloud-fan and @huaxingao

SparkQA · 2020-10-23T15:59:45Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34806/

SparkQA · 2020-10-23T16:00:16Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34807/

SparkQA · 2020-10-23T16:23:12Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34806/

SparkQA · 2020-10-23T16:24:57Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34807/

SparkQA · 2020-10-23T17:44:22Z

Test build #130205 has finished for PR 30142 at commit 73b0550.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-10-23T18:17:10Z

Test build #130207 has finished for PR 30142 at commit f4b8a24.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-10-23T18:23:03Z

Test build #130206 has finished for PR 30142 at commit 7c9aa72.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-10-26T09:16:39Z

...er-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MySQLIntegrationSuite.scala

  }

+  override def testRenameColumn(tbl: String): Unit = {
+    if (db.imageName.matches(".*mysql.5\\.[0-9].*")) {


we can check the config value here.

cloud-fan · 2020-10-26T09:17:15Z

...er-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MySQLIntegrationSuite.scala

+  override def testRenameColumn(tbl: String): Unit = {
+    if (db.imageName.matches(".*mysql.5\\.[0-9].*")) {
+      sql(s"CREATE TABLE $tbl (ID STRING NOT NULL) USING _")
+      // Update nullability is unsupported for mysql db.


rename is unsupported ...

cloud-fan · 2020-10-26T09:18:50Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

    .createWithDefault("")

+  val JDBC_MYSQL_VERSION =
+    buildConf("spark.sql.jdbc.mysql.version")


what if users want to connect to both mysql 5 and 8? I think it's better to make it a catalog option, not a session config.

huaxingao · 2020-10-28T17:06:48Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/MySQLDialect.scala

+      tableName: String,
+      columnName: String,
+      newName: String): String = {
+    if (SQLConf.get.jdbcMySQLVersion.matches("^8\\.[0-9].*")) {


Instead of using a session config, we probably want to get database version using DatabaseMetaData in JdbcUtils.altertable and pass the info here, and only do RENAME if version >= 8

conn.getMetaData.getDatabaseMajorVersion

ah, it's even better if we can get the database version, so that users don't need to specify it.

Thank you @huaxingao and @cloud-fan, for this suggestion.

SparkQA · 2020-10-29T09:53:53Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35006/

SparkQA · 2020-10-29T10:22:08Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35006/

ScrapCodes · 2020-10-29T10:28:44Z

@cloud-fan and @huaxingao I have updated the PR with your suggestions and also rebased it with master. Please take a look again.

SparkQA · 2020-10-29T11:14:11Z

Test build #130402 has finished for PR 30142 at commit 2e4fd62.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-10-29T11:17:39Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35008/

ScrapCodes · 2020-10-29T11:29:50Z

Jenkins, retest this please

SparkQA · 2020-10-29T11:47:01Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35008/

cloud-fan · 2020-10-29T12:10:27Z

...er-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MySQLIntegrationSuite.scala

-  override def sparkConf: SparkConf = super.sparkConf
-    .set("spark.sql.catalog.mysql", classOf[JDBCTableCatalog].getName)
-    .set("spark.sql.catalog.mysql.url", db.getJdbcUrl(dockerIp, externalPort))
+  override def sparkConf: SparkConf = {


unnecessary change

SparkQA · 2020-10-29T12:12:14Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35011/

cloud-fan · 2020-10-29T12:14:52Z

...er-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MySQLIntegrationSuite.scala

  }

+  override def testRenameColumn(tbl: String): Unit = {
+    assert( mySQLVersion > 0)


nit: assert(mySQLVersion > 0) (remove extra space)

cloud-fan · 2020-10-29T12:16:03Z

external/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/V2JDBCTest.scala

+    val expectedSchema = new StructType().add("ID2", StringType, nullable = true)
+      .add("ID1", StringType, nullable = true)
+    assert(t.schema === expectedSchema)
+    // Rename to already existing column


This can be put in the test("SPARK-33034: ALTER TABLE ... rename column"), as the behavior should always be the same.

SparkQA · 2020-10-29T12:33:01Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35011/

SparkQA · 2020-10-29T13:03:13Z

Test build #130404 has finished for PR 30142 at commit 9e1f75a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-10-29T13:03:25Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35012/

SparkQA · 2020-10-29T13:31:47Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35012/

SparkQA · 2020-10-29T14:28:55Z

Test build #130407 has finished for PR 30142 at commit 9e1f75a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-10-29T15:22:57Z

Test build #130408 has finished for PR 30142 at commit fe1e44f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

huaxingao · 2020-10-29T17:23:36Z

@ScrapCodes seems you forgot to address this #30142 (comment). Everything else is good.

SparkQA · 2020-10-30T10:17:43Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35057/

SparkQA · 2020-10-30T10:47:19Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35057/

SparkQA · 2020-10-30T12:30:37Z

Test build #130452 has finished for PR 30142 at commit ce70996.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

ScrapCodes · 2020-10-31T10:05:57Z

Hi @huaxingao and @cloud-fan, Please take a look.

cloud-fan · 2020-11-02T05:03:33Z

thanks, merging to master!

ScrapCodes force-pushed the mysql-dialect-rename branch 2 times, most recently from 7c9aa72 to f4b8a24 Compare October 23, 2020 15:19

huaxingao mentioned this pull request Oct 23, 2020

[SPARK-33137][SQL] Support ALTER TABLE in JDBC v2 Table Catalog: update type and nullability of columns (Postgres dialect) #30089

Closed

cloud-fan reviewed Oct 26, 2020

View reviewed changes

huaxingao reviewed Oct 28, 2020

View reviewed changes

ScrapCodes added 3 commits October 29, 2020 10:35

[SPARK-33095] Follow up, support alter table column rename.

4c2e622

improved test and used database meta data to get the db major version.

f7c02e4

Removed the SQL Conf, which is no longer used.

2e4fd62

ScrapCodes force-pushed the mysql-dialect-rename branch from f4b8a24 to 2e4fd62 Compare October 29, 2020 09:08

ScrapCodes requested review from cloud-fan and huaxingao October 29, 2020 10:27

fixed unused import

9e1f75a

ScrapCodes force-pushed the mysql-dialect-rename branch from 55cca16 to 9e1f75a Compare October 29, 2020 10:32

cloud-fan reviewed Oct 29, 2020

View reviewed changes

cloud-fan approved these changes Oct 29, 2020

View reviewed changes

feedback

fe1e44f

improved the tests

31e68e4

huaxingao approved these changes Oct 29, 2020

View reviewed changes

feedback

ce70996

ScrapCodes force-pushed the mysql-dialect-rename branch from 5b8e45e to ce70996 Compare October 30, 2020 09:32

cloud-fan closed this in 6226ccc Nov 2, 2020

[SPARK-33095] Follow up, support alter table column rename. #30142

[SPARK-33095] Follow up, support alter table column rename. #30142

Uh oh!

Conversation

ScrapCodes commented Oct 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

ScrapCodes commented Oct 23, 2020

Uh oh!

SparkQA commented Oct 23, 2020

Uh oh!

SparkQA commented Oct 23, 2020

Uh oh!

SparkQA commented Oct 23, 2020

Uh oh!

SparkQA commented Oct 23, 2020

Uh oh!

SparkQA commented Oct 23, 2020

Uh oh!

SparkQA commented Oct 23, 2020

Uh oh!

SparkQA commented Oct 23, 2020

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!

SparkQA commented Oct 29, 2020

Uh oh!

SparkQA commented Oct 29, 2020

Uh oh!

ScrapCodes commented Oct 29, 2020

Uh oh!

SparkQA commented Oct 29, 2020

Uh oh!

SparkQA commented Oct 29, 2020

Uh oh!

ScrapCodes commented Oct 29, 2020

Uh oh!

SparkQA commented Oct 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 29, 2020

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!

SparkQA commented Oct 29, 2020

Uh oh!

SparkQA commented Oct 29, 2020

Uh oh!

SparkQA commented Oct 29, 2020

Uh oh!

SparkQA commented Oct 29, 2020

Uh oh!

SparkQA commented Oct 29, 2020

Uh oh!

SparkQA commented Oct 29, 2020

Uh oh!

huaxingao commented Oct 29, 2020

Uh oh!

SparkQA commented Oct 30, 2020

Uh oh!

SparkQA commented Oct 30, 2020

ScrapCodes commented Oct 23, 2020 •

edited

Loading