Skip to content

Conversation

@gengliangwang
Copy link
Member

What changes were proposed in this pull request?

In the PR #20886, I mistakenly check the table location only when ignoreIfExists is false, which was following the original deprecated PR.
That was wrong. When ignoreIfExists is true and the target table doesn't exist, we should also check the table location. In other word, ignoreIfExists has nothing to do with table location validation.
This is a follow-up PR to fix the mistake.

How was this patch tested?

Add one unit test.

@gengliangwang
Copy link
Member Author

@cloud-fan @gatorsmile

@SparkQA
Copy link

SparkQA commented Apr 7, 2018

Test build #89011 has finished for PR 21001 at commit 3d1c909.

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

@gengliangwang
Copy link
Member Author

retest this please.

@SparkQA
Copy link

SparkQA commented Apr 7, 2018

Test build #89017 has finished for PR 21001 at commit 3d1c909.

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

@SparkQA
Copy link

SparkQA commented Apr 9, 2018

Test build #89050 has finished for PR 21001 at commit 3fe648f.

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

Copy link
Member Author

Choose a reason for hiding this comment

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

The error message is

"Table or view 'tbl' already exists in database 'default';"

This is soft of behavior change, as we check table existence first.

Copy link
Member

Choose a reason for hiding this comment

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

Instead of deleting the test, we should change the name of the table

Copy link
Member Author

Choose a reason for hiding this comment

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

We check table existence in SessionCatalog instead of ExternalCatalog now.
SessionCatalogSuite has similar test case, so no need to create new one.

@SparkQA
Copy link

SparkQA commented Apr 9, 2018

Test build #89072 has finished for PR 21001 at commit 1e27cc8.

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

@SparkQA
Copy link

SparkQA commented Apr 9, 2018

Test build #89071 has finished for PR 21001 at commit 577a399.

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

@SparkQA
Copy link

SparkQA commented Apr 10, 2018

Test build #89080 has finished for PR 21001 at commit 3f3391b.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

We should not remove this logic here, even it's also done in SessionCatalog

@SparkQA
Copy link

SparkQA commented Apr 10, 2018

Test build #89115 has finished for PR 21001 at commit ffd0c66.

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

@gatorsmile
Copy link
Member

LGTM

Thanks! Merged to master.

@asfgit asfgit closed this in e179658 Apr 10, 2018
@SparkQA
Copy link

SparkQA commented Apr 10, 2018

Test build #89124 has finished for PR 21001 at commit c4f359a.

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

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.

4 participants