[SPARK-29772][TESTS][SQL] Add withNamespace in SQLTestUtils #26411
Conversation
ulysses-you
changed the title
|
ok to test |
|
cc @cloud-fan and @rdblue |
|
Test build #113348 has finished for PR 26411 at commit
|
| namespaces.foreach { name => | ||
| spark.sql(s"DROP NAMESPACE IF EXISTS $name CASCADE") | ||
| } | ||
| spark.sql(s"USE default") |
There was a problem hiding this comment.
We should set back to the default namespace of the current catalog. Not sure we have such a command yet, cc @imback82
There was a problem hiding this comment.
We can get current catalog and set back like this code.
spark.sessionState.catalogManager.currentCatalog match {
case supportNamespace: SupportsNamespaces =>
supportNamespace.asNamespaceCatalog.defaultNamespace() match {
case Array(name) => spark.sql(s"USE $name")
case Array(name, _*) => spark.sql(s"USE $name")
case _ => spark.sql("USE default")
}
case _ => spark.sql("USE default")
}
There was a problem hiding this comment.
Let's remove it first, and assume the test code won't switch the current namespace.
Things can get complicated if the test code switches the current catalog.
There was a problem hiding this comment.
Shall we add some doc to remind like this ?
If you will change current catalog, you should use this method carefully. This may be remove the wrong namespace.
There was a problem hiding this comment.
how about we add doc to remind callers to not switch current namespace?
There was a problem hiding this comment.
Updated, please take an another review.
There was a problem hiding this comment.
We should set back to the default namespace of the current catalog. Not sure we have such a command yet, cc @imback82
No, we don't have a such a command yet. Do you think it will be useful to have one?
This also reminds me about the behavior of dropping namespace. If you are trying to drop a namespace, which is set to the current namespace, this shouldn't be allowed right? We don't have a such a check yet, but I can add one if needed.
| namespaces.foreach { name => | ||
| spark.sql(s"DROP NAMESPACE IF EXISTS $name CASCADE") | ||
| } | ||
| import org.apache.spark.sql.connector.catalog.CatalogV2Implicits._ |
There was a problem hiding this comment.
let's make it simple and not add complexity before we really need it.
For now all the caller sides do not switch current catalog/namespace. We can just drop the namespace here, with doc saying callers should not switch current catalog/namespace.
|
|
||
| /** | ||
| * Drops namespace `namespace` after calling `f`. | ||
| * Note that, do not switch current catalog in `f`. It may remove the wrong namespace. |
There was a problem hiding this comment.
Note that, if you switch current catalog/namespace in f, you should switch it back manually.
|
Test build #113384 has finished for PR 26411 at commit
|
|
Test build #113390 has finished for PR 26411 at commit
|
|
Test build #113411 has finished for PR 26411 at commit
|
|
thanks, merging to master! |
|
thanks for merging! |
6 participants
What changes were proposed in this pull request?
V2 catalog support namespace, we should add
withNamespacelikewithDatabase.Why are the changes needed?
Make test easy.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Add UT.