Skip to content

[SPARK-31100][SQL] Check namespace existens when setting namespace #27900

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

Jump to bottom
Closed
jackylee-ch wants to merge 5 commits into from
Closed

[SPARK-31100][SQL] Check namespace existens when setting namespace #27900

jackylee-ch wants to merge 5 commits into from

Conversation

@jackylee-ch
Copy link
Contributor

@jackylee-ch jackylee-ch commented Mar 13, 2020

What changes were proposed in this pull request?

Check the namespace existence while calling "use namespace", and throw NoSuchNamespaceException if namespace not exists.

Why are the changes needed?

Users need to know that the namespace does not exist when they try to set a wrong namespace.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Run all suites and add a test for this

[SPARK-31100] Check namespace existens when setting namespace
9c72aac 
Change-Id: Ibbb3eff4e56d5cf725ebce1e35445bc1155d8d90
@jackylee-ch
Copy link
Contributor Author

jackylee-ch commented Mar 13, 2020

cc @HyukjinKwon @cloud-fan @dongjoon-hyun

@jackylee-ch
Copy link
Contributor Author

jackylee-ch commented Mar 17, 2020

cc @rxin @xuanyuanking

@github-actions
Copy link

github-actions bot commented Jul 1, 2020

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

@github-actions github-actions bot added the Stale label Jul 1, 2020
@cloud-fan cloud-fan removed the Stale label Jul 1, 2020
@cloud-fan
Copy link
Contributor

cloud-fan commented Jul 1, 2020

how about this: we check namespace existence if the catalog implements SupportsNamespace. Otherwise, we just allow it.

@jackylee-ch
Copy link
Contributor Author

jackylee-ch commented Jul 1, 2020

how about this: we check namespace existence if the catalog implements SupportsNamespace. Otherwise, we just allow it.

Ah, that's a better idea.
thanks @cloud-fan

enable setCurrentNamespace on Catalog which not implements SupportsNa…
0623c22 
…mespaces

Change-Id: I8eb09649a8ce294c257bcba1313f89dfe57c912f
@cloud-fan
Copy link
Contributor

cloud-fan commented Jul 1, 2020

ok to test

cloud-fan
cloud-fan reviewed Jul 1, 2020
View reviewed changes
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/CatalogManagerSuite.scala Outdated
assert(v1SessionCatalog.getCurrentDatabase == "default")

// Check namespace existence if currentCatalog implements SupportsNamespaces.
conf.setConfString("spark.sql.catalog.dummyNamespace", classOf[DummyNameSpaceCatalog].getName)
Copy link
Contributor

@cloud-fan cloud-fan Jul 1, 2020

Choose a reason for hiding this comment

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

We can reuse InMemoryTableCatalog and create some namespaces before testing setCurrentNamespace

chagne suite
0b5536a 
Change-Id: I934a73eeba768abe3dd20d06edcb4931c89dd54e
@SparkQA
Copy link

SparkQA commented Jul 1, 2020

Test build #124796 has finished for PR 27900 at commit 0b5536a.

  • This patch fails to generate documentation.
  • This patch merges cleanly.
  • This patch adds no public classes.

cloud-fan
cloud-fan approved these changes Jul 1, 2020
View reviewed changes
Copy link
Contributor

@cloud-fan cloud-fan left a comment

Choose a reason for hiding this comment

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

LGTM. This behavior is also consistent with the v1 session catalog.

@cloud-fan
Copy link
Contributor

cloud-fan commented Jul 1, 2020

cc @rdblue

@rdblue
Copy link
Contributor

rdblue commented Jul 1, 2020

Sounds good to me. +1

@SparkQA
Copy link

SparkQA commented Jul 1, 2020

Test build #124787 has finished for PR 27900 at commit 0623c22.

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

@SparkQA
Copy link

SparkQA commented Jul 1, 2020

Test build #5050 has finished for PR 27900 at commit 0b5536a.

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

fix test failed
6fa00aa 
Change-Id: I77526c816650e368092e882e202e4a537a5b4720
@jackylee-ch
Copy link
Contributor Author

jackylee-ch commented Jul 2, 2020

@cloud-fan @rdblue can we reset build?

cloud-fan
cloud-fan reviewed Jul 2, 2020
View reviewed changes
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala Outdated
assert(exception.getMessage.contains("Database 'ns1' not found"))
}

test("Use: v2 catalog is used and namespace does not exist") {
Copy link
Contributor

@cloud-fan cloud-fan Jul 2, 2020

Choose a reason for hiding this comment

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

we should update this test with catalogs that don't implement SupportsNamespace

cloud-fan
cloud-fan reviewed Jul 2, 2020
View reviewed changes
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala Outdated

sql("USE testcat")
testShowCurrentNamespace("testcat", "")
sql("create namespace testcat.ns1.ns2")
Copy link
Contributor

@cloud-fan cloud-fan Jul 2, 2020

Choose a reason for hiding this comment

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

please upper case the SQL keywords

cloud-fan
cloud-fan reviewed Jul 2, 2020
View reviewed changes
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala Outdated
spark.conf.unset(V2_SESSION_CATALOG_IMPLEMENTATION.key)
val sessionCatalogName = CatalogManager.SESSION_CATALOG_NAME

sql("create namespace testcat.ns1.ns2")
Copy link
Contributor

@cloud-fan cloud-fan Jul 2, 2020

Choose a reason for hiding this comment

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

ditto

add suites
d888906 
Change-Id: I3f7a2440621747024ec239a23c014a716b9f7bec
@SparkQA
Copy link

SparkQA commented Jul 2, 2020

Test build #124832 has finished for PR 27900 at commit 6fa00aa.

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

@SparkQA
Copy link

SparkQA commented Jul 2, 2020

Test build #124863 has finished for PR 27900 at commit d888906.

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

@cloud-fan
Copy link
Contributor

cloud-fan commented Jul 2, 2020

retest this please

@SparkQA
Copy link

SparkQA commented Jul 2, 2020

Test build #124880 has finished for PR 27900 at commit d888906.

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

@cloud-fan
Copy link
Contributor

cloud-fan commented Jul 2, 2020

thanks, merging to master!

@cloud-fan cloud-fan closed this in f082a79 Jul 2, 2020
@jackylee-ch jackylee-ch deleted the SPARK-31100 branch July 2, 2020 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloud-fan cloud-fan cloud-fan approved these changes

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jackylee-ch @cloud-fan @SparkQA @rdblue @dongjoon-hyun