Skip to content

[SPARK-33872][SQL] Throw more specific exceptions when relations/namespaces are not found #30878

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
wants to merge 2 commits into from
Closed

[SPARK-33872][SQL] Throw more specific exceptions when relations/namespaces are not found #30878

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f39b455
initial commit
imback82 Dec 21, 2020
6893510
change line
imback82 Dec 21, 2020
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
24 changes: 12 additions & 12 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,10 @@ trait CheckAnalysis extends PredicateHelper with LookupCatalog {
throw QueryExecutionErrors.logicalPlanHaveOutputOfCharOrVarcharError(leaf)

case u: UnresolvedNamespace =>
u.failAnalysis(s"Namespace not found: ${u.multipartIdentifier.quoted}")
throw new NoSuchNamespaceException(s"Namespace not found: ${u.multipartIdentifier.quoted}")
Copy link
Contributor

@cloud-fan cloud-fan Dec 22, 2020

Choose a reason for hiding this comment

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

One benefit of u.failAnalysis is that it can report the error position in SQL string. If we can't keep this feature with the new exception, I'd suggest we don't change the exception type.

Copy link
Contributor Author

@imback82 imback82 Dec 23, 2020

Choose a reason for hiding this comment

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

Should we close this PR or update NoSuchNamespaceException and NoSuchTableException to handle positions?

Btw, it looks like there is a bug in keeping the position. I will fix it separately.

Copy link
Contributor

@cloud-fan cloud-fan Dec 23, 2020

Choose a reason for hiding this comment

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

... update NoSuchNamespaceException and NoSuchTableException to handle positions?

If it's easy to do, let's go for it. Otherwise, we can close this PR as exception type seems not a big deal to me.


case u: UnresolvedTable =>
u.failAnalysis(s"Table not found: ${u.multipartIdentifier.quoted}")
throw new NoSuchTableException(s"Table not found: ${u.multipartIdentifier.quoted}")

case u @ UnresolvedView(NonSessionCatalogAndIdentifier(catalog, ident), cmd, _, _) =>
u.failAnalysis(
Expand All @@ -111,43 +111,43 @@ trait CheckAnalysis extends PredicateHelper with LookupCatalog {
s"$cmd expects a view.")

case u: UnresolvedView =>
u.failAnalysis(s"View not found: ${u.multipartIdentifier.quoted}")
throw new NoSuchTableException(s"View not found: ${u.multipartIdentifier.quoted}")

case u: UnresolvedTableOrView =>
val viewStr = if (u.allowTempView) "view" else "permanent view"
u.failAnalysis(
throw new NoSuchTableException(
s"Table or $viewStr not found: ${u.multipartIdentifier.quoted}")

case u: UnresolvedRelation =>
u.failAnalysis(s"Table or view not found: ${u.multipartIdentifier.quoted}")
throw new NoSuchTableException(s"Table or view not found: ${u.multipartIdentifier.quoted}")

case InsertIntoStatement(u: UnresolvedRelation, _, _, _, _, _) =>
failAnalysis(s"Table not found: ${u.multipartIdentifier.quoted}")
throw new NoSuchTableException(s"Table not found: ${u.multipartIdentifier.quoted}")

case CacheTable(u: UnresolvedRelation, _, _, _) =>
failAnalysis(s"Table or view not found: ${u.multipartIdentifier.quoted}")
throw new NoSuchTableException(s"Table or view not found: ${u.multipartIdentifier.quoted}")

case UncacheTable(u: UnresolvedRelation, _, _) =>
failAnalysis(s"Table or view not found: ${u.multipartIdentifier.quoted}")
throw new NoSuchTableException(s"Table or view not found: ${u.multipartIdentifier.quoted}")

// TODO (SPARK-27484): handle streaming write commands when we have them.
case write: V2WriteCommand if write.table.isInstanceOf[UnresolvedRelation] =>
val tblName = write.table.asInstanceOf[UnresolvedRelation].multipartIdentifier
write.table.failAnalysis(s"Table or view not found: ${tblName.quoted}")
throw new NoSuchTableException(s"Table or view not found: ${tblName.quoted}")
Copy link
Contributor Author

@imback82 imback82 Dec 21, 2020

Choose a reason for hiding this comment

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

Do we want to throw this exception at a specific node, or is it not needed?

    /** Fails the analysis at the point where a specific tree node was parsed. */
    def failAnalysis(msg: String): Nothing = {
      throw new AnalysisException(msg, t.origin.line, t.origin.startPosition)
    }


case u: UnresolvedV2Relation if isView(u.originalNameParts) =>
u.failAnalysis(
throw new NoSuchTableException(
s"Invalid command: '${u.originalNameParts.quoted}' is a view not a table.")

case u: UnresolvedV2Relation =>
u.failAnalysis(s"Table not found: ${u.originalNameParts.quoted}")
throw new NoSuchTableException(s"Table not found: ${u.originalNameParts.quoted}")

case AlterTable(_, _, u: UnresolvedV2Relation, _) if isView(u.originalNameParts) =>
u.failAnalysis(
s"Invalid command: '${u.originalNameParts.quoted}' is a view not a table.")

case AlterTable(_, _, u: UnresolvedV2Relation, _) =>
failAnalysis(s"Table not found: ${u.originalNameParts.quoted}")
throw new NoSuchTableException(s"Table not found: ${u.originalNameParts.quoted}")

case operator: LogicalPlan =>
// Check argument data types of higher-order functions downwards first.
Expand Down