Skip to content

Commit

Permalink
[SPARK-33515][SQL] Improve exception messages while handling Unresolv…
Browse files Browse the repository at this point in the history
…edTable

### What changes were proposed in this pull request?

This PR proposes to improve the exception messages while `UnresolvedTable` is handled based on this suggestion: #30321 (comment).

Currently, when an identifier is resolved to a view when a table is expected, the following exception message is displayed (e.g., for `COMMENT ON TABLE`):
```
v is a temp view not table.
```
After this PR, the message will be:
```
v is a temp view. 'COMMENT ON TABLE' expects a table.
```

Also, if an identifier is not resolved, the following exception message is currently used:
```
Table not found: t
```
After this PR, the message will be:
```
Table not found for 'COMMENT ON TABLE': t
```

### Why are the changes needed?

To improve the exception message.

### Does this PR introduce _any_ user-facing change?

Yes, the exception message will be changed as described above.

### How was this patch tested?

Updated existing tests.

Closes #30461 from imback82/unresolved_table_message.

Authored-by: Terry Kim <yuminkim@gmail.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
  • Loading branch information
imback82 authored and cloud-fan committed Nov 23, 2020
1 parent c891e02 commit 60f3a73
Show file tree
Hide file tree
Showing 8 changed files with 34 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -861,9 +861,9 @@ class Analyzer(override val catalogManager: CatalogManager)
}.getOrElse(write)
case _ => write
}
case u @ UnresolvedTable(ident) =>
case u @ UnresolvedTable(ident, cmd) =>
lookupTempView(ident).foreach { _ =>
u.failAnalysis(s"${ident.quoted} is a temp view not table.")
u.failAnalysis(s"${ident.quoted} is a temp view. '$cmd' expects a table")
}
u
case u @ UnresolvedTableOrView(ident, allowTempView) =>
Expand Down Expand Up @@ -950,7 +950,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(ResolvedTable(catalog.asTableCatalog, ident, _))
.getOrElse(u)
Expand Down Expand Up @@ -1077,11 +1077,11 @@ class Analyzer(override val catalogManager: CatalogManager)
lookupRelation(u.multipartIdentifier, u.options, u.isStreaming)
.map(resolveViews).getOrElse(u)

case u @ UnresolvedTable(identifier) =>
case u @ UnresolvedTable(identifier, cmd) =>
lookupTableOrView(identifier).map {
case v: ResolvedView =>
val viewStr = if (v.isTemp) "temp view" else "view"
u.failAnalysis(s"${v.identifier.quoted} is a $viewStr not table.")
u.failAnalysis(s"${v.identifier.quoted} is a $viewStr. '$cmd' expects a table.'")
case table => table
}.getOrElse(u)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ trait CheckAnalysis extends PredicateHelper {
u.failAnalysis(s"Namespace not found: ${u.multipartIdentifier.quoted}")

case u: UnresolvedTable =>
u.failAnalysis(s"Table not found: ${u.multipartIdentifier.quoted}")
u.failAnalysis(s"Table not found for '${u.commandName}': ${u.multipartIdentifier.quoted}")

case u: UnresolvedTableOrView =>
u.failAnalysis(s"Table or view not found: ${u.multipartIdentifier.quoted}")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,9 @@ case class UnresolvedNamespace(multipartIdentifier: Seq[String]) extends LeafNod
* Holds the name of a table that has yet to be looked up in a catalog. It will be resolved to
* [[ResolvedTable]] during analysis.
*/
case class UnresolvedTable(multipartIdentifier: Seq[String]) extends LeafNode {
case class UnresolvedTable(
multipartIdentifier: Seq[String],
commandName: String) extends LeafNode {
override lazy val resolved: Boolean = false

override def output: Seq[Attribute] = Nil
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3303,7 +3303,7 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
*/
override def visitLoadData(ctx: LoadDataContext): LogicalPlan = withOrigin(ctx) {
LoadData(
child = UnresolvedTable(visitMultipartIdentifier(ctx.multipartIdentifier)),
child = UnresolvedTable(visitMultipartIdentifier(ctx.multipartIdentifier), "LOAD DATA"),
path = string(ctx.path),
isLocal = ctx.LOCAL != null,
isOverwrite = ctx.OVERWRITE != null,
Expand Down Expand Up @@ -3449,7 +3449,9 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
UnresolvedPartitionSpec(spec, location)
}
AlterTableAddPartition(
UnresolvedTable(visitMultipartIdentifier(ctx.multipartIdentifier)),
UnresolvedTable(
visitMultipartIdentifier(ctx.multipartIdentifier),
"ALTER TABLE ... ADD PARTITION ..."),
specsAndLocs.toSeq,
ctx.EXISTS != null)
}
Expand Down Expand Up @@ -3491,7 +3493,9 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
val partSpecs = ctx.partitionSpec.asScala.map(visitNonOptionalPartitionSpec)
.map(spec => UnresolvedPartitionSpec(spec))
AlterTableDropPartition(
UnresolvedTable(visitMultipartIdentifier(ctx.multipartIdentifier)),
UnresolvedTable(
visitMultipartIdentifier(ctx.multipartIdentifier),
"ALTER TABLE ... DROP PARTITION ..."),
partSpecs.toSeq,
ifExists = ctx.EXISTS != null,
purge = ctx.PURGE != null,
Expand Down Expand Up @@ -3720,6 +3724,6 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with SQLConfHelper with Logg
case _ => string(ctx.STRING)
}
val nameParts = visitMultipartIdentifier(ctx.multipartIdentifier)
CommentOnTable(UnresolvedTable(nameParts), comment)
CommentOnTable(UnresolvedTable(nameParts, "COMMENT ON TABLE"), comment)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1555,15 +1555,15 @@ class DDLParserSuite extends AnalysisTest {
test("LOAD DATA INTO table") {
comparePlans(
parsePlan("LOAD DATA INPATH 'filepath' INTO TABLE a.b.c"),
LoadData(UnresolvedTable(Seq("a", "b", "c")), "filepath", false, false, None))
LoadData(UnresolvedTable(Seq("a", "b", "c"), "LOAD DATA"), "filepath", false, false, None))

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

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

comparePlans(
parsePlan(
Expand All @@ -1572,7 +1572,7 @@ class DDLParserSuite extends AnalysisTest {
|PARTITION(ds='2017-06-10')
""".stripMargin),
LoadData(
UnresolvedTable(Seq("a", "b", "c")),
UnresolvedTable(Seq("a", "b", "c"), "LOAD DATA"),
"filepath",
true,
true,
Expand Down Expand Up @@ -1674,13 +1674,13 @@ class DDLParserSuite extends AnalysisTest {
val parsed2 = parsePlan(sql2)

val expected1 = AlterTableAddPartition(
UnresolvedTable(Seq("a", "b", "c")),
UnresolvedTable(Seq("a", "b", "c"), "ALTER TABLE ... ADD PARTITION ..."),
Seq(
UnresolvedPartitionSpec(Map("dt" -> "2008-08-08", "country" -> "us"), Some("location1")),
UnresolvedPartitionSpec(Map("dt" -> "2009-09-09", "country" -> "uk"), None)),
ifNotExists = true)
val expected2 = AlterTableAddPartition(
UnresolvedTable(Seq("a", "b", "c")),
UnresolvedTable(Seq("a", "b", "c"), "ALTER TABLE ... ADD PARTITION ..."),
Seq(UnresolvedPartitionSpec(Map("dt" -> "2008-08-08"), Some("loc"))),
ifNotExists = false)

Expand Down Expand Up @@ -1747,7 +1747,7 @@ class DDLParserSuite extends AnalysisTest {
assertUnsupported(sql2_view)

val expected1_table = AlterTableDropPartition(
UnresolvedTable(Seq("table_name")),
UnresolvedTable(Seq("table_name"), "ALTER TABLE ... DROP PARTITION ..."),
Seq(
UnresolvedPartitionSpec(Map("dt" -> "2008-08-08", "country" -> "us")),
UnresolvedPartitionSpec(Map("dt" -> "2009-09-09", "country" -> "uk"))),
Expand All @@ -1763,7 +1763,7 @@ class DDLParserSuite extends AnalysisTest {

val sql3_table = "ALTER TABLE a.b.c DROP IF EXISTS PARTITION (ds='2017-06-10')"
val expected3_table = AlterTableDropPartition(
UnresolvedTable(Seq("a", "b", "c")),
UnresolvedTable(Seq("a", "b", "c"), "ALTER TABLE ... DROP PARTITION ..."),
Seq(UnresolvedPartitionSpec(Map("ds" -> "2017-06-10"))),
ifExists = true,
purge = false,
Expand Down Expand Up @@ -2174,7 +2174,7 @@ class DDLParserSuite extends AnalysisTest {

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

// TODO: ignored by SPARK-31707, restore the test after create table syntax unification
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2414,7 +2414,8 @@ class DataSourceV2SQLSuite
withTempView("v") {
sql("create global temp view v as select 1")
val e = intercept[AnalysisException](sql("COMMENT ON TABLE global_temp.v IS NULL"))
assert(e.getMessage.contains("global_temp.v is a temp view not table."))
assert(e.getMessage.contains(
"global_temp.v is a temp view. 'COMMENT ON TABLE' expects a table"))
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,10 @@ abstract class SQLViewSuite extends QueryTest with SQLTestUtils {
s"'$viewName' is a view not a table")
assertAnalysisError(
s"ALTER TABLE $viewName ADD IF NOT EXISTS PARTITION (a='4', b='8')",
s"$viewName is a temp view not table")
s"$viewName is a temp view. 'ALTER TABLE ... ADD PARTITION ...' expects a table")
assertAnalysisError(
s"ALTER TABLE $viewName DROP PARTITION (a='4', b='8')",
s"$viewName is a temp view not table")
s"$viewName is a temp view. 'ALTER TABLE ... DROP PARTITION ...' expects a table")

// For the following v2 ALERT TABLE statements, unsupported operations are checked first
// before resolving the relations.
Expand All @@ -175,7 +175,7 @@ abstract class SQLViewSuite extends QueryTest with SQLTestUtils {
val e2 = intercept[AnalysisException] {
sql(s"""LOAD DATA LOCAL INPATH "$dataFilePath" INTO TABLE $viewName""")
}.getMessage
assert(e2.contains(s"$viewName is a temp view not table"))
assert(e2.contains(s"$viewName is a temp view. 'LOAD DATA' expects a table"))
assertNoSuchTable(s"TRUNCATE TABLE $viewName")
val e3 = intercept[AnalysisException] {
sql(s"SHOW CREATE TABLE $viewName")
Expand Down Expand Up @@ -214,7 +214,7 @@ abstract class SQLViewSuite extends QueryTest with SQLTestUtils {
e = intercept[AnalysisException] {
sql(s"""LOAD DATA LOCAL INPATH "$dataFilePath" INTO TABLE $viewName""")
}.getMessage
assert(e.contains("default.testView is a view not table"))
assert(e.contains("default.testView is a view. 'LOAD DATA' expects a table"))

e = intercept[AnalysisException] {
sql(s"TRUNCATE TABLE $viewName")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -904,10 +904,10 @@ class HiveDDLSuite

assertAnalysisError(
s"ALTER TABLE $oldViewName ADD IF NOT EXISTS PARTITION (a='4', b='8')",
s"$oldViewName is a view not table")
s"$oldViewName is a view. 'ALTER TABLE ... ADD PARTITION ...' expects a table.")
assertAnalysisError(
s"ALTER TABLE $oldViewName DROP IF EXISTS PARTITION (a='2')",
s"$oldViewName is a view not table")
s"$oldViewName is a view. 'ALTER TABLE ... DROP PARTITION ...' expects a table.")

assert(catalog.tableExists(TableIdentifier(tabName)))
assert(catalog.tableExists(TableIdentifier(oldViewName)))
Expand Down

0 comments on commit 60f3a73

Please sign in to comment.