-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-32402][SQL] Implement ALTER TABLE in JDBC Table Catalog #29324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
1f38d8a
9164379
dc75eee
ce00d06
3672250
b868b1d
6c32593
8170ec3
81f34d9
1b2a53c
c14d508
d0e2a88
114b7a3
db266fa
a7eccc1
dfc7387
ef55b5e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,11 +17,16 @@ | |
|
|
||
| package org.apache.spark.sql.jdbc | ||
|
|
||
| import java.sql.{Connection, Date, Timestamp} | ||
| import java.sql.{Connection, Date, SQLFeatureNotSupportedException, Timestamp} | ||
|
|
||
| import scala.collection.mutable.ArrayBuilder | ||
|
|
||
| import org.apache.commons.lang3.StringUtils | ||
|
|
||
| import org.apache.spark.annotation.{DeveloperApi, Since} | ||
| import org.apache.spark.sql.connector.catalog.TableChange | ||
| import org.apache.spark.sql.connector.catalog.TableChange._ | ||
| import org.apache.spark.sql.execution.datasources.jdbc.JdbcUtils | ||
| import org.apache.spark.sql.types._ | ||
|
|
||
| /** | ||
|
|
@@ -184,15 +189,51 @@ abstract class JdbcDialect extends Serializable { | |
| /** | ||
| * Rename an existing table. | ||
| * | ||
| * TODO (SPARK-32382): Override this method in the dialects that don't support such syntax. | ||
| * | ||
| * @param oldTable The existing table. | ||
| * @param newTable New name of the table. | ||
| * @return The SQL statement to use for renaming the table. | ||
| */ | ||
| def renameTable(oldTable: String, newTable: String): String = { | ||
| s"ALTER TABLE $oldTable RENAME TO $newTable" | ||
| } | ||
|
|
||
| /** | ||
| * Alter an existing table. | ||
| * TODO (SPARK-32523): Override this method in the dialects that have different syntax. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because you will override this method in other places, not here. Remember to remove this later. :) |
||
| * | ||
| * @param tableName The name of the table to be altered. | ||
| * @param changes Changes to apply to the table. | ||
| * @return The SQL statements to use for altering the table. | ||
| */ | ||
| def alterTable(tableName: String, changes: Seq[TableChange]): Array[String] = { | ||
| val updateClause = ArrayBuilder.make[String] | ||
| for (change <- changes) { | ||
| change match { | ||
| case add: AddColumn if add.fieldNames.length == 1 => | ||
| val dataType = JdbcUtils.getJdbcType(add.dataType(), this).databaseTypeDefinition | ||
| val name = add.fieldNames | ||
| updateClause += s"ALTER TABLE $tableName ADD COLUMN ${name(0)} $dataType" | ||
| case rename: RenameColumn if rename.fieldNames.length == 1 => | ||
| val name = rename.fieldNames | ||
| updateClause += s"ALTER TABLE $tableName RENAME COLUMN ${name(0)} TO ${rename.newName}" | ||
| case delete: DeleteColumn if delete.fieldNames.length == 1 => | ||
| val name = delete.fieldNames | ||
| updateClause += s"ALTER TABLE $tableName DROP COLUMN ${name(0)}" | ||
| case updateColumnType: UpdateColumnType if updateColumnType.fieldNames.length == 1 => | ||
| val name = updateColumnType.fieldNames | ||
| val dataType = JdbcUtils.getJdbcType(updateColumnType.newDataType(), this) | ||
| .databaseTypeDefinition | ||
| updateClause += s"ALTER TABLE $tableName ALTER COLUMN ${name(0)} $dataType" | ||
| case updateNull: UpdateColumnNullability if updateNull.fieldNames.length == 1 => | ||
| val name = updateNull.fieldNames | ||
| val nullable = if (updateNull.nullable()) "NULL" else "NOT NULL" | ||
| updateClause += s"ALTER TABLE $tableName ALTER COLUMN ${name(0)} SET $nullable" | ||
| case _ => | ||
| throw new SQLFeatureNotSupportedException(s"Unsupported TableChange $change") | ||
| } | ||
| } | ||
| updateClause.result() | ||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,7 @@ import java.util.Properties | |
| import org.apache.spark.SparkConf | ||
| import org.apache.spark.sql.{QueryTest, Row} | ||
| import org.apache.spark.sql.test.SharedSparkSession | ||
| import org.apache.spark.sql.types.{IntegerType, StringType, StructType} | ||
| import org.apache.spark.sql.types._ | ||
| import org.apache.spark.util.Utils | ||
|
|
||
| class JDBCTableCatalogSuite extends QueryTest with SharedSparkSession { | ||
|
|
@@ -106,4 +106,71 @@ class JDBCTableCatalogSuite extends QueryTest with SharedSparkSession { | |
| Seq(Row("test", "people"), Row("test", "new_table"))) | ||
| } | ||
| } | ||
|
|
||
| test("alter table ... add column") { | ||
| withTable("h2.test.alt_table") { | ||
| sql("CREATE TABLE h2.test.alt_table (ID INTEGER) USING _") | ||
| sql("ALTER TABLE h2.test.alt_table ADD COLUMNS (C1 INTEGER, C2 STRING)") | ||
| var t = spark.table("h2.test.alt_table") | ||
| var expectedSchema = new StructType() | ||
| .add("ID", IntegerType) | ||
| .add("C1", IntegerType) | ||
| .add("C2", StringType) | ||
| assert(t.schema === expectedSchema) | ||
| sql("ALTER TABLE h2.test.alt_table ADD COLUMNS (C3 DOUBLE)") | ||
| t = spark.table("h2.test.alt_table") | ||
| expectedSchema = expectedSchema.add("C3", DoubleType) | ||
| assert(t.schema === expectedSchema) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's better to compare schemas than my previous method to use DESCRIBE TABLE. |
||
| } | ||
| } | ||
|
|
||
| test("alter table ... rename column") { | ||
| withTable("h2.test.alt_table") { | ||
| sql("CREATE TABLE h2.test.alt_table (ID INTEGER) USING _") | ||
| sql("ALTER TABLE h2.test.alt_table RENAME COLUMN ID TO C") | ||
| val t = spark.table("h2.test.alt_table") | ||
| val expectedSchema = new StructType().add("C", IntegerType) | ||
| assert(t.schema === expectedSchema) | ||
| } | ||
| } | ||
|
|
||
| test("alter table ... drop column") { | ||
| withTable("h2.test.alt_table") { | ||
| sql("CREATE TABLE h2.test.alt_table (C1 INTEGER, C2 INTEGER) USING _") | ||
| sql("ALTER TABLE h2.test.alt_table DROP COLUMN C1") | ||
| val t = spark.table("h2.test.alt_table") | ||
| val expectedSchema = new StructType().add("C2", IntegerType) | ||
| assert(t.schema === expectedSchema) | ||
| } | ||
| } | ||
|
|
||
| test("alter table ... update column type") { | ||
cloud-fan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| withTable("h2.test.alt_table") { | ||
| sql("CREATE TABLE h2.test.alt_table (ID INTEGER) USING _") | ||
| sql("ALTER TABLE h2.test.alt_table ALTER COLUMN id TYPE DOUBLE") | ||
| val t = spark.table("h2.test.alt_table") | ||
| val expectedSchema = new StructType().add("ID", DoubleType) | ||
| assert(t.schema === expectedSchema) | ||
| } | ||
| } | ||
|
|
||
| test("alter table ... update column nullability") { | ||
| withTable("h2.test.alt_table") { | ||
| sql("CREATE TABLE h2.test.alt_table (ID INTEGER NOT NULL) USING _") | ||
| sql("ALTER TABLE h2.test.alt_table ALTER COLUMN ID DROP NOT NULL") | ||
| val t = spark.table("h2.test.alt_table") | ||
| val expectedSchema = new StructType().add("ID", IntegerType, nullable = true) | ||
| assert(t.schema === expectedSchema) | ||
| } | ||
| } | ||
|
|
||
| test("alter table ... update column comment not supported") { | ||
| withTable("h2.test.alt_table") { | ||
| sql("CREATE TABLE h2.test.alt_table (ID INTEGER) USING _") | ||
| val thrown = intercept[java.sql.SQLFeatureNotSupportedException] { | ||
| sql("ALTER TABLE h2.test.alt_table ALTER COLUMN ID COMMENT 'test'") | ||
| } | ||
| assert(thrown.getMessage.contains("Unsupported TableChange")) | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we record auto commit status and restore back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we don't need to record the original auto commit status. The default value of autocommit is true.