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-30420][SQL][FOLLOWUP] Remove statement logical plans for namespace commands #27125

Closed
wants to merge 2 commits into from

Conversation

imback82
Copy link
Contributor

@imback82 imback82 commented Jan 8, 2020

What changes were proposed in this pull request?

This is a follow-up to address the following comment: #27095 (comment)

Currently, a SQL command string is parsed to a "statement" logical plan, converted to a logical plan with catalog/namespace, then finally converted to a physical plan. With the new resolution framework, there is no need to create a "statement" logical plan; a logical plan can contain UnresolvedNamespace which will be resolved to a ResolvedNamespace. This should simply the code base and make it a bit easier to add a new command.

Why are the changes needed?

Clean up codebase.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing tests should cover the changes.

@imback82
Copy link
Contributor Author

imback82 commented Jan 8, 2020

cc: @cloud-fan

@SparkQA
Copy link

SparkQA commented Jan 8, 2020

Test build #116278 has finished for PR 27125 at commit f65672f.

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

@cloud-fan cloud-fan closed this in b2ed6d0 Jan 8, 2020
@cloud-fan
Copy link
Contributor

thanks, merging to master!

@imback82 imback82 changed the title [SPARK-30214][SQL][FOLLOWUP] Remove statement logical plans for namespace commands [SPARK-30420][SQL][FOLLOWUP] Remove statement logical plans for namespace commands Jan 16, 2020
DescribeNamespace(catalog, ns, extended)

case ShowNamespacesStatement(NonSessionCatalogAndNamespace(catalog, ns), pattern) =>
ShowNamespaces(catalog, ns, pattern)
Copy link
Contributor

Choose a reason for hiding this comment

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

We missed the fallback to the v1 ShowDatabasesCommand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me check, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cloud-fan could you clarify on the fallback?

"show databases" and "show namespaces" were merged by #26006 and both are handled by v2 ShowNamespacesExec.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I misremember that. So we forgot to add a migration guide for the schema change... (databaseName -> namespace) Anyway it's too late now, let's catch schema changes in the following development work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants