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-22686][SQL] DROP TABLE IF EXISTS should not show AnalysisException #19888

Closed
wants to merge 3 commits into from
Closed

[SPARK-22686][SQL] DROP TABLE IF EXISTS should not show AnalysisException #19888

wants to merge 3 commits into from

Conversation

dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Dec 5, 2017

What changes were proposed in this pull request?

During SPARK-22488 to fix view resolution issue, there occurs a regression at 2.2.1 and master branch like the following. This PR fixes that.

scala> spark.version
res2: String = 2.2.1

scala> sql("DROP TABLE IF EXISTS t").show
17/12/04 21:01:06 WARN DropTableCommand: org.apache.spark.sql.AnalysisException:
Table or view not found: t;
org.apache.spark.sql.AnalysisException: Table or view not found: t;

How was this patch tested?

Manual.

@@ -206,7 +206,8 @@ case class DropTableCommand(
try {
sparkSession.sharedState.cacheManager.uncacheQuery(sparkSession.table(tableName))
} catch {
case _: NoSuchTableException if ifExists =>
case ae: AnalysisException
if ifExists && ae.cause.nonEmpty && ae.getCause.isInstanceOf[NoSuchTableException] =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

scala> spark.version
res2: String = 2.3.0-SNAPSHOT

scala> sql("DROP TABLE IF EXISTS t")
res3: org.apache.spark.sql.DataFrame = []

scala> sql("DROP TABLE IF EXISTS t")
res4: org.apache.spark.sql.DataFrame = []

Unable to reproduce it.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's in the log, which means it's not a serious bug.

Copy link
Member

Choose a reason for hiding this comment

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

I can see it in 2.2.1. It is a warning message.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm, it's fixed in master?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dongjoon-hyun can you verify it? thanks!

Copy link
Member

Choose a reason for hiding this comment

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

I can reproduce it in master now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. It exists in master, too.
I'll update like the example by @cloud-fan .

@SparkQA
Copy link

SparkQA commented Dec 5, 2017

Test build #84454 has finished for PR 19888 at commit 2b113a2.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Dec 5, 2017

Test build #84458 has finished for PR 19888 at commit b2b4d54.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun
Copy link
Member Author

Retest this please.

case _: NoSuchTableException if ifExists =>
case NonFatal(e) => log.warn(e.toString, e)

if (!ifExists || catalog.tableExists(tableName)) {
Copy link
Contributor

@cloud-fan cloud-fan Dec 5, 2017

Choose a reason for hiding this comment

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

I feel it's more readable to write

if (catalog.tableExists(tableName)) {
  ... do the drop
} else if (ifExists) {
  // do nothing
} else {
  log.warn("The table to drop does not exist: " +tableName)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We can also update the UNCACHE TABLE, cc @gatorsmile

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll update soon.

@SparkQA
Copy link

SparkQA commented Dec 5, 2017

Test build #84463 has finished for PR 19888 at commit b2b4d54.

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

@dongjoon-hyun
Copy link
Member Author

I added a VIEW case and we need to throw AnalysisException if ifExists=false.

@gatorsmile
Copy link
Member

LGTM pending Jenkins

@dongjoon-hyun
Copy link
Member Author

Thank you, @gatorsmile .

@SparkQA
Copy link

SparkQA commented Dec 5, 2017

Test build #84495 has finished for PR 19888 at commit 4b6041b.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-22686][SQL] DROP TABLE IF EXISTS should not throw AnalysisException [SPARK-22686][SQL] DROP TABLE IF EXISTS should not show AnalysisException Dec 5, 2017
@cloud-fan
Copy link
Contributor

thanks, merging to master/2.2!

asfgit pushed a commit that referenced this pull request Dec 6, 2017
…tion

## What changes were proposed in this pull request?

During [SPARK-22488](#19713) to fix view resolution issue, there occurs a regression at `2.2.1` and `master` branch like the following. This PR fixes that.

```scala
scala> spark.version
res2: String = 2.2.1

scala> sql("DROP TABLE IF EXISTS t").show
17/12/04 21:01:06 WARN DropTableCommand: org.apache.spark.sql.AnalysisException:
Table or view not found: t;
org.apache.spark.sql.AnalysisException: Table or view not found: t;
```

## How was this patch tested?

Manual.

Author: Dongjoon Hyun <dongjoon@apache.org>

Closes #19888 from dongjoon-hyun/SPARK-22686.

(cherry picked from commit 82183f7)
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
@asfgit asfgit closed this in 82183f7 Dec 6, 2017
@dongjoon-hyun
Copy link
Member Author

Thank you, @cloud-fan and @gatorsmile !

@dongjoon-hyun dongjoon-hyun deleted the SPARK-22686 branch December 6, 2017 03:38
MatthewRBruce pushed a commit to Shopify/spark that referenced this pull request Jul 31, 2018
…tion

## What changes were proposed in this pull request?

During [SPARK-22488](apache#19713) to fix view resolution issue, there occurs a regression at `2.2.1` and `master` branch like the following. This PR fixes that.

```scala
scala> spark.version
res2: String = 2.2.1

scala> sql("DROP TABLE IF EXISTS t").show
17/12/04 21:01:06 WARN DropTableCommand: org.apache.spark.sql.AnalysisException:
Table or view not found: t;
org.apache.spark.sql.AnalysisException: Table or view not found: t;
```

## How was this patch tested?

Manual.

Author: Dongjoon Hyun <dongjoon@apache.org>

Closes apache#19888 from dongjoon-hyun/SPARK-22686.

(cherry picked from commit 82183f7)
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants