Skip to content
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

[SPARK-19952][SQL] Remove various analysis exceptions #17716

Closed
wants to merge 2 commits into from

Conversation

hvanhovell
Copy link
Contributor

What changes were proposed in this pull request?

We currently have quite a few analysis exception subclasses, the problem with these is that they are not well supported throughout Spark (in pySpark for example) and that the added value is limited. This PR removes the no such object and the already exists exceptions, and replaces them by a factory methods that create analysis exceptions with the required error messages.

How was this patch tested?

Modified existing tests.

case _: NoSuchTableException =>
u.failAnalysis(s"Table or view not found: ${tableIdentWithDb.unquotedString}")
// If the database is defined and that database is not found, throw an AnalysisException.
if (!tableIdentWithDb.database.exists(catalog.databaseExists)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps we should move this into catalog.lookupRelation and attach the location here.

Copy link
Contributor

Choose a reason for hiding this comment

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

how about

try {
  catalog.lookupRelation(tableIdentWithDb)
} catch {
  case a: AnalysisException => u.failAnalysis(a.message)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, could work. This change is also causing the tests to fail, so I am revisiting it.

@hvanhovell
Copy link
Contributor Author

cc @cloud-fan @ueshin @rxin

@SparkQA
Copy link

SparkQA commented Apr 21, 2017

Test build #76031 has finished for PR 17716 at commit 6dc3204.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Apr 21, 2017

Test build #76039 has finished for PR 17716 at commit 79ee4a6.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@@ -160,7 +159,7 @@ abstract class SQLViewSuite extends QueryTest with SQLTestUtils {
}

private def assertNoSuchTable(query: String): Unit = {
intercept[NoSuchTableException] {
intercept[AnalysisException] {
Copy link
Member

Choose a reason for hiding this comment

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

AnalysisException is a pretty general Exception in Spark SQL. The future refactoring might introduce bugs without trigger any test case failure.

Could we check the error messages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

@HyukjinKwon
Copy link
Member

@hvanhovell, how is it going?

@jiangxb1987
Copy link
Contributor

ping @hvanhovell Are you still working on this?

@asfgit asfgit closed this in b32bd00 Jun 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants