Skip to content
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

[SPARK-33714][SQL] Migrate ALTER VIEW ... SET/UNSET TBLPROPERTIES commands to use UnresolvedView to resolve the identifier #30676

Closed
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -889,8 +889,11 @@ class Analyzer(override val catalogManager: CatalogManager)
u.failAnalysis(s"${ident.quoted} is a temp view. '$cmd' expects a table")
}
u
case u @ UnresolvedView(ident, _, _) =>
case u @ UnresolvedView(ident, cmd, allowTemp, _) =>
lookupTempView(ident).map { _ =>
if (!allowTemp) {
u.failAnalysis(s"${ident.quoted} is a temp view. '$cmd' expects a permanent view.")
}
ResolvedView(ident.asIdentifier, isTemp = true)
}
.getOrElse(u)
Expand Down Expand Up @@ -1118,7 +1121,7 @@ class Analyzer(override val catalogManager: CatalogManager)
case table => table
}.getOrElse(u)

case u @ UnresolvedView(identifier, cmd, relationTypeMismatchHint) =>
case u @ UnresolvedView(identifier, cmd, _, relationTypeMismatchHint) =>
lookupTableOrView(identifier).map {
case v: ResolvedView => v
case _ =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ trait CheckAnalysis extends PredicateHelper with LookupCatalog {
case u: UnresolvedTable =>
u.failAnalysis(s"Table not found for '${u.commandName}': ${u.multipartIdentifier.quoted}")

case u @ UnresolvedView(NonSessionCatalogAndIdentifier(catalog, ident), cmd, _) =>
case u @ UnresolvedView(NonSessionCatalogAndIdentifier(catalog, ident), cmd, _, _) =>
u.failAnalysis(
s"Cannot specify catalog `${catalog.name}` for view ${ident.quoted} " +
"because view support in v2 catalog has not been implemented yet. " +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,18 +121,6 @@ class ResolveCatalogs(val catalogManager: CatalogManager)
val changes = Seq(TableChange.setProperty(TableCatalog.PROP_LOCATION, newLoc))
createAlterTable(nameParts, catalog, tbl, changes)

case AlterViewSetPropertiesStatement(
NonSessionCatalogAndTable(catalog, tbl), props) =>
throw new AnalysisException(
s"Can not specify catalog `${catalog.name}` for view ${tbl.quoted} " +
s"because view support in catalog has not been implemented yet")

case AlterViewUnsetPropertiesStatement(
NonSessionCatalogAndTable(catalog, tbl), keys, ifExists) =>
throw new AnalysisException(
s"Can not specify catalog `${catalog.name}` for view ${tbl.quoted} " +
s"because view support in catalog has not been implemented yet")

case c @ CreateTableStatement(
NonSessionCatalogAndTable(catalog, tbl), _, _, _, _, _, _, _, _, _, _, _) =>
assertNoNullTypeInSchema(c.tableSchema)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ case class UnresolvedTable(
case class UnresolvedView(
multipartIdentifier: Seq[String],
commandName: String,
allowTemp: Boolean = true,
relationTypeMismatchHint: Option[String] = None) extends LeafNode {
override lazy val resolved: Boolean = false

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3161,8 +3161,9 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
DropView(
UnresolvedView(
visitMultipartIdentifier(ctx.multipartIdentifier()),
"DROP VIEW",
Some("Please use DROP TABLE instead.")),
commandName = "DROP VIEW",
allowTemp = true,
relationTypeMismatchHint = Some("Please use DROP TABLE instead.")),
ctx.EXISTS != null)
}

Expand Down Expand Up @@ -3399,7 +3400,7 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
}

/**
* Parse [[AlterViewSetPropertiesStatement]] or [[AlterTableSetPropertiesStatement]] commands.
* Parse [[AlterViewSetProperties]] or [[AlterTableSetPropertiesStatement]] commands.
*
* For example:
* {{{
Expand All @@ -3413,14 +3414,20 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
val properties = visitPropertyKeyValues(ctx.tablePropertyList)
val cleanedTableProperties = cleanTableProperties(ctx, properties)
if (ctx.VIEW != null) {
AlterViewSetPropertiesStatement(identifier, cleanedTableProperties)
AlterViewSetProperties(
UnresolvedView(
identifier,
commandName = "ALTER VIEW ... SET TBLPROPERTIES",
allowTemp = false,
relationTypeMismatchHint = Some("Please use ALTER TABLE instead.")),
cleanedTableProperties)
} else {
AlterTableSetPropertiesStatement(identifier, cleanedTableProperties)
}
}

/**
* Parse [[AlterViewUnsetPropertiesStatement]] or [[AlterTableUnsetPropertiesStatement]] commands.
* Parse [[AlterViewUnsetProperties]] or [[AlterTableUnsetPropertiesStatement]] commands.
*
* For example:
* {{{
Expand All @@ -3436,7 +3443,14 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg

val ifExists = ctx.EXISTS != null
if (ctx.VIEW != null) {
AlterViewUnsetPropertiesStatement(identifier, cleanedProperties, ifExists)
AlterViewUnsetProperties(
UnresolvedView(
identifier,
commandName = "ALTER VIEW ... UNSET TBLPROPERTIES",
allowTemp = false,
relationTypeMismatchHint = Some("Please use ALTER TABLE instead.")),
cleanedProperties,
ifExists)
} else {
AlterTableUnsetPropertiesStatement(identifier, cleanedProperties, ifExists)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -315,21 +315,6 @@ case class AlterTableSerDePropertiesStatement(
serdeProperties: Option[Map[String, String]],
partitionSpec: Option[TablePartitionSpec]) extends ParsedStatement

/**
* ALTER VIEW ... SET TBLPROPERTIES command, as parsed from SQL.
*/
case class AlterViewSetPropertiesStatement(
viewName: Seq[String],
properties: Map[String, String]) extends ParsedStatement

/**
* ALTER VIEW ... UNSET TBLPROPERTIES command, as parsed from SQL.
*/
case class AlterViewUnsetPropertiesStatement(
viewName: Seq[String],
propertyKeys: Seq[String],
ifExists: Boolean) extends ParsedStatement

/**
* ALTER VIEW ... Query command, as parsed from SQL.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -742,3 +742,22 @@ case class DropView(
case class RepairTable(child: LogicalPlan) extends Command {
override def children: Seq[LogicalPlan] = child :: Nil
}

/**
* The logical plan of the ALTER VIEW ... SET TBLPROPERTIES command.
*/
case class AlterViewSetProperties(
child: LogicalPlan,
properties: Map[String, String]) extends Command {
override def children: Seq[LogicalPlan] = child :: Nil
}

/**
* The logical plan of the ALTER VIEW ... UNSET TBLPROPERTIES command.
*/
case class AlterViewUnsetProperties(
child: LogicalPlan,
propertyKeys: Seq[String],
ifExists: Boolean) extends Command {
override def children: Seq[LogicalPlan] = child :: Nil
}
Original file line number Diff line number Diff line change
Expand Up @@ -724,15 +724,15 @@ class DDLParserSuite extends AnalysisTest {
val cmd = "DROP VIEW"
val hint = Some("Please use DROP TABLE instead.")
parseCompare(s"DROP VIEW testcat.db.view",
DropView(UnresolvedView(Seq("testcat", "db", "view"), cmd, hint), ifExists = false))
DropView(UnresolvedView(Seq("testcat", "db", "view"), cmd, true, hint), ifExists = false))
parseCompare(s"DROP VIEW db.view",
DropView(UnresolvedView(Seq("db", "view"), cmd, hint), ifExists = false))
DropView(UnresolvedView(Seq("db", "view"), cmd, true, hint), ifExists = false))
parseCompare(s"DROP VIEW IF EXISTS db.view",
DropView(UnresolvedView(Seq("db", "view"), cmd, hint), ifExists = true))
DropView(UnresolvedView(Seq("db", "view"), cmd, true, hint), ifExists = true))
parseCompare(s"DROP VIEW view",
DropView(UnresolvedView(Seq("view"), cmd, hint), ifExists = false))
DropView(UnresolvedView(Seq("view"), cmd, true, hint), ifExists = false))
parseCompare(s"DROP VIEW IF EXISTS view",
DropView(UnresolvedView(Seq("view"), cmd, hint), ifExists = true))
DropView(UnresolvedView(Seq("view"), cmd, true, hint), ifExists = true))
}

private def testCreateOrReplaceDdl(
Expand Down Expand Up @@ -764,16 +764,22 @@ class DDLParserSuite extends AnalysisTest {
"'comment' = 'new_comment')"
val sql2_view = "ALTER VIEW table_name UNSET TBLPROPERTIES ('comment', 'test')"
val sql3_view = "ALTER VIEW table_name UNSET TBLPROPERTIES IF EXISTS ('comment', 'test')"
val hint = Some("Please use ALTER TABLE instead.")

comparePlans(parsePlan(sql1_view),
AlterViewSetPropertiesStatement(
Seq("table_name"), Map("test" -> "test", "comment" -> "new_comment")))
AlterViewSetProperties(
UnresolvedView(Seq("table_name"), "ALTER VIEW ... SET TBLPROPERTIES", false, hint),
Map("test" -> "test", "comment" -> "new_comment")))
comparePlans(parsePlan(sql2_view),
AlterViewUnsetPropertiesStatement(
Seq("table_name"), Seq("comment", "test"), ifExists = false))
AlterViewUnsetProperties(
UnresolvedView(Seq("table_name"), "ALTER VIEW ... UNSET TBLPROPERTIES", false, hint),
Seq("comment", "test"),
ifExists = false))
comparePlans(parsePlan(sql3_view),
AlterViewUnsetPropertiesStatement(
Seq("table_name"), Seq("comment", "test"), ifExists = true))
AlterViewUnsetProperties(
UnresolvedView(Seq("table_name"), "ALTER VIEW ... UNSET TBLPROPERTIES", false, hint),
Seq("comment", "test"),
ifExists = true))
}

// ALTER TABLE table_name SET TBLPROPERTIES ('comment' = new_comment);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,12 +209,11 @@ class ResolveSessionCatalog(
createAlterTable(nameParts, catalog, tbl, changes)
}

// ALTER VIEW should always use v1 command if the resolved catalog is session catalog.
case AlterViewSetPropertiesStatement(SessionCatalogAndTable(_, tbl), props) =>
AlterTableSetPropertiesCommand(tbl.asTableIdentifier, props, isView = true)
case AlterViewSetProperties(ResolvedView(ident, _), props) =>
AlterTableSetPropertiesCommand(ident.asTableIdentifier, props, isView = true)

case AlterViewUnsetPropertiesStatement(SessionCatalogAndTable(_, tbl), keys, ifExists) =>
AlterTableUnsetPropertiesCommand(tbl.asTableIdentifier, keys, ifExists, isView = true)
case AlterViewUnsetProperties(ResolvedView(ident, _), keys, ifExists) =>
AlterTableUnsetPropertiesCommand(ident.asTableIdentifier, keys, ifExists, isView = true)

case d @ DescribeNamespace(SessionCatalogAndNamespace(_, ns), _) =>
if (ns.length != 1) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2594,11 +2594,29 @@ class DataSourceV2SQLSuite
}
}

test("DROP VIEW is not supported for v2 catalogs") {
assertAnalysisError(
"DROP VIEW testcat.v",
"Cannot specify catalog `testcat` for view v because view support in v2 catalog " +
"has not been implemented yet. DROP VIEW expects a view.")
test("View commands are not supported in v2 catalogs") {
def validateViewCommand(
sql: String,
catalogName: String,
viewName: String,
cmdName: String): Unit = {
assertAnalysisError(
sql,
s"Cannot specify catalog `$catalogName` for view $viewName because view support " +
s"in v2 catalog has not been implemented yet. $cmdName expects a view.")
}

validateViewCommand("DROP VIEW testcat.v", "testcat", "v", "DROP VIEW")
validateViewCommand(
"ALTER VIEW testcat.v SET TBLPROPERTIES ('key' = 'val')",
"testcat",
"v",
"ALTER VIEW ... SET TBLPROPERTIES")
validateViewCommand(
"ALTER VIEW testcat.v UNSET TBLPROPERTIES ('key')",
"testcat",
"v",
"ALTER VIEW ... UNSET TBLPROPERTIES")
}

private def testNotSupportedV2Command(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,12 @@ abstract class SQLViewSuite extends QueryTest with SQLTestUtils {
val viewName = "testView"
withTempView(viewName) {
spark.range(10).createTempView(viewName)
assertNoSuchTable(s"ALTER VIEW $viewName SET TBLPROPERTIES ('p' = 'an')")
assertNoSuchTable(s"ALTER VIEW $viewName UNSET TBLPROPERTIES ('p')")
assertAnalysisError(
s"ALTER VIEW $viewName SET TBLPROPERTIES ('p' = 'an')",
"testView is a temp view. 'ALTER VIEW ... SET TBLPROPERTIES' expects a permanent view.")
assertAnalysisError(
s"ALTER VIEW $viewName UNSET TBLPROPERTIES ('p')",
"testView is a temp view. 'ALTER VIEW ... UNSET TBLPROPERTIES' expects a permanent view.")
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -721,16 +721,16 @@ class PlanResolutionSuite extends AnalysisTest {
// ALTER VIEW view_name SET TBLPROPERTIES ('comment' = new_comment);
// ALTER VIEW view_name UNSET TBLPROPERTIES [IF EXISTS] ('comment', 'key');
test("alter view: alter view properties") {
val sql1_view = "ALTER VIEW table_name SET TBLPROPERTIES ('test' = 'test', " +
val sql1_view = "ALTER VIEW view SET TBLPROPERTIES ('test' = 'test', " +
"'comment' = 'new_comment')"
val sql2_view = "ALTER VIEW table_name UNSET TBLPROPERTIES ('comment', 'test')"
val sql3_view = "ALTER VIEW table_name UNSET TBLPROPERTIES IF EXISTS ('comment', 'test')"
val sql2_view = "ALTER VIEW view UNSET TBLPROPERTIES ('comment', 'test')"
val sql3_view = "ALTER VIEW view UNSET TBLPROPERTIES IF EXISTS ('comment', 'test')"

val parsed1_view = parseAndResolve(sql1_view)
val parsed2_view = parseAndResolve(sql2_view)
val parsed3_view = parseAndResolve(sql3_view)

val tableIdent = TableIdentifier("table_name", Some("default"))
val tableIdent = TableIdentifier("view", Some("default"))
val expected1_view = AlterTableSetPropertiesCommand(
tableIdent, Map("test" -> "test", "comment" -> "new_comment"), isView = true)
val expected2_view = AlterTableUnsetPropertiesCommand(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -879,11 +879,17 @@ class HiveDDLSuite

assertErrorForAlterTableOnView(s"ALTER TABLE $oldViewName RENAME TO $newViewName")

assertErrorForAlterViewOnTable(s"ALTER VIEW $tabName SET TBLPROPERTIES ('p' = 'an')")
assertAnalysisError(
s"ALTER VIEW $tabName SET TBLPROPERTIES ('p' = 'an')",
s"$tabName is a table. 'ALTER VIEW ... SET TBLPROPERTIES' expects a view. " +
"Please use ALTER TABLE instead.")

assertErrorForAlterTableOnView(s"ALTER TABLE $oldViewName SET TBLPROPERTIES ('p' = 'an')")

assertErrorForAlterViewOnTable(s"ALTER VIEW $tabName UNSET TBLPROPERTIES ('p')")
assertAnalysisError(
s"ALTER VIEW $tabName UNSET TBLPROPERTIES ('p')",
s"$tabName is a table. 'ALTER VIEW ... UNSET TBLPROPERTIES' expects a view. " +
"Please use ALTER TABLE instead.")

assertErrorForAlterTableOnView(s"ALTER TABLE $oldViewName UNSET TBLPROPERTIES ('p')")

Expand Down