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-36841][SQL] Add ansi syntax set catalog xxx to change the current catalog #34096

Closed
wants to merge 1 commit into from

Conversation

Peng-Lei
Copy link
Contributor

@Peng-Lei Peng-Lei commented Sep 24, 2021

What changes were proposed in this pull request?

1、Add the statement of set catalog xxx to change the current catalog
2、Retain the USE statement to change the current catalog
3、Forcible loading the new catalog when change the new catalog.

Why are the changes needed?

Ansi SQL use SET CATALOG XXX statement to change the catalog.

DISCUSS

set-catalog

Does this PR introduce any user-facing change?

Yes, User can use SET CATALOG XXX to change the current catalog

How was this patch tested?

Add ut testcase

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@Peng-Lei
Copy link
Contributor Author

@cloud-fan @imback82 Could you take a look ? Thank you.

@@ -120,6 +120,7 @@ class CatalogManager(
def setCurrentCatalog(catalogName: String): Unit = synchronized {
// `setCurrentCatalog` is noop if it doesn't switch to a different catalog.
if (currentCatalog.name() != catalogName) {
catalog(catalogName)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I just want to check the catalogName is exist and catalog is configed.

@@ -107,6 +107,7 @@ statement
: query #statementDefault
| ctes? dmlStatementNoWith #dmlStatement
| USE NAMESPACE? multipartIdentifier #use
| SET CATALOG multipartIdentifier #setCatalog
Copy link
Contributor

Choose a reason for hiding this comment

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

multipartIdentifier? I think it should be identifier | STRING, so that people can write SET CATALOG abc or SET CATALOG '_ 56'

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I get it.

@@ -2916,6 +2916,22 @@ class DataSourceV2SQLSuite
}
}

test("SPARK-36481: Test for SET CATALOG statement") {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add UT in DDLParserSuite?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok.

/**
* A SetCatalog statement, as parsed from SQL.
*/
case class SetCatalogStatement(nameParts: Option[String]) extends LeafParsedStatement
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just make this nameParts: String without Option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your advice. Done

@cloud-fan
Copy link
Contributor

SET CATALOG is a very simple command that we can just implement as RunnableCommand. Can you follow #34128 and simplify this command as well?

@Peng-Lei
Copy link
Contributor Author

SET CATALOG is a very simple command that we can just implement as RunnableCommand. Can you follow #34128 and simplify this command as well?

ok. Thank you for the reminder.

@Peng-Lei Peng-Lei force-pushed the set-catalog-statement branch 2 times, most recently from 8d29466 to 7d24ab7 Compare September 29, 2021 07:14
@@ -107,6 +107,7 @@ statement
: query #statementDefault
| ctes? dmlStatementNoWith #dmlStatement
| USE NAMESPACE? multipartIdentifier #use
| SET CATALOG catalogIdentifier #setCatalog
Copy link
Contributor

Choose a reason for hiding this comment

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

is it simpler to do SET CATALOG identifier | STRING

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

} else if (ctx.catalogIdentifier.STRING() != null) {
string(ctx.catalogIdentifier().STRING())
} else {
ctx.catalogIdentifier().getText
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 never hit here. How about throw IllegalStateException here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, How about QueryParsingErrors.invalidCatalogNameError(ctx) with ParseException ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a user-facing error, so we shouldn't use QueryParsingErrors

@Peng-Lei
Copy link
Contributor Author

Peng-Lei commented Sep 29, 2021

@cloud-fan A little off-topic: I also think SHOW CATALOGS #33175 is a very simple command that we can just implement as RunnableCommand follow #34128. Could I continue to update after holiday?

@cloud-fan
Copy link
Contributor

Could I continue to update after holiday?

Yes thanks!

@cloud-fan
Copy link
Contributor

thanks, merging to master!

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