Skip to content

Conversation

@imback82
Copy link
Contributor

What changes were proposed in this pull request?

This PR adds DROP NAMESPACE support for V2 catalogs.

Why are the changes needed?

Currently, you cannot drop namespaces for v2 catalogs.

Does this PR introduce any user-facing change?

The user can now perform the following:

CREATE NAMESPACE mycatalog.ns
DROP NAMESPACE mycatalog.ns
SHOW NAMESPACES IN mycatalog # Will show no namespaces 

to drop a namespace ns inside mycatalog V2 catalog.

How was this patch tested?

Added unit tests.

@imback82
Copy link
Contributor Author

cc: @cloud-fan @rdblue

cascade: Boolean)
extends V2CommandExec {
override protected def run(): Seq[InternalRow] = {
// TODO: How to handle when cascade is true?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rdblue / @cloud-fan did we discuss how we want to drop namespaces if cascade is set for v2 catalogs?

Copy link
Contributor

Choose a reason for hiding this comment

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

In the v2 API, namespaces that are not empty can be rejected by throwing IllegalStateException. I'd say catch that if cascade is set and throw an error that cascade isn't supported.

Copy link
Contributor

Choose a reason for hiding this comment

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

yea this is a good workaround now. I'm wondering if we should support cascade before 3.0, as this may break DS v2 APIs, e.g. we may need to add a cascade parameter to the dropNamespace API. DROP TABLE has the same problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I updated with a workaround solution. And +1 for adding cascade option to dropNamespace API. I initially tried to implement cascading option with existing APIs, but it was a bit cumbersome.

@SparkQA
Copy link

SparkQA commented Oct 26, 2019

Test build #112691 has finished for PR 26262 at commit 588b2ce.

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

@dongjoon-hyun
Copy link
Member

Could you resolve the conflicts, @imback82 ?

@SparkQA
Copy link

SparkQA commented Oct 28, 2019

Test build #112788 has finished for PR 26262 at commit 634952e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class Version() extends LeafExpression with CodegenFallback
  • case class LoadDataStatement(
  • case class LocalShuffleReaderExec(child: QueryStageExec) extends UnaryExecNode

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Thank you, @imback82 , @cloud-fan , @rdblue .
Merged to master.

@imback82
Copy link
Contributor Author

Thanks everyone for help!

@imback82 imback82 deleted the drop_namespace branch October 29, 2019 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants