Skip to content
Closed
Show file tree
Hide file tree
Changes from all 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 @@ -168,8 +168,8 @@ statement
DROP (IF EXISTS)? partitionSpec (',' partitionSpec)* PURGE? #dropTablePartitions
| ALTER VIEW tableIdentifier
DROP (IF EXISTS)? partitionSpec (',' partitionSpec)* #dropTablePartitions
| ALTER TABLE multipartIdentifier SET locationSpec #setTableLocation
| ALTER TABLE tableIdentifier partitionSpec SET locationSpec #setPartitionLocation
| ALTER TABLE multipartIdentifier
(partitionSpec)? SET locationSpec #setTableLocation
Copy link
Contributor Author

@imback82 imback82 Oct 30, 2019

Choose a reason for hiding this comment

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

Please let me know if this needs to be parsed separately.

| ALTER TABLE multipartIdentifier RECOVER PARTITIONS #recoverPartitions
| DROP TABLE (IF EXISTS)? multipartIdentifier PURGE? #dropTable
| DROP VIEW (IF EXISTS)? multipartIdentifier #dropView
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,11 @@ class ResolveCatalogs(val catalogManager: CatalogManager)
createAlterTable(nameParts, catalog, tableName, changes)

case AlterTableSetLocationStatement(
nameParts @ NonSessionCatalog(catalog, tableName), newLoc) =>
nameParts @ NonSessionCatalog(catalog, tableName), partitionSpec, newLoc) =>
if (partitionSpec.nonEmpty) {
throw new AnalysisException(
"ALTER TABLE SET LOCATION does not support partition for v2 tables.")
}
val changes = Seq(TableChange.setProperty("location", newLoc))
createAlterTable(nameParts, catalog, tableName, changes)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2717,6 +2717,7 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging
override def visitSetTableLocation(ctx: SetTableLocationContext): LogicalPlan = withOrigin(ctx) {
AlterTableSetLocationStatement(
visitMultipartIdentifier(ctx.multipartIdentifier),
Option(ctx.partitionSpec).map(visitNonOptionalPartitionSpec),
visitLocationSpec(ctx.locationSpec))
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ case class AlterTableUnsetPropertiesStatement(
*/
case class AlterTableSetLocationStatement(
tableName: Seq[String],
partitionSpec: Option[TablePartitionSpec],
Copy link
Contributor Author

@imback82 imback82 Oct 30, 2019

Choose a reason for hiding this comment

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

I am piggybacking on the existing AlterTableSetLocationStatement (after looking at how AlterTableSetLocationCommand is doing). Please let me know if this is not what we want and I will create a separate statement for partition.

location: String) extends ParsedStatement

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -540,10 +540,16 @@ class DDLParserSuite extends AnalysisTest {
}

test("alter table: set location") {
val sql1 = "ALTER TABLE table_name SET LOCATION 'new location'"
val parsed1 = parsePlan(sql1)
val expected1 = AlterTableSetLocationStatement(Seq("table_name"), "new location")
comparePlans(parsed1, expected1)
comparePlans(
parsePlan("ALTER TABLE a.b.c SET LOCATION 'new location'"),
AlterTableSetLocationStatement(Seq("a", "b", "c"), None, "new location"))

comparePlans(
parsePlan("ALTER TABLE a.b.c PARTITION(ds='2017-06-10') SET LOCATION 'new location'"),
AlterTableSetLocationStatement(
Seq("a", "b", "c"),
Some(Map("ds" -> "2017-06-10")),
"new location"))
}

test("alter table: rename column") {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,15 @@ class ResolveSessionCatalog(
}

case AlterTableSetLocationStatement(
nameParts @ SessionCatalog(catalog, tableName), newLoc) =>
nameParts @ SessionCatalog(catalog, tableName), partitionSpec, newLoc) =>
loadTable(catalog, tableName.asIdentifier).collect {
case v1Table: V1Table =>
AlterTableSetLocationCommand(tableName.asTableIdentifier, None, newLoc)
AlterTableSetLocationCommand(tableName.asTableIdentifier, partitionSpec, newLoc)
}.getOrElse {
if (partitionSpec.nonEmpty) {
throw new AnalysisException(
"ALTER TABLE SET LOCATION does not support partition for v2 tables.")
}
val changes = Seq(TableChange.setProperty("location", newLoc))
createAlterTable(nameParts, catalog, tableName, changes)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -515,22 +515,6 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
retainData = false)
}

/**
* Create an [[AlterTableSetLocationCommand]] command for a partition.
*
* For example:
* {{{
* ALTER TABLE table PARTITION spec SET LOCATION "loc";
* }}}
*/
override def visitSetPartitionLocation(
ctx: SetPartitionLocationContext): LogicalPlan = withOrigin(ctx) {
AlterTableSetLocationCommand(
visitTableIdentifier(ctx.tableIdentifier),
Some(visitNonOptionalPartitionSpec(ctx.partitionSpec)),
visitLocationSpec(ctx.locationSpec))
}

/**
* Create a [[AlterTableChangeColumnCommand]] command.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -816,6 +816,19 @@ trait AlterTableTests extends SharedSparkSession {
}
}

test("AlterTable: set partition location") {
val t = s"${catalogAndNamespace}table_name"
withTable(t) {
sql(s"CREATE TABLE $t (id int) USING $v2Format")

val exc = intercept[AnalysisException] {
sql(s"ALTER TABLE $t PARTITION(ds='2017-06-10') SET LOCATION 's3://bucket/path'")
}
assert(exc.getMessage.contains(
"ALTER TABLE SET LOCATION does not support partition for v2 tables"))
}
}

test("AlterTable: set table property") {
val t = s"${catalogAndNamespace}table_name"
withTable(t) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,14 +136,21 @@ abstract class SQLViewSuite extends QueryTest with SQLTestUtils {
assertNoSuchTable(s"ALTER TABLE $viewName SET SERDE 'whatever'")
assertNoSuchTable(s"ALTER TABLE $viewName PARTITION (a=1, b=2) SET SERDE 'whatever'")
assertNoSuchTable(s"ALTER TABLE $viewName SET SERDEPROPERTIES ('p' = 'an')")
assertNoSuchTable(s"ALTER TABLE $viewName PARTITION (a='4') SET LOCATION '/path/to/home'")
assertNoSuchTable(s"ALTER TABLE $viewName ADD IF NOT EXISTS PARTITION (a='4', b='8')")
assertNoSuchTable(s"ALTER TABLE $viewName DROP PARTITION (a='4', b='8')")
assertNoSuchTable(s"ALTER TABLE $viewName PARTITION (a='4') RENAME TO PARTITION (a='5')")
assertNoSuchTable(s"ALTER TABLE $viewName RECOVER PARTITIONS")

// For v2 ALTER TABLE statements, we have better error message saying view is not supported.
assertViewNotSupported(s"ALTER TABLE $viewName SET LOCATION '/path/to/your/lovely/heart'")
assertAnalysisError(
s"ALTER TABLE $viewName SET LOCATION '/path/to/your/lovely/heart'",
s"'$viewName' is a view not a table")

// For the following v2 ALERT TABLE statements, unsupported operations are checked first
// before resolving the relations.
assertAnalysisError(
s"ALTER TABLE $viewName PARTITION (a='4') SET LOCATION '/path/to/home'",
"ALTER TABLE SET LOCATION does not support partition for v2 tables")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cloud-fan, If we need to retain the same behavior to throw an exception with "'$viewName' is a view not a table" message, we need to pass partitionSpec to AlterTable, then check if partitionSpec is set inside AlterTableExec. I don't think it's necessary, but please let me know if that is preferred.

Copy link
Contributor

Choose a reason for hiding this comment

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

looks reasonable to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume you meant the current approach looks reasonable, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

yea, the unsupported feature should be reported first.

}
}

Expand Down Expand Up @@ -177,9 +184,9 @@ abstract class SQLViewSuite extends QueryTest with SQLTestUtils {
}
}

private def assertViewNotSupported(query: String): Unit = {
private def assertAnalysisError(query: String, message: String): Unit = {
val e = intercept[AnalysisException](sql(query))
assert(e.message.contains("'testView' is a view not a table"))
assert(e.message.contains(message))
}

test("error handling: insert/load/truncate table commands against a view") {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -635,18 +635,6 @@ class DDLParserSuite extends AnalysisTest with SharedSparkSession {
"SET FILEFORMAT PARQUET")
}

test("alter table: set partition location") {
val sql2 = "ALTER TABLE table_name PARTITION (dt='2008-08-08', country='us') " +
"SET LOCATION 'new location'"
val parsed2 = parser.parsePlan(sql2)
val tableIdent = TableIdentifier("table_name", None)
val expected2 = AlterTableSetLocationCommand(
tableIdent,
Some(Map("dt" -> "2008-08-08", "country" -> "us")),
"new location")
comparePlans(parsed2, expected2)
}

test("alter table: change column name/type/comment") {
val sql1 = "ALTER TABLE table_name CHANGE COLUMN col_old_name col_new_name INT"
val sql2 = "ALTER TABLE table_name CHANGE COLUMN col_name col_name INT COMMENT 'new_comment'"
Expand Down