Skip to content

[SPARK-33515][SQL] Improve exception messages while handling UnresolvedTable #30461

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

Closed
wants to merge 3 commits into from
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 @@ -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.'")
Copy link
Member

Choose a reason for hiding this comment

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

' at the end is not needed. I will remove it in this PR: #30398

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @MaxGekk!

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