Skip to content

[SPARK-34317][SQL] Introduce relationTypeMismatchHint to UnresolvedTable for a better error message #31424

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.

Sign up for GitHub

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

Closed
imback82 wants to merge 1 commit into from
Closed

[SPARK-34317][SQL] Introduce relationTypeMismatchHint to UnresolvedTable for a better error message #31424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 10 additions & 8 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -893,7 +893,7 @@ class Analyzer(override val catalogManager: CatalogManager)
}.getOrElse(write)
case _ => write
}
case u @ UnresolvedTable(ident, cmd) =>
case u @ UnresolvedTable(ident, cmd, _) =>
lookupTempView(ident).foreach { _ =>
throw QueryCompilationErrors.expectTableNotTempViewError(ident.quoted, cmd, u)
}
Expand Down Expand Up @@ -990,7 +990,7 @@ class Analyzer(override val catalogManager: CatalogManager)
SubqueryAlias(catalog.get.name +: ident.namespace :+ ident.name, relation)
}.getOrElse(u)

case u @ UnresolvedTable(NonSessionCatalogAndIdentifier(catalog, ident), _) =>
case u @ UnresolvedTable(NonSessionCatalogAndIdentifier(catalog, ident), _, _) =>
CatalogV2Util.loadTable(catalog, ident)
.map(table => ResolvedTable.create(catalog.asTableCatalog, ident, table))
.getOrElse(u)
Expand Down Expand Up @@ -1144,18 +1144,20 @@ class Analyzer(override val catalogManager: CatalogManager)
lookupRelation(u.multipartIdentifier, u.options, u.isStreaming)
.map(resolveViews).getOrElse(u)

case u @ UnresolvedTable(identifier, cmd) =>
case u @ UnresolvedTable(identifier, cmd, relationTypeMismatchHint) =>
lookupTableOrView(identifier).map {
case v: ResolvedView => throw QueryCompilationErrors.expectTableNotViewError(v, cmd, u)
case v: ResolvedView =>
Copy link
Contributor Author

@imback82 imback82 Feb 3, 2021

Choose a reason for hiding this comment

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

@cloud-fan looks like I missed handling this in ResolveTempViews. I will create a follow up PR. Sorry about that.

throw QueryCompilationErrors.expectTableNotViewError(
v, cmd, relationTypeMismatchHint, u)
case table => table
}.getOrElse(u)

case u @ UnresolvedView(identifier, cmd, _, relationTypeMismatchHint) =>
lookupTableOrView(identifier).map {
case v: ResolvedView => v
case _ =>
u.failAnalysis(s"${identifier.quoted} is a table. '$cmd' expects a view." +
relationTypeMismatchHint.map(" " + _).getOrElse(""))
case t: ResolvedTable =>
throw QueryCompilationErrors.expectViewNotTableError(
t, cmd, relationTypeMismatchHint, u)
case view => view
}.getOrElse(u)

case u @ UnresolvedTableOrView(identifier, _, _) =>
Expand Down
3 changes: 2 additions & 1 deletion sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/v2ResolutionPlans.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ case class UnresolvedNamespace(multipartIdentifier: Seq[String]) extends LeafNod
*/
case class UnresolvedTable(
multipartIdentifier: Seq[String],
commandName: String) extends LeafNode {
commandName: String,
relationTypeMismatchHint: Option[String]) extends LeafNode {
override lazy val resolved: Boolean = false

override def output: Seq[Attribute] = Nil
Expand Down
31 changes: 21 additions & 10 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2166,8 +2166,9 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
*/
private def createUnresolvedTable(
ctx: MultipartIdentifierContext,
commandName: String): UnresolvedTable = withOrigin(ctx) {
UnresolvedTable(visitMultipartIdentifier(ctx), commandName)
commandName: String,
relationTypeMismatchHint: Option[String] = None): UnresolvedTable = withOrigin(ctx) {
UnresolvedTable(visitMultipartIdentifier(ctx), commandName, relationTypeMismatchHint)
}

/**
Expand Down Expand Up @@ -3490,7 +3491,7 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
ctx.multipartIdentifier,
commandName = "ALTER VIEW ... SET TBLPROPERTIES",
allowTemp = false,
relationTypeMismatchHint = Some("Please use ALTER TABLE instead.")),
relationTypeMismatchHint = alterViewTypeMismatchHint),
cleanedTableProperties)
} else {
AlterTableSetPropertiesStatement(
Expand Down Expand Up @@ -3520,7 +3521,7 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
ctx.multipartIdentifier,
commandName = "ALTER VIEW ... UNSET TBLPROPERTIES",
allowTemp = false,
relationTypeMismatchHint = Some("Please use ALTER TABLE instead.")),
relationTypeMismatchHint = alterViewTypeMismatchHint),
cleanedProperties,
ifExists)
} else {
Expand All @@ -3543,7 +3544,8 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
AlterTableSetLocation(
createUnresolvedTable(
ctx.multipartIdentifier,
"ALTER TABLE ... SET LOCATION ..."),
"ALTER TABLE ... SET LOCATION ...",
alterTableTypeMismatchHint),
Option(ctx.partitionSpec).map(visitNonOptionalPartitionSpec),
visitLocationSpec(ctx.locationSpec))
}
Expand Down Expand Up @@ -3810,7 +3812,8 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
AlterTableRecoverPartitions(
createUnresolvedTable(
ctx.multipartIdentifier,
"ALTER TABLE ... RECOVER PARTITIONS"))
"ALTER TABLE ... RECOVER PARTITIONS",
alterTableTypeMismatchHint))
}

/**
Expand Down Expand Up @@ -3839,7 +3842,8 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
AlterTableAddPartition(
createUnresolvedTable(
ctx.multipartIdentifier,
"ALTER TABLE ... ADD PARTITION ..."),
"ALTER TABLE ... ADD PARTITION ...",
alterTableTypeMismatchHint),
specsAndLocs.toSeq,
ctx.EXISTS != null)
}
Expand All @@ -3857,7 +3861,8 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
AlterTableRenamePartition(
createUnresolvedTable(
ctx.multipartIdentifier,
"ALTER TABLE ... RENAME TO PARTITION"),
"ALTER TABLE ... RENAME TO PARTITION",
alterTableTypeMismatchHint),
UnresolvedPartitionSpec(visitNonOptionalPartitionSpec(ctx.from)),
UnresolvedPartitionSpec(visitNonOptionalPartitionSpec(ctx.to)))
}
Expand Down Expand Up @@ -3885,7 +3890,8 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
AlterTableDropPartition(
createUnresolvedTable(
ctx.multipartIdentifier,
"ALTER TABLE ... DROP PARTITION ..."),
"ALTER TABLE ... DROP PARTITION ...",
alterTableTypeMismatchHint),
partSpecs.toSeq,
ifExists = ctx.EXISTS != null,
purge = ctx.PURGE != null)
Expand All @@ -3905,7 +3911,8 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
AlterTableSerDeProperties(
createUnresolvedTable(
ctx.multipartIdentifier,
"ALTER TABLE ... SET [SERDE|SERDEPROPERTIES]"),
"ALTER TABLE ... SET [SERDE|SERDEPROPERTIES]",
alterTableTypeMismatchHint),
Option(ctx.STRING).map(string),
Option(ctx.tablePropertyList).map(visitPropertyKeyValues),
// TODO a partition spec is allowed to have optional values. This is currently violated.
Expand Down Expand Up @@ -4118,4 +4125,8 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
}
CommentOnTable(createUnresolvedTable(ctx.multipartIdentifier, "COMMENT ON TABLE"), comment)
}

private def alterViewTypeMismatchHint: Option[String] = Some("Please use ALTER TABLE instead.")

private def alterTableTypeMismatchHint: Option[String] = Some("Please use ALTER VIEW instead.")
}
15 changes: 12 additions & 3 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import org.apache.hadoop.fs.Path

import org.apache.spark.sql.AnalysisException
import org.apache.spark.sql.catalyst.{FunctionIdentifier, QualifiedTableName, TableIdentifier}
import org.apache.spark.sql.catalyst.analysis.{ResolvedNamespace, ResolvedView}
import org.apache.spark.sql.catalyst.analysis.{ResolvedNamespace, ResolvedTable, ResolvedView}
import org.apache.spark.sql.catalyst.catalog.InvalidUDFClassException
import org.apache.spark.sql.catalyst.expressions.{Alias, Attribute, CreateMap, Expression, GroupingID, NamedExpression, SpecifiedWindowFrame, WindowFrame, WindowFunction, WindowSpecDefinition}
import org.apache.spark.sql.catalyst.plans.logical.{LogicalPlan, SerdeInfo}
Expand Down Expand Up @@ -231,9 +231,18 @@ private[spark] object QueryCompilationErrors {
t.origin.line, t.origin.startPosition)
}

def expectTableNotViewError(v: ResolvedView, cmd: String, t: TreeNode[_]): Throwable = {
def expectTableNotViewError(
v: ResolvedView, cmd: String, mismatchHint: Option[String], t: TreeNode[_]): Throwable = {
val viewStr = if (v.isTemp) "temp view" else "view"
new AnalysisException(s"${v.identifier.quoted} is a $viewStr. '$cmd' expects a table.",
val hintStr = mismatchHint.map(" " + _).getOrElse("")
new AnalysisException(s"${v.identifier.quoted} is a $viewStr. '$cmd' expects a table.$hintStr",
t.origin.line, t.origin.startPosition)
}

def expectViewNotTableError(
v: ResolvedTable, cmd: String, mismatchHint: Option[String], t: TreeNode[_]): Throwable = {
val hintStr = mismatchHint.map(" " + _).getOrElse("")
new AnalysisException(s"${v.identifier.quoted} is a table. '$cmd' expects a view.$hintStr",
t.origin.line, t.origin.startPosition)
}

Expand Down
51 changes: 34 additions & 17 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -867,17 +867,18 @@ class DDLParserSuite extends AnalysisTest {
}

test("alter table: set location") {
val hint = Some("Please use ALTER VIEW instead.")
comparePlans(
parsePlan("ALTER TABLE a.b.c SET LOCATION 'new location'"),
AlterTableSetLocation(
UnresolvedTable(Seq("a", "b", "c"), "ALTER TABLE ... SET LOCATION ..."),
UnresolvedTable(Seq("a", "b", "c"), "ALTER TABLE ... SET LOCATION ...", hint),
None,
"new location"))

comparePlans(
parsePlan("ALTER TABLE a.b.c PARTITION(ds='2017-06-10') SET LOCATION 'new location'"),
AlterTableSetLocation(
UnresolvedTable(Seq("a", "b", "c"), "ALTER TABLE ... SET LOCATION ..."),
UnresolvedTable(Seq("a", "b", "c"), "ALTER TABLE ... SET LOCATION ...", hint),
Some(Map("ds" -> "2017-06-10")),
"new location"))
}
Expand Down Expand Up @@ -1915,21 +1916,36 @@ class DDLParserSuite extends AnalysisTest {
test("MSCK REPAIR TABLE") {
comparePlans(
parsePlan("MSCK REPAIR TABLE a.b.c"),
RepairTable(UnresolvedTable(Seq("a", "b", "c"), "MSCK REPAIR TABLE")))
RepairTable(UnresolvedTable(Seq("a", "b", "c"), "MSCK REPAIR TABLE", None)))
}

test("LOAD DATA INTO table") {
comparePlans(
parsePlan("LOAD DATA INPATH 'filepath' INTO TABLE a.b.c"),
LoadData(UnresolvedTable(Seq("a", "b", "c"), "LOAD DATA"), "filepath", false, false, None))
LoadData(
UnresolvedTable(Seq("a", "b", "c"), "LOAD DATA", None),
"filepath",
false,
false,
None))

comparePlans(
parsePlan("LOAD DATA LOCAL INPATH 'filepath' INTO TABLE a.b.c"),
LoadData(UnresolvedTable(Seq("a", "b", "c"), "LOAD DATA"), "filepath", true, false, None))
LoadData(
UnresolvedTable(Seq("a", "b", "c"), "LOAD DATA", None),
"filepath",
true,
false,
None))

comparePlans(
parsePlan("LOAD DATA LOCAL INPATH 'filepath' OVERWRITE INTO TABLE a.b.c"),
LoadData(UnresolvedTable(Seq("a", "b", "c"), "LOAD DATA"), "filepath", true, true, None))
LoadData(
UnresolvedTable(Seq("a", "b", "c"), "LOAD DATA", None),
"filepath",
true,
true,
None))

comparePlans(
parsePlan(
Expand All @@ -1938,7 +1954,7 @@ class DDLParserSuite extends AnalysisTest {
|PARTITION(ds='2017-06-10')
""".stripMargin),
LoadData(
UnresolvedTable(Seq("a", "b", "c"), "LOAD DATA"),
UnresolvedTable(Seq("a", "b", "c"), "LOAD DATA", None),
"filepath",
true,
true,
Expand Down Expand Up @@ -2003,12 +2019,12 @@ class DDLParserSuite extends AnalysisTest {
test("TRUNCATE table") {
comparePlans(
parsePlan("TRUNCATE TABLE a.b.c"),
TruncateTable(UnresolvedTable(Seq("a", "b", "c"), "TRUNCATE TABLE"), None))
TruncateTable(UnresolvedTable(Seq("a", "b", "c"), "TRUNCATE TABLE", None), None))

comparePlans(
parsePlan("TRUNCATE TABLE a.b.c PARTITION(ds='2017-06-10')"),
TruncateTable(
UnresolvedTable(Seq("a", "b", "c"), "TRUNCATE TABLE"),
UnresolvedTable(Seq("a", "b", "c"), "TRUNCATE TABLE", None),
Some(Map("ds" -> "2017-06-10"))))
}

Expand Down Expand Up @@ -2058,9 +2074,10 @@ class DDLParserSuite extends AnalysisTest {

test("alter table: SerDe properties") {
val sql1 = "ALTER TABLE table_name SET SERDE 'org.apache.class'"
val hint = Some("Please use ALTER VIEW instead.")
val parsed1 = parsePlan(sql1)
val expected1 = AlterTableSerDeProperties(
UnresolvedTable(Seq("table_name"), "ALTER TABLE ... SET [SERDE|SERDEPROPERTIES]"),
UnresolvedTable(Seq("table_name"), "ALTER TABLE ... SET [SERDE|SERDEPROPERTIES]", hint),
Some("org.apache.class"),
None,
None)
Expand All @@ -2073,7 +2090,7 @@ class DDLParserSuite extends AnalysisTest {
""".stripMargin
val parsed2 = parsePlan(sql2)
val expected2 = AlterTableSerDeProperties(
UnresolvedTable(Seq("table_name"), "ALTER TABLE ... SET [SERDE|SERDEPROPERTIES]"),
UnresolvedTable(Seq("table_name"), "ALTER TABLE ... SET [SERDE|SERDEPROPERTIES]", hint),
Some("org.apache.class"),
Some(Map("columns" -> "foo,bar", "field.delim" -> ",")),
None)
Expand All @@ -2086,7 +2103,7 @@ class DDLParserSuite extends AnalysisTest {
""".stripMargin
val parsed3 = parsePlan(sql3)
val expected3 = AlterTableSerDeProperties(
UnresolvedTable(Seq("table_name"), "ALTER TABLE ... SET [SERDE|SERDEPROPERTIES]"),
UnresolvedTable(Seq("table_name"), "ALTER TABLE ... SET [SERDE|SERDEPROPERTIES]", hint),
None,
Some(Map("columns" -> "foo,bar", "field.delim" -> ",")),
None)
Expand All @@ -2100,7 +2117,7 @@ class DDLParserSuite extends AnalysisTest {
""".stripMargin
val parsed4 = parsePlan(sql4)
val expected4 = AlterTableSerDeProperties(
UnresolvedTable(Seq("table_name"), "ALTER TABLE ... SET [SERDE|SERDEPROPERTIES]"),
UnresolvedTable(Seq("table_name"), "ALTER TABLE ... SET [SERDE|SERDEPROPERTIES]", hint),
Some("org.apache.class"),
Some(Map("columns" -> "foo,bar", "field.delim" -> ",")),
Some(Map("test" -> "1", "dt" -> "2008-08-08", "country" -> "us")))
Expand All @@ -2113,7 +2130,7 @@ class DDLParserSuite extends AnalysisTest {
""".stripMargin
val parsed5 = parsePlan(sql5)
val expected5 = AlterTableSerDeProperties(
UnresolvedTable(Seq("table_name"), "ALTER TABLE ... SET [SERDE|SERDEPROPERTIES]"),
UnresolvedTable(Seq("table_name"), "ALTER TABLE ... SET [SERDE|SERDEPROPERTIES]", hint),
None,
Some(Map("columns" -> "foo,bar", "field.delim" -> ",")),
Some(Map("test" -> "1", "dt" -> "2008-08-08", "country" -> "us")))
Expand All @@ -2126,7 +2143,7 @@ class DDLParserSuite extends AnalysisTest {
""".stripMargin
val parsed6 = parsePlan(sql6)
val expected6 = AlterTableSerDeProperties(
UnresolvedTable(Seq("a", "b", "c"), "ALTER TABLE ... SET [SERDE|SERDEPROPERTIES]"),
UnresolvedTable(Seq("a", "b", "c"), "ALTER TABLE ... SET [SERDE|SERDEPROPERTIES]", hint),
Some("org.apache.class"),
Some(Map("columns" -> "foo,bar", "field.delim" -> ",")),
None)
Expand All @@ -2139,7 +2156,7 @@ class DDLParserSuite extends AnalysisTest {
""".stripMargin
val parsed7 = parsePlan(sql7)
val expected7 = AlterTableSerDeProperties(
UnresolvedTable(Seq("a", "b", "c"), "ALTER TABLE ... SET [SERDE|SERDEPROPERTIES]"),
UnresolvedTable(Seq("a", "b", "c"), "ALTER TABLE ... SET [SERDE|SERDEPROPERTIES]", hint),
None,
Some(Map("columns" -> "foo,bar", "field.delim" -> ",")),
Some(Map("test" -> "1", "dt" -> "2008-08-08", "country" -> "us")))
Expand Down Expand Up @@ -2470,7 +2487,7 @@ class DDLParserSuite extends AnalysisTest {

comparePlans(
parsePlan("COMMENT ON TABLE a.b.c IS 'xYz'"),
CommentOnTable(UnresolvedTable(Seq("a", "b", "c"), "COMMENT ON TABLE"), "xYz"))
CommentOnTable(UnresolvedTable(Seq("a", "b", "c"), "COMMENT ON TABLE", None), "xYz"))
}

test("create table - without using") {
Expand Down
3 changes: 2 additions & 1 deletion sql/core/src/main/scala/org/apache/spark/sql/internal/CatalogImpl.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,8 @@ class CatalogImpl(sparkSession: SparkSession) extends Catalog {
override def recoverPartitions(tableName: String): Unit = {
val multiPartIdent = sparkSession.sessionState.sqlParser.parseMultipartIdentifier(tableName)
sparkSession.sessionState.executePlan(
AlterTableRecoverPartitions(UnresolvedTable(multiPartIdent, "recoverPartitions()"))).toRdd
AlterTableRecoverPartitions(
UnresolvedTable(multiPartIdent, "recoverPartitions()", None))).toRdd
}

/**
Expand Down
10 changes: 8 additions & 2 deletions ...test/scala/org/apache/spark/sql/execution/command/AlterTableAddPartitionParserSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@ class AlterTableAddPartitionParserSuite extends AnalysisTest with SharedSparkSes
|(dt='2009-09-09', country='uk')""".stripMargin
val parsed = parsePlan(sql)
val expected = AlterTableAddPartition(
UnresolvedTable(Seq("a", "b", "c"), "ALTER TABLE ... ADD PARTITION ..."),
UnresolvedTable(
Seq("a", "b", "c"),
"ALTER TABLE ... ADD PARTITION ...",
Some("Please use ALTER VIEW instead.")),
Seq(
UnresolvedPartitionSpec(Map("dt" -> "2008-08-08", "country" -> "us"), Some("location1")),
UnresolvedPartitionSpec(Map("dt" -> "2009-09-09", "country" -> "uk"), None)),
Expand All @@ -42,7 +45,10 @@ class AlterTableAddPartitionParserSuite extends AnalysisTest with SharedSparkSes
val sql = "ALTER TABLE a.b.c ADD PARTITION (dt='2008-08-08') LOCATION 'loc'"
val parsed = parsePlan(sql)
val expected = AlterTableAddPartition(
UnresolvedTable(Seq("a", "b", "c"), "ALTER TABLE ... ADD PARTITION ..."),
UnresolvedTable(
Seq("a", "b", "c"),
"ALTER TABLE ... ADD PARTITION ...",
Some("Please use ALTER VIEW instead.")),
Seq(UnresolvedPartitionSpec(Map("dt" -> "2008-08-08"), Some("loc"))),
ifNotExists = false)

Expand Down
Loading