Skip to content

Commit 3ccb23e

Browse files
cloud-fancloud-fan
committed
[SPARK-17394][SQL] should not allow specify database in table/view name after RENAME TO
## What changes were proposed in this pull request? It's really weird that we allow users to specify database in both from table name and to table name in `ALTER TABLE RENAME TO`, while logically we can't support rename a table to a different database. Both postgres and MySQL disallow this syntax, it's reasonable to follow them and simply our code. ## How was this patch tested? new test in `DDLCommandSuite` Author: Wenchen Fan <[email protected]> Closes #14955 from cloud-fan/rename.
1 parent c1e9a6d commit 3ccb23e

File tree

6 files changed

+32
-72
lines changed

6 files changed

+32
-72
lines changed

‎sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala‎

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -349,29 +349,17 @@ class SessionCatalog(
349349
* If a database is specified in `oldName`, this will rename the table in that database.
350350
* If no database is specified, this will first attempt to rename a temporary table with
351351
* the same name, then, if that does not exist, rename the table in the current database.
352-
*
353-
* This assumes the database specified in `oldName` matches the one specified in `newName`.
354352
*/
355-
def renameTable(oldName: TableIdentifier, newName: TableIdentifier): Unit = synchronized {
353+
def renameTable(oldName: TableIdentifier, newName: String): Unit = synchronized {
356354
val db = formatDatabaseName(oldName.database.getOrElse(currentDb))
357355
requireDbExists(db)
358-
val newDb = formatDatabaseName(newName.database.getOrElse(currentDb))
359-
if (db != newDb) {
360-
throw new AnalysisException(
361-
s"RENAME TABLE source and destination databases do not match: '$db' != '$newDb'")
362-
}
363356
val oldTableName = formatTableName(oldName.table)
364-
val newTableName = formatTableName(newName.table)
357+
val newTableName = formatTableName(newName)
365358
if (oldName.database.isDefined || !tempTables.contains(oldTableName)) {
366359
requireTableExists(TableIdentifier(oldTableName, Some(db)))
367360
requireTableNotExists(TableIdentifier(newTableName, Some(db)))
368361
externalCatalog.renameTable(db, oldTableName, newTableName)
369362
} else {
370-
if (newName.database.isDefined) {
371-
throw new AnalysisException(
372-
s"RENAME TEMPORARY TABLE from '$oldName' to '$newName': cannot specify database " +
373-
s"name '${newName.database.get}' in the destination table")
374-
}
375363
if (tempTables.contains(newTableName)) {
376364
throw new AnalysisException(
377365
s"RENAME TEMPORARY TABLE from '$oldName' to '$newName': destination table already exists")

‎sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala‎

Lines changed: 8 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -273,37 +273,27 @@ class SessionCatalogSuite extends SparkFunSuite {
273273
val externalCatalog = newBasicCatalog()
274274
val sessionCatalog = new SessionCatalog(externalCatalog)
275275
assert(externalCatalog.listTables("db2").toSet == Set("tbl1", "tbl2"))
276-
sessionCatalog.renameTable(
277-
TableIdentifier("tbl1", Some("db2")), TableIdentifier("tblone", Some("db2")))
276+
sessionCatalog.renameTable(TableIdentifier("tbl1", Some("db2")), "tblone")
278277
assert(externalCatalog.listTables("db2").toSet == Set("tblone", "tbl2"))
279-
sessionCatalog.renameTable(
280-
TableIdentifier("tbl2", Some("db2")), TableIdentifier("tbltwo", Some("db2")))
278+
sessionCatalog.renameTable(TableIdentifier("tbl2", Some("db2")), "tbltwo")
281279
assert(externalCatalog.listTables("db2").toSet == Set("tblone", "tbltwo"))
282280
// Rename table without explicitly specifying database
283281
sessionCatalog.setCurrentDatabase("db2")
284-
sessionCatalog.renameTable(TableIdentifier("tbltwo"), TableIdentifier("table_two"))
282+
sessionCatalog.renameTable(TableIdentifier("tbltwo"), "table_two")
285283
assert(externalCatalog.listTables("db2").toSet == Set("tblone", "table_two"))
286-
// Renaming "db2.tblone" to "db1.tblones" should fail because databases don't match
287-
intercept[AnalysisException] {
288-
sessionCatalog.renameTable(
289-
TableIdentifier("tblone", Some("db2")), TableIdentifier("tblones", Some("db1")))
290-
}
291284
// The new table already exists
292285
intercept[TableAlreadyExistsException] {
293-
sessionCatalog.renameTable(
294-
TableIdentifier("tblone", Some("db2")), TableIdentifier("table_two", Some("db2")))
286+
sessionCatalog.renameTable(TableIdentifier("tblone", Some("db2")), "table_two")
295287
}
296288
}
297289

298290
test("rename table when database/table does not exist") {
299291
val catalog = new SessionCatalog(newBasicCatalog())
300292
intercept[NoSuchDatabaseException] {
301-
catalog.renameTable(
302-
TableIdentifier("tbl1", Some("unknown_db")), TableIdentifier("tbl2", Some("unknown_db")))
293+
catalog.renameTable(TableIdentifier("tbl1", Some("unknown_db")), "tbl2")
303294
}
304295
intercept[NoSuchTableException] {
305-
catalog.renameTable(
306-
TableIdentifier("unknown_table", Some("db2")), TableIdentifier("tbl2", Some("db2")))
296+
catalog.renameTable(TableIdentifier("unknown_table", Some("db2")), "tbl2")
307297
}
308298
}
309299

@@ -316,13 +306,12 @@ class SessionCatalogSuite extends SparkFunSuite {
316306
assert(sessionCatalog.getTempTable("tbl1") == Option(tempTable))
317307
assert(externalCatalog.listTables("db2").toSet == Set("tbl1", "tbl2"))
318308
// If database is not specified, temp table should be renamed first
319-
sessionCatalog.renameTable(TableIdentifier("tbl1"), TableIdentifier("tbl3"))
309+
sessionCatalog.renameTable(TableIdentifier("tbl1"), "tbl3")
320310
assert(sessionCatalog.getTempTable("tbl1").isEmpty)
321311
assert(sessionCatalog.getTempTable("tbl3") == Option(tempTable))
322312
assert(externalCatalog.listTables("db2").toSet == Set("tbl1", "tbl2"))
323313
// If database is specified, temp tables are never renamed
324-
sessionCatalog.renameTable(
325-
TableIdentifier("tbl2", Some("db2")), TableIdentifier("tbl4", Some("db2")))
314+
sessionCatalog.renameTable(TableIdentifier("tbl2", Some("db2")), "tbl4")
326315
assert(sessionCatalog.getTempTable("tbl3") == Option(tempTable))
327316
assert(sessionCatalog.getTempTable("tbl4").isEmpty)
328317
assert(externalCatalog.listTables("db2").toSet == Set("tbl1", "tbl4"))

‎sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -666,9 +666,15 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder {
666666
* }}}
667667
*/
668668
override def visitRenameTable(ctx: RenameTableContext): LogicalPlan = withOrigin(ctx) {
669+
val fromName = visitTableIdentifier(ctx.from)
670+
val toName = visitTableIdentifier(ctx.to)
671+
if (toName.database.isDefined) {
672+
operationNotAllowed("Can not specify database in table/view name after RENAME TO", ctx)
673+
}
674+
669675
AlterTableRenameCommand(
670-
visitTableIdentifier(ctx.from),
671-
visitTableIdentifier(ctx.to),
676+
fromName,
677+
toName.table,
672678
ctx.VIEW != null)
673679
}
674680

‎sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ case class CreateTableCommand(table: CatalogTable, ifNotExists: Boolean) extends
152152
*/
153153
case class AlterTableRenameCommand(
154154
oldName: TableIdentifier,
155-
newName: TableIdentifier,
155+
newName: String,
156156
isView: Boolean)
157157
extends RunnableCommand {
158158

@@ -165,6 +165,7 @@ case class AlterTableRenameCommand(
165165
if (isTemporary) {
166166
catalog.renameTable(oldName, newName)
167167
} else {
168+
val newTblName = TableIdentifier(newName, oldName.database)
168169
// If an exception is thrown here we can just assume the table is uncached;
169170
// this can happen with Hive tables when the underlying catalog is in-memory.
170171
val wasCached = Try(sparkSession.catalog.isCached(oldName.unquotedString)).getOrElse(false)
@@ -178,7 +179,7 @@ case class AlterTableRenameCommand(
178179
// For datasource tables, we also need to update the "path" serde property
179180
val table = catalog.getTableMetadata(oldName)
180181
if (DDLUtils.isDatasourceTable(table) && table.tableType == CatalogTableType.MANAGED) {
181-
val newPath = catalog.defaultTablePath(newName)
182+
val newPath = catalog.defaultTablePath(newTblName)
182183
val newTable = table.withNewStorage(
183184
serdeProperties = table.storage.properties ++ Map("path" -> newPath))
184185
catalog.alterTable(newTable)
@@ -188,7 +189,7 @@ case class AlterTableRenameCommand(
188189
catalog.refreshTable(oldName)
189190
catalog.renameTable(oldName, newName)
190191
if (wasCached) {
191-
sparkSession.catalog.cacheTable(newName.unquotedString)
192+
sparkSession.catalog.cacheTable(newTblName.unquotedString)
192193
}
193194
}
194195
Seq.empty[Row]

‎sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLCommandSuite.scala‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -388,14 +388,19 @@ class DDLCommandSuite extends PlanTest {
388388
val parsed_view = parser.parsePlan(sql_view)
389389
val expected_table = AlterTableRenameCommand(
390390
TableIdentifier("table_name", None),
391-
TableIdentifier("new_table_name", None),
391+
"new_table_name",
392392
isView = false)
393393
val expected_view = AlterTableRenameCommand(
394394
TableIdentifier("table_name", None),
395-
TableIdentifier("new_table_name", None),
395+
"new_table_name",
396396
isView = true)
397397
comparePlans(parsed_table, expected_table)
398398
comparePlans(parsed_view, expected_view)
399+
400+
val e = intercept[ParseException](
401+
parser.parsePlan("ALTER TABLE db1.tbl RENAME TO db1.tbl2")
402+
)
403+
assert(e.getMessage.contains("Can not specify database in table/view name after RENAME TO"))
399404
}
400405

401406
// ALTER TABLE table_name SET TBLPROPERTIES ('comment' = new_comment);

‎sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala‎

Lines changed: 3 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -657,19 +657,15 @@ class DDLSuite extends QueryTest with SharedSQLContext with BeforeAndAfterEach {
657657
createDatabase(catalog, "dby")
658658
createTable(catalog, tableIdent1)
659659
assert(catalog.listTables("dbx") == Seq(tableIdent1))
660-
sql("ALTER TABLE dbx.tab1 RENAME TO dbx.tab2")
660+
sql("ALTER TABLE dbx.tab1 RENAME TO tab2")
661661
assert(catalog.listTables("dbx") == Seq(tableIdent2))
662662
catalog.setCurrentDatabase("dbx")
663663
// rename without explicitly specifying database
664664
sql("ALTER TABLE tab2 RENAME TO tab1")
665665
assert(catalog.listTables("dbx") == Seq(tableIdent1))
666666
// table to rename does not exist
667667
intercept[AnalysisException] {
668-
sql("ALTER TABLE dbx.does_not_exist RENAME TO dbx.tab2")
669-
}
670-
// destination database is different
671-
intercept[AnalysisException] {
672-
sql("ALTER TABLE dbx.tab1 RENAME TO dby.tab2")
668+
sql("ALTER TABLE dbx.does_not_exist RENAME TO tab2")
673669
}
674670
}
675671

@@ -691,31 +687,6 @@ class DDLSuite extends QueryTest with SharedSQLContext with BeforeAndAfterEach {
691687
assert(spark.table("teachers").collect().toSeq == df.collect().toSeq)
692688
}
693689

694-
test("rename temporary table - destination table with database name") {
695-
withTempView("tab1") {
696-
sql(
697-
"""
698-
|CREATE TEMPORARY TABLE tab1
699-
|USING org.apache.spark.sql.sources.DDLScanSource
700-
|OPTIONS (
701-
| From '1',
702-
| To '10',
703-
| Table 'test1'
704-
|)
705-
""".stripMargin)
706-
707-
val e = intercept[AnalysisException] {
708-
sql("ALTER TABLE tab1 RENAME TO default.tab2")
709-
}
710-
assert(e.getMessage.contains(
711-
"RENAME TEMPORARY TABLE from '`tab1`' to '`default`.`tab2`': " +
712-
"cannot specify database name 'default' in the destination table"))
713-
714-
val catalog = spark.sessionState.catalog
715-
assert(catalog.listTables("default") == Seq(TableIdentifier("tab1")))
716-
}
717-
}
718-
719690
test("rename temporary table - destination table already exists") {
720691
withTempView("tab1", "tab2") {
721692
sql(
@@ -744,7 +715,7 @@ class DDLSuite extends QueryTest with SharedSQLContext with BeforeAndAfterEach {
744715
sql("ALTER TABLE tab1 RENAME TO tab2")
745716
}
746717
assert(e.getMessage.contains(
747-
"RENAME TEMPORARY TABLE from '`tab1`' to '`tab2`': destination table already exists"))
718+
"RENAME TEMPORARY TABLE from '`tab1`' to 'tab2': destination table already exists"))
748719

749720
val catalog = spark.sessionState.catalog
750721
assert(catalog.listTables("default") == Seq(TableIdentifier("tab1"), TableIdentifier("tab2")))

0 commit comments

Comments
 (0)