Skip to content

Commit b112e2b

Browse files
imback82cloud-fan
imback82
authored and
cloud-fan
committed
[SPARK-33714][SQL] Migrate ALTER VIEW ... SET/UNSET TBLPROPERTIES commands to use UnresolvedView to resolve the identifier
### What changes were proposed in this pull request? This PR adds `allowTemp` flag to `UnresolvedView` so that `Analyzer` can check whether to resolve temp views or not. This PR also migrates `ALTER VIEW ... SET/UNSET TBLPROPERTIES` to use `UnresolvedView` to resolve the table/view identifier. This allows consistent resolution rules (temp view first, etc.) to be applied for both v1/v2 commands. More info about the consistent resolution rule proposal can be found in [JIRA](https://issues.apache.org/jira/browse/SPARK-29900) or [proposal doc](https://docs.google.com/document/d/1hvLjGA8y_W_hhilpngXVub1Ebv8RsMap986nENCFnrg/edit?usp=sharing). ### Why are the changes needed? To use `UnresolvedView` for view resolution. One benefit is that the exception message is better for `ALTER VIEW ... SET/UNSET TBLPROPERTIES`. Before, if a temp view is passed, you will just get `NoSuchTableException` with `Table or view 'tmpView' not found in database 'default'`. But with this PR, you will get more description exception message: `tmpView is a temp view. ALTER VIEW ... SET TBLPROPERTIES expects a permanent view`. ### Does this PR introduce _any_ user-facing change? The exception message changes as describe above. ### How was this patch tested? Updated existing tests. Closes #30676 from imback82/alter_view_set_unset_properties. Authored-by: Terry Kim <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>
1 parent af37c7f commit b112e2b

File tree

13 files changed

+108
-65
lines changed

13 files changed

+108
-65
lines changed

‎sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -889,8 +889,11 @@ class Analyzer(override val catalogManager: CatalogManager)
889889
u.failAnalysis(s"${ident.quoted} is a temp view. '$cmd' expects a table")
890890
}
891891
u
892-
case u @ UnresolvedView(ident, _, _) =>
892+
case u @ UnresolvedView(ident, cmd, allowTemp, _) =>
893893
lookupTempView(ident).map { _ =>
894+
if (!allowTemp) {
895+
u.failAnalysis(s"${ident.quoted} is a temp view. '$cmd' expects a permanent view.")
896+
}
894897
ResolvedView(ident.asIdentifier, isTemp = true)
895898
}
896899
.getOrElse(u)
@@ -1118,7 +1121,7 @@ class Analyzer(override val catalogManager: CatalogManager)
11181121
case table => table
11191122
}.getOrElse(u)
11201123

1121-
case u @ UnresolvedView(identifier, cmd, relationTypeMismatchHint) =>
1124+
case u @ UnresolvedView(identifier, cmd, _, relationTypeMismatchHint) =>
11221125
lookupTableOrView(identifier).map {
11231126
case v: ResolvedView => v
11241127
case _ =>

‎sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ trait CheckAnalysis extends PredicateHelper with LookupCatalog {
104104
case u: UnresolvedTable =>
105105
u.failAnalysis(s"Table not found for '${u.commandName}': ${u.multipartIdentifier.quoted}")
106106

107-
case u @ UnresolvedView(NonSessionCatalogAndIdentifier(catalog, ident), cmd, _) =>
107+
case u @ UnresolvedView(NonSessionCatalogAndIdentifier(catalog, ident), cmd, _, _) =>
108108
u.failAnalysis(
109109
s"Cannot specify catalog `${catalog.name}` for view ${ident.quoted} " +
110110
"because view support in v2 catalog has not been implemented yet. " +

‎sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -121,18 +121,6 @@ class ResolveCatalogs(val catalogManager: CatalogManager)
121121
val changes = Seq(TableChange.setProperty(TableCatalog.PROP_LOCATION, newLoc))
122122
createAlterTable(nameParts, catalog, tbl, changes)
123123

124-
case AlterViewSetPropertiesStatement(
125-
NonSessionCatalogAndTable(catalog, tbl), props) =>
126-
throw new AnalysisException(
127-
s"Can not specify catalog `${catalog.name}` for view ${tbl.quoted} " +
128-
s"because view support in catalog has not been implemented yet")
129-
130-
case AlterViewUnsetPropertiesStatement(
131-
NonSessionCatalogAndTable(catalog, tbl), keys, ifExists) =>
132-
throw new AnalysisException(
133-
s"Can not specify catalog `${catalog.name}` for view ${tbl.quoted} " +
134-
s"because view support in catalog has not been implemented yet")
135-
136124
case c @ CreateTableStatement(
137125
NonSessionCatalogAndTable(catalog, tbl), _, _, _, _, _, _, _, _, _, _, _) =>
138126
assertNoNullTypeInSchema(c.tableSchema)

‎sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/v2ResolutionPlans.scala‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ case class UnresolvedTable(
5252
case class UnresolvedView(
5353
multipartIdentifier: Seq[String],
5454
commandName: String,
55+
allowTemp: Boolean = true,
5556
relationTypeMismatchHint: Option[String] = None) extends LeafNode {
5657
override lazy val resolved: Boolean = false
5758

‎sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala‎

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3161,8 +3161,9 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
31613161
DropView(
31623162
UnresolvedView(
31633163
visitMultipartIdentifier(ctx.multipartIdentifier()),
3164-
"DROP VIEW",
3165-
Some("Please use DROP TABLE instead.")),
3164+
commandName = "DROP VIEW",
3165+
allowTemp = true,
3166+
relationTypeMismatchHint = Some("Please use DROP TABLE instead.")),
31663167
ctx.EXISTS != null)
31673168
}
31683169

@@ -3399,7 +3400,7 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
33993400
}
34003401

34013402
/**
3402-
* Parse [[AlterViewSetPropertiesStatement]] or [[AlterTableSetPropertiesStatement]] commands.
3403+
* Parse [[AlterViewSetProperties]] or [[AlterTableSetPropertiesStatement]] commands.
34033404
*
34043405
* For example:
34053406
* {{{
@@ -3413,14 +3414,20 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
34133414
val properties = visitPropertyKeyValues(ctx.tablePropertyList)
34143415
val cleanedTableProperties = cleanTableProperties(ctx, properties)
34153416
if (ctx.VIEW != null) {
3416-
AlterViewSetPropertiesStatement(identifier, cleanedTableProperties)
3417+
AlterViewSetProperties(
3418+
UnresolvedView(
3419+
identifier,
3420+
commandName = "ALTER VIEW ... SET TBLPROPERTIES",
3421+
allowTemp = false,
3422+
relationTypeMismatchHint = Some("Please use ALTER TABLE instead.")),
3423+
cleanedTableProperties)
34173424
} else {
34183425
AlterTableSetPropertiesStatement(identifier, cleanedTableProperties)
34193426
}
34203427
}
34213428

34223429
/**
3423-
* Parse [[AlterViewUnsetPropertiesStatement]] or [[AlterTableUnsetPropertiesStatement]] commands.
3430+
* Parse [[AlterViewUnsetProperties]] or [[AlterTableUnsetPropertiesStatement]] commands.
34243431
*
34253432
* For example:
34263433
* {{{
@@ -3436,7 +3443,14 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
34363443

34373444
val ifExists = ctx.EXISTS != null
34383445
if (ctx.VIEW != null) {
3439-
AlterViewUnsetPropertiesStatement(identifier, cleanedProperties, ifExists)
3446+
AlterViewUnsetProperties(
3447+
UnresolvedView(
3448+
identifier,
3449+
commandName = "ALTER VIEW ... UNSET TBLPROPERTIES",
3450+
allowTemp = false,
3451+
relationTypeMismatchHint = Some("Please use ALTER TABLE instead.")),
3452+
cleanedProperties,
3453+
ifExists)
34403454
} else {
34413455
AlterTableUnsetPropertiesStatement(identifier, cleanedProperties, ifExists)
34423456
}

‎sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala‎

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -315,21 +315,6 @@ case class AlterTableSerDePropertiesStatement(
315315
serdeProperties: Option[Map[String, String]],
316316
partitionSpec: Option[TablePartitionSpec]) extends ParsedStatement
317317

318-
/**
319-
* ALTER VIEW ... SET TBLPROPERTIES command, as parsed from SQL.
320-
*/
321-
case class AlterViewSetPropertiesStatement(
322-
viewName: Seq[String],
323-
properties: Map[String, String]) extends ParsedStatement
324-
325-
/**
326-
* ALTER VIEW ... UNSET TBLPROPERTIES command, as parsed from SQL.
327-
*/
328-
case class AlterViewUnsetPropertiesStatement(
329-
viewName: Seq[String],
330-
propertyKeys: Seq[String],
331-
ifExists: Boolean) extends ParsedStatement
332-
333318
/**
334319
* ALTER VIEW ... Query command, as parsed from SQL.
335320
*/

‎sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -742,3 +742,22 @@ case class DropView(
742742
case class RepairTable(child: LogicalPlan) extends Command {
743743
override def children: Seq[LogicalPlan] = child :: Nil
744744
}
745+
746+
/**
747+
* The logical plan of the ALTER VIEW ... SET TBLPROPERTIES command.
748+
*/
749+
case class AlterViewSetProperties(
750+
child: LogicalPlan,
751+
properties: Map[String, String]) extends Command {
752+
override def children: Seq[LogicalPlan] = child :: Nil
753+
}
754+
755+
/**
756+
* The logical plan of the ALTER VIEW ... UNSET TBLPROPERTIES command.
757+
*/
758+
case class AlterViewUnsetProperties(
759+
child: LogicalPlan,
760+
propertyKeys: Seq[String],
761+
ifExists: Boolean) extends Command {
762+
override def children: Seq[LogicalPlan] = child :: Nil
763+
}

‎sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala‎

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -724,15 +724,15 @@ class DDLParserSuite extends AnalysisTest {
724724
val cmd = "DROP VIEW"
725725
val hint = Some("Please use DROP TABLE instead.")
726726
parseCompare(s"DROP VIEW testcat.db.view",
727-
DropView(UnresolvedView(Seq("testcat", "db", "view"), cmd, hint), ifExists = false))
727+
DropView(UnresolvedView(Seq("testcat", "db", "view"), cmd, true, hint), ifExists = false))
728728
parseCompare(s"DROP VIEW db.view",
729-
DropView(UnresolvedView(Seq("db", "view"), cmd, hint), ifExists = false))
729+
DropView(UnresolvedView(Seq("db", "view"), cmd, true, hint), ifExists = false))
730730
parseCompare(s"DROP VIEW IF EXISTS db.view",
731-
DropView(UnresolvedView(Seq("db", "view"), cmd, hint), ifExists = true))
731+
DropView(UnresolvedView(Seq("db", "view"), cmd, true, hint), ifExists = true))
732732
parseCompare(s"DROP VIEW view",
733-
DropView(UnresolvedView(Seq("view"), cmd, hint), ifExists = false))
733+
DropView(UnresolvedView(Seq("view"), cmd, true, hint), ifExists = false))
734734
parseCompare(s"DROP VIEW IF EXISTS view",
735-
DropView(UnresolvedView(Seq("view"), cmd, hint), ifExists = true))
735+
DropView(UnresolvedView(Seq("view"), cmd, true, hint), ifExists = true))
736736
}
737737

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

768769
comparePlans(parsePlan(sql1_view),
769-
AlterViewSetPropertiesStatement(
770-
Seq("table_name"), Map("test" -> "test", "comment" -> "new_comment")))
770+
AlterViewSetProperties(
771+
UnresolvedView(Seq("table_name"), "ALTER VIEW ... SET TBLPROPERTIES", false, hint),
772+
Map("test" -> "test", "comment" -> "new_comment")))
771773
comparePlans(parsePlan(sql2_view),
772-
AlterViewUnsetPropertiesStatement(
773-
Seq("table_name"), Seq("comment", "test"), ifExists = false))
774+
AlterViewUnsetProperties(
775+
UnresolvedView(Seq("table_name"), "ALTER VIEW ... UNSET TBLPROPERTIES", false, hint),
776+
Seq("comment", "test"),
777+
ifExists = false))
774778
comparePlans(parsePlan(sql3_view),
775-
AlterViewUnsetPropertiesStatement(
776-
Seq("table_name"), Seq("comment", "test"), ifExists = true))
779+
AlterViewUnsetProperties(
780+
UnresolvedView(Seq("table_name"), "ALTER VIEW ... UNSET TBLPROPERTIES", false, hint),
781+
Seq("comment", "test"),
782+
ifExists = true))
777783
}
778784

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

‎sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -209,12 +209,11 @@ class ResolveSessionCatalog(
209209
createAlterTable(nameParts, catalog, tbl, changes)
210210
}
211211

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

216-
case AlterViewUnsetPropertiesStatement(SessionCatalogAndTable(_, tbl), keys, ifExists) =>
217-
AlterTableUnsetPropertiesCommand(tbl.asTableIdentifier, keys, ifExists, isView = true)
215+
case AlterViewUnsetProperties(ResolvedView(ident, _), keys, ifExists) =>
216+
AlterTableUnsetPropertiesCommand(ident.asTableIdentifier, keys, ifExists, isView = true)
218217

219218
case d @ DescribeNamespace(SessionCatalogAndNamespace(_, ns), _) =>
220219
if (ns.length != 1) {

‎sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala‎

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2594,11 +2594,29 @@ class DataSourceV2SQLSuite
25942594
}
25952595
}
25962596

2597-
test("DROP VIEW is not supported for v2 catalogs") {
2598-
assertAnalysisError(
2599-
"DROP VIEW testcat.v",
2600-
"Cannot specify catalog `testcat` for view v because view support in v2 catalog " +
2601-
"has not been implemented yet. DROP VIEW expects a view.")
2597+
test("View commands are not supported in v2 catalogs") {
2598+
def validateViewCommand(
2599+
sql: String,
2600+
catalogName: String,
2601+
viewName: String,
2602+
cmdName: String): Unit = {
2603+
assertAnalysisError(
2604+
sql,
2605+
s"Cannot specify catalog `$catalogName` for view $viewName because view support " +
2606+
s"in v2 catalog has not been implemented yet. $cmdName expects a view.")
2607+
}
2608+
2609+
validateViewCommand("DROP VIEW testcat.v", "testcat", "v", "DROP VIEW")
2610+
validateViewCommand(
2611+
"ALTER VIEW testcat.v SET TBLPROPERTIES ('key' = 'val')",
2612+
"testcat",
2613+
"v",
2614+
"ALTER VIEW ... SET TBLPROPERTIES")
2615+
validateViewCommand(
2616+
"ALTER VIEW testcat.v UNSET TBLPROPERTIES ('key')",
2617+
"testcat",
2618+
"v",
2619+
"ALTER VIEW ... UNSET TBLPROPERTIES")
26022620
}
26032621

26042622
private def testNotSupportedV2Command(

0 commit comments

Comments
 (0)