Skip to content
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import org.apache.spark.sql.catalyst.encoders.RowEncoder
import org.apache.spark.sql.catalyst.expressions.SpecificInternalRow
import org.apache.spark.sql.catalyst.parser.CatalystSqlParser
import org.apache.spark.sql.catalyst.util.{CaseInsensitiveMap, DateTimeUtils, GenericArrayData}
import org.apache.spark.sql.connector.catalog.TableChange
import org.apache.spark.sql.execution.datasources.jdbc.connection.ConnectionProvider
import org.apache.spark.sql.jdbc.{JdbcDialect, JdbcDialects, JdbcType}
import org.apache.spark.sql.types._
Expand Down Expand Up @@ -94,13 +95,7 @@ object JdbcUtils extends Logging {
* Drops a table from the JDBC database.
*/
def dropTable(conn: Connection, table: String, options: JDBCOptions): Unit = {
val statement = conn.createStatement
try {
statement.setQueryTimeout(options.queryTimeout)
statement.executeUpdate(s"DROP TABLE $table")
} finally {
statement.close()
}
executeStatement(conn, options, s"DROP TABLE $table")
}

/**
Expand Down Expand Up @@ -184,7 +179,7 @@ object JdbcUtils extends Logging {
}
}

private def getJdbcType(dt: DataType, dialect: JdbcDialect): JdbcType = {
private[sql] def getJdbcType(dt: DataType, dialect: JdbcDialect): JdbcType = {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's okay to remove private[sql] because execution is already in the private package (see also SPARK-16964)

dialect.getJDBCType(dt).orElse(getCommonJDBCType(dt)).getOrElse(
throw new IllegalArgumentException(s"Can't get JDBC type for ${dt.catalogString}"))
}
Expand Down Expand Up @@ -882,13 +877,7 @@ object JdbcUtils extends Logging {
// table_options or partition_options.
// E.g., "CREATE TABLE t (name string) ENGINE=InnoDB DEFAULT CHARSET=utf8"
val sql = s"CREATE TABLE $tableName ($strSchema) $createTableOptions"
val statement = conn.createStatement
try {
statement.setQueryTimeout(options.queryTimeout)
statement.executeUpdate(sql)
} finally {
statement.close()
}
executeStatement(conn, options, sql)
}

/**
Expand All @@ -900,10 +889,41 @@ object JdbcUtils extends Logging {
newTable: String,
options: JDBCOptions): Unit = {
val dialect = JdbcDialects.get(options.url)
executeStatement(conn, options, dialect.renameTable(oldTable, newTable))
}

/**
* Update a table from the JDBC database.
*/
def alterTable(
conn: Connection,
tableName: String,
changes: Seq[TableChange],
options: JDBCOptions): Unit = {
val dialect = JdbcDialects.get(options.url)
conn.setAutoCommit(false)
val statement = conn.createStatement
try {
statement.setQueryTimeout(options.queryTimeout)
Comment on lines +923 to +935

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other methods have similar code:

      val statement = ...
      try {
        statement.setQueryTimeout(options.queryTimeout)
        statement.execute ...
      } finally {
        statement.close()
      }

Could you put it to a private method.

statement.executeUpdate(dialect.renameTable(oldTable, newTable))
for (sql <- dialect.alterTable(tableName, changes)) {
statement.executeUpdate(sql)
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am debating if I should use statement.executeBatch. The code is simpler without using batch. Not sure if it is common for user to add lots of columns at one time.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if one of the statements fails? Do we leave the table in partially modified state? Should we perform all the statements atomically?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

conn.commit()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cloud-fan proposed in off-line discussions to do not use transaction if changes.length == 1

} catch {
case e: SQLException =>
if (conn != null) conn.rollback()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if all JDBC servers support it. At least Spark thriftserver doesn't support it. How about we limit the scope to only support ALTER TABLE when changes.length == 1? Then we don't need to worry about atomic issues.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would check changes.length == 1 only if supportsTransactions() is false.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that also works, but may require more test cases (one more dimension to test)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My tests have changes.length == 1 (ALTER TABLE h2.test.alt_table ADD COLUMNS (C3 DOUBLE)) and changes.length == 2 (ALTER TABLE h2.test.alt_table ADD COLUMNS (C1 INTEGER, C2 STRING))

throw e
} finally {
statement.close()
conn.setAutoCommit(true)
}
}

private def executeStatement(conn: Connection, options: JDBCOptions, sql: String): Unit = {
val statement = conn.createStatement
try {
statement.setQueryTimeout(options.queryTimeout)
statement.executeUpdate(sql)
} finally {
statement.close()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,11 +129,12 @@ class JDBCTableCatalog extends TableCatalog with Logging {
JDBCTable(ident, schema, writeOptions)
}

// TODO (SPARK-32402): Implement ALTER TABLE in JDBC Table Catalog
override def alterTable(ident: Identifier, changes: TableChange*): Table = {
// scalastyle:off throwerror
throw new NotImplementedError()
// scalastyle:on throwerror
checkNamespace(ident.namespace())
withConnection { conn =>
JdbcUtils.alterTable(conn, getTableName(ident), changes, options)
loadTable(ident)
}
}

private def checkNamespace(namespace: Array[String]): Unit = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,14 @@ package org.apache.spark.sql.jdbc

import java.sql.{Connection, Date, 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._

/**
Expand Down Expand Up @@ -184,15 +189,65 @@ 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.
*
* @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 =>
add.fieldNames match {
case Array(name) =>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should have a better error message if the field name has more than one parts.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to me fieldName always has only one element.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do believe it always has one element, maybe add an assert?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People can alter a nested field, that why the type of fieldNames is Array[String]. e.g. ALTER TABLE t RENAME COLUMN a.b TO c

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this look OK?

            case _ =>
              throw new SQLFeatureNotSupportedException("Nested column is not supported.")

@cloud-fan cloud-fan Aug 4, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add.fieldNames match {
            case Array(name) =>

We will fail at this pattern match.

One way is

case add: AddColumn if add.fieldNames.length == 1 =>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cloud-fan I changed code to what you suggested.
We don't support nested column in JDBC yet. In JdbcUtils.getCatalystType, we map java.sql.Types.STRUCT => StringType. In getCommonJDBCType, we don't have a match case for StructType and the default is None. Seems to me currently it is not possible to reach the code path of multiple parts fieldNames in JdbcDialects.alterTable, so I will not have a negative test case for this code path for now.

val dataType = JdbcUtils.getJdbcType(add.dataType(), this).databaseTypeDefinition
updateClause += s"ALTER TABLE $tableName ADD COLUMN $name $dataType"
}
Comment thread
cloud-fan marked this conversation as resolved.
Outdated
case rename: RenameColumn =>
rename.fieldNames match {
case Array(name) =>
updateClause += s"ALTER TABLE $tableName RENAME COLUMN $name TO ${rename.newName}"
}
case delete: DeleteColumn =>
delete.fieldNames match {
case Array(name) =>
updateClause += s"ALTER TABLE $tableName DROP COLUMN $name"
}
case update: UpdateColumnType =>
update.fieldNames match {
case Array(name) =>
val dataType = JdbcUtils.getJdbcType(update.newDataType(), this)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We know fieldNames must be one element now. We don't need match and can just access fieldNames(0).

.databaseTypeDefinition
updateClause += s"ALTER TABLE $tableName ALTER COLUMN $name $dataType"
}
case update: UpdateColumnNullability =>
update.fieldNames match {
case Array(name) =>
if (update.nullable()) {
updateClause += s"ALTER TABLE $tableName ALTER COLUMN $name SET NULL"
} else {
updateClause += s"ALTER TABLE $tableName ALTER COLUMN $name SET NOT NULL"
}
Comment thread
huaxingao marked this conversation as resolved.
Outdated
}
// scalastyle:off throwerror
case _ => throw new NotImplementedError(s"JDBC alterTable has Unsupported" +
s" TableChange $change")
// scalastyle:on throwerror
}
}
updateClause.result()
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ package org.apache.spark.sql.execution.datasources.v2.jdbc
import java.sql.{Connection, DriverManager}
import java.util.Properties

import scala.collection.mutable.ArrayBuilder

import org.apache.spark.SparkConf
import org.apache.spark.sql.{QueryTest, Row}
import org.apache.spark.sql.test.SharedSparkSession
Expand Down Expand Up @@ -106,4 +108,70 @@ class JDBCTableCatalogSuite extends QueryTest with SharedSparkSession {
Seq(Row("test", "people"), Row("test", "new_table")))
}
}

test("alter table ... add column") {
withTable("h2.test.alt_table") {
withConnection { conn =>
sql("CREATE TABLE h2.test.alt_table (ID INTEGER) USING _")
}
Comment thread
huaxingao marked this conversation as resolved.
Outdated
assert(checkColumnExistence("h2.test.alt_table", Array("ID")))
sql("ALTER TABLE h2.test.alt_table ADD COLUMNS (C1 INTEGER, C2 STRING)")
assert(checkColumnExistence("h2.test.alt_table", Array("ID", "C1", "C2")))
sql("ALTER TABLE h2.test.alt_table ADD COLUMNS (C3 DOUBLE)")
assert(checkColumnExistence("h2.test.alt_table", Array("ID", "C1", "C2", "C3")))
}
}

test("alter table ... rename column") {
withTable("h2.test.alt_table") {
withConnection { conn =>
sql("CREATE TABLE h2.test.alt_table (ID INTEGER) USING _")
}
Comment thread
huaxingao marked this conversation as resolved.
Outdated
assert(checkColumnExistence("h2.test.alt_table", Array("ID")))
sql("ALTER TABLE h2.test.alt_table RENAME COLUMN ID TO C")
assert(checkColumnExistence("h2.test.alt_table", Array("C")))
}
}

test("alter table ... drop column") {
withTable("h2.test.alt_table") {
withConnection { conn =>
sql("CREATE TABLE h2.test.alt_table (C1 INTEGER, C2 INTEGER) USING _")
}
Comment thread
huaxingao marked this conversation as resolved.
Outdated
assert(checkColumnExistence("h2.test.alt_table", Array("C1", "C2")))
sql("ALTER TABLE h2.test.alt_table DROP COLUMN C1")
assert(checkColumnExistence("h2.test.alt_table", Array("C2")))
}
}

test("alter table ... update column type") {
Comment thread
cloud-fan marked this conversation as resolved.
withTable("h2.test.alt_table") {
withConnection { conn =>
sql("CREATE TABLE h2.test.alt_table (ID INTEGER) USING _")
}
Comment thread
huaxingao marked this conversation as resolved.
Outdated
sql("ALTER TABLE h2.test.alt_table ALTER COLUMN id TYPE DOUBLE")
assert(sql(s"DESCRIBE TABLE h2.test.alt_table").select("data_type").first()
=== Row("double"))
}
}

test("alter table ... update column comment not supported") {
withTable("h2.test.alt_table") {
withConnection { conn =>
sql("CREATE TABLE h2.test.alt_table (ID INTEGER) USING _")
}
Comment thread
huaxingao marked this conversation as resolved.
Outdated
val thrown = intercept[scala.NotImplementedError] {
sql("ALTER TABLE h2.test.alt_table ALTER COLUMN ID COMMENT 'test'")
}
assert(thrown.getMessage.contains("JDBC alterTable has Unsupported TableChange"))
}
}

private def checkColumnExistence(tableName: String, columns: Array[String]): Boolean = {
val rows = ArrayBuilder.make[Row]
for (column <- columns) {
rows += Row(column)
}
sql(s"DESCRIBE TABLE $tableName").select("col_name").take(columns.length) === rows.result()
}
}