You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/09/24 10:00:29 UTC
[GitHub] [spark] Peng-Lei opened a new pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Peng-Lei opened a new pull request #34096:
URL: https://github.com/apache/spark/pull/34096
### 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.
### 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Peng-Lei commented on pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
Peng-Lei commented on pull request #34096:
URL: https://github.com/apache/spark/pull/34096#issuecomment-929801182
> `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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Peng-Lei commented on a change in pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
Peng-Lei commented on a change in pull request #34096:
URL: https://github.com/apache/spark/pull/34096#discussion_r718094327
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala
##########
@@ -278,6 +278,11 @@ case class InsertIntoStatement(
*/
case class UseStatement(isNamespaceSet: Boolean, nameParts: Seq[String]) extends LeafParsedStatement
+/**
+ * A SetCatalog statement, as parsed from SQL.
+ */
+case class SetCatalogStatement(nameParts: Option[String]) extends LeafParsedStatement
Review comment:
Thank you for your advice. Done
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -107,6 +107,7 @@ statement
: query #statementDefault
| ctes? dmlStatementNoWith #dmlStatement
| USE NAMESPACE? multipartIdentifier #use
+ | SET CATALOG catalogIdentifier #setCatalog
Review comment:
ok
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala
##########
@@ -404,4 +404,13 @@ class DDLParserSuite extends AnalysisTest with SharedSparkSession {
assert(fileFormat6.locationUri.isEmpty)
assert(provider6 == Some("ORC"))
}
+
+ test("SET CATALOG") {
+ comparePlans(
+ parser.parsePlan("SET CATALOG abc"),
+ SetCatalogCommand("abc"))
+ comparePlans(
+ parser.parsePlan("SET CATALOG 'a b c'"),
Review comment:
ok
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -239,6 +239,21 @@ class SparkSqlAstBuilder extends AstBuilder {
ShowCurrentNamespaceCommand()
}
+ /**
+ * Create a [[SetCatalogCommand]] logical command.
+ */
+ override def visitSetCatalog(ctx: SetCatalogContext): LogicalPlan = withOrigin(ctx) {
+ val name =
+ if (ctx.catalogIdentifier().identifier() != null) {
+ ctx.catalogIdentifier().identifier().getText
+ } else if (ctx.catalogIdentifier.STRING() != null) {
+ string(ctx.catalogIdentifier().STRING())
+ } else {
+ ctx.catalogIdentifier().getText
Review comment:
OK, How about `QueryParsingErrors.invalidCatalogNameError(ctx)` with `ParseException` ?
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala
##########
@@ -404,4 +404,13 @@ class DDLParserSuite extends AnalysisTest with SharedSparkSession {
assert(fileFormat6.locationUri.isEmpty)
assert(provider6 == Some("ORC"))
}
+
+ test("SET CATALOG") {
+ comparePlans(
+ parser.parsePlan("SET CATALOG abc"),
+ SetCatalogCommand("abc"))
+ comparePlans(
+ parser.parsePlan("SET CATALOG 'a b c'"),
Review comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] imback82 commented on a change in pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
imback82 commented on a change in pull request #34096:
URL: https://github.com/apache/spark/pull/34096#discussion_r717748633
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala
##########
@@ -278,6 +278,11 @@ case class InsertIntoStatement(
*/
case class UseStatement(isNamespaceSet: Boolean, nameParts: Seq[String]) extends LeafParsedStatement
+/**
+ * A SetCatalog statement, as parsed from SQL.
+ */
+case class SetCatalogStatement(nameParts: Option[String]) extends LeafParsedStatement
Review comment:
Can't we just make this `nameParts: String` without `Option`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Peng-Lei commented on a change in pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
Peng-Lei commented on a change in pull request #34096:
URL: https://github.com/apache/spark/pull/34096#discussion_r718270663
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala
##########
@@ -404,4 +404,13 @@ class DDLParserSuite extends AnalysisTest with SharedSparkSession {
assert(fileFormat6.locationUri.isEmpty)
assert(provider6 == Some("ORC"))
}
+
+ test("SET CATALOG") {
+ comparePlans(
+ parser.parsePlan("SET CATALOG abc"),
+ SetCatalogCommand("abc"))
+ comparePlans(
+ parser.parsePlan("SET CATALOG 'a b c'"),
Review comment:
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Peng-Lei commented on pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
Peng-Lei commented on pull request #34096:
URL: https://github.com/apache/spark/pull/34096#issuecomment-929801182
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #34096:
URL: https://github.com/apache/spark/pull/34096#discussion_r718243116
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -239,6 +239,21 @@ class SparkSqlAstBuilder extends AstBuilder {
ShowCurrentNamespaceCommand()
}
+ /**
+ * Create a [[SetCatalogCommand]] logical command.
+ */
+ override def visitSetCatalog(ctx: SetCatalogContext): LogicalPlan = withOrigin(ctx) {
+ val name =
+ if (ctx.catalogIdentifier().identifier() != null) {
+ ctx.catalogIdentifier().identifier().getText
+ } else if (ctx.catalogIdentifier.STRING() != null) {
+ string(ctx.catalogIdentifier().STRING())
+ } else {
+ ctx.catalogIdentifier().getText
Review comment:
We should never hit here. How about throw `IllegalStateException` here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Peng-Lei commented on a change in pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
Peng-Lei commented on a change in pull request #34096:
URL: https://github.com/apache/spark/pull/34096#discussion_r717152083
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -107,6 +107,7 @@ statement
: query #statementDefault
| ctes? dmlStatementNoWith #dmlStatement
| USE NAMESPACE? multipartIdentifier #use
+ | SET CATALOG multipartIdentifier #setCatalog
Review comment:
Thank you. I get it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Peng-Lei edited a comment on pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
Peng-Lei edited a comment on pull request #34096:
URL: https://github.com/apache/spark/pull/34096#issuecomment-930062048
@cloud-fan A little off-topic: I also think `SHOW CATALOGS` [#33175](https://github.com/apache/spark/pull/33175) is a very simple command that we can just implement as RunnableCommand follow [#34128](https://github.com/apache/spark/pull/34128). Could I continue to update after holiday?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #34096:
URL: https://github.com/apache/spark/pull/34096#issuecomment-929778730
`SET CATALOG` is a very simple command that we can just implement as `RunnableCommand`. Can you follow https://github.com/apache/spark/pull/34128 and simplify this command as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Peng-Lei commented on a change in pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
Peng-Lei commented on a change in pull request #34096:
URL: https://github.com/apache/spark/pull/34096#discussion_r716163006
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogManager.scala
##########
@@ -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)
Review comment:
Here I just want to check the catalogName is exist and catalog is configed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #34096:
URL: https://github.com/apache/spark/pull/34096#discussion_r716474146
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala
##########
@@ -2916,6 +2916,22 @@ class DataSourceV2SQLSuite
}
}
+ test("SPARK-36481: Test for SET CATALOG statement") {
Review comment:
can we add UT in `DDLParserSuite`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #34096:
URL: https://github.com/apache/spark/pull/34096#issuecomment-934229504
thanks, merging to master!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Peng-Lei commented on a change in pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
Peng-Lei commented on a change in pull request #34096:
URL: https://github.com/apache/spark/pull/34096#discussion_r718241158
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -107,6 +107,7 @@ statement
: query #statementDefault
| ctes? dmlStatementNoWith #dmlStatement
| USE NAMESPACE? multipartIdentifier #use
+ | SET CATALOG catalogIdentifier #setCatalog
Review comment:
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #34096:
URL: https://github.com/apache/spark/pull/34096#discussion_r718240441
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -107,6 +107,7 @@ statement
: query #statementDefault
| ctes? dmlStatementNoWith #dmlStatement
| USE NAMESPACE? multipartIdentifier #use
+ | SET CATALOG catalogIdentifier #setCatalog
Review comment:
is it simpler to do `SET CATALOG identifier | STRING`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan closed pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #34096:
URL: https://github.com/apache/spark/pull/34096
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Peng-Lei edited a comment on pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
Peng-Lei edited a comment on pull request #34096:
URL: https://github.com/apache/spark/pull/34096#issuecomment-930062048
@cloud-fan A little off-topic: I also think `SHOW CATALOGS` [#33175](https://github.com/apache/spark/pull/33175) is a very simple command that we can just implement as RunnableCommand follow [#34128](https://github.com/apache/spark/pull/34128). Could I continue to update after holiday?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #34096:
URL: https://github.com/apache/spark/pull/34096#discussion_r718240441
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -107,6 +107,7 @@ statement
: query #statementDefault
| ctes? dmlStatementNoWith #dmlStatement
| USE NAMESPACE? multipartIdentifier #use
+ | SET CATALOG catalogIdentifier #setCatalog
Review comment:
is it simpler to do `SET CATALOG identifier | STRING`
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -239,6 +239,21 @@ class SparkSqlAstBuilder extends AstBuilder {
ShowCurrentNamespaceCommand()
}
+ /**
+ * Create a [[SetCatalogCommand]] logical command.
+ */
+ override def visitSetCatalog(ctx: SetCatalogContext): LogicalPlan = withOrigin(ctx) {
+ val name =
+ if (ctx.catalogIdentifier().identifier() != null) {
+ ctx.catalogIdentifier().identifier().getText
+ } else if (ctx.catalogIdentifier.STRING() != null) {
+ string(ctx.catalogIdentifier().STRING())
+ } else {
+ ctx.catalogIdentifier().getText
Review comment:
We should never hit here. How about throw `IllegalStateException` here?
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala
##########
@@ -404,4 +404,13 @@ class DDLParserSuite extends AnalysisTest with SharedSparkSession {
assert(fileFormat6.locationUri.isEmpty)
assert(provider6 == Some("ORC"))
}
+
+ test("SET CATALOG") {
+ comparePlans(
+ parser.parsePlan("SET CATALOG abc"),
+ SetCatalogCommand("abc"))
+ comparePlans(
+ parser.parsePlan("SET CATALOG 'a b c'"),
Review comment:
let's test this as well
```
SET CATALOG `a b c`
```
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -239,6 +239,21 @@ class SparkSqlAstBuilder extends AstBuilder {
ShowCurrentNamespaceCommand()
}
+ /**
+ * Create a [[SetCatalogCommand]] logical command.
+ */
+ override def visitSetCatalog(ctx: SetCatalogContext): LogicalPlan = withOrigin(ctx) {
+ val name =
+ if (ctx.catalogIdentifier().identifier() != null) {
+ ctx.catalogIdentifier().identifier().getText
+ } else if (ctx.catalogIdentifier.STRING() != null) {
+ string(ctx.catalogIdentifier().STRING())
+ } else {
+ ctx.catalogIdentifier().getText
Review comment:
It's not a user-facing error, so we shouldn't use `QueryParsingErrors`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Peng-Lei commented on a change in pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
Peng-Lei commented on a change in pull request #34096:
URL: https://github.com/apache/spark/pull/34096#discussion_r718094327
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala
##########
@@ -278,6 +278,11 @@ case class InsertIntoStatement(
*/
case class UseStatement(isNamespaceSet: Boolean, nameParts: Seq[String]) extends LeafParsedStatement
+/**
+ * A SetCatalog statement, as parsed from SQL.
+ */
+case class SetCatalogStatement(nameParts: Option[String]) extends LeafParsedStatement
Review comment:
Thank you for your advice. Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Peng-Lei commented on pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
Peng-Lei commented on pull request #34096:
URL: https://github.com/apache/spark/pull/34096#issuecomment-930062048
@cloud-fan A little off-topic: I also think `SHOW CATALOGS` [#33175](https://github.com/apache/spark/pull/33175) is a very simple command that we can just implement as RunnableCommand follow [#34128](https://github.com/apache/spark/pull/34128). Could I
continue to update after holiday?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] imback82 commented on a change in pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
imback82 commented on a change in pull request #34096:
URL: https://github.com/apache/spark/pull/34096#discussion_r717748633
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala
##########
@@ -278,6 +278,11 @@ case class InsertIntoStatement(
*/
case class UseStatement(isNamespaceSet: Boolean, nameParts: Seq[String]) extends LeafParsedStatement
+/**
+ * A SetCatalog statement, as parsed from SQL.
+ */
+case class SetCatalogStatement(nameParts: Option[String]) extends LeafParsedStatement
Review comment:
Can't we just make this `nameParts: String` without `Option`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gengliangwang commented on a change in pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on a change in pull request #34096:
URL: https://github.com/apache/spark/pull/34096#discussion_r716793599
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -107,6 +107,7 @@ statement
: query #statementDefault
| ctes? dmlStatementNoWith #dmlStatement
| USE NAMESPACE? multipartIdentifier #use
+ | SET CATALOG multipartIdentifier #setCatalog
Review comment:
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Peng-Lei commented on pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
Peng-Lei commented on pull request #34096:
URL: https://github.com/apache/spark/pull/34096#issuecomment-926514085
@cloud-fan @imback82 Could you take a look ? Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Peng-Lei commented on a change in pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
Peng-Lei commented on a change in pull request #34096:
URL: https://github.com/apache/spark/pull/34096#discussion_r717152256
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala
##########
@@ -2916,6 +2916,22 @@ class DataSourceV2SQLSuite
}
}
+ test("SPARK-36481: Test for SET CATALOG statement") {
Review comment:
ok.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #34096:
URL: https://github.com/apache/spark/pull/34096#issuecomment-929778730
`SET CATALOG` is a very simple command that we can just implement as `RunnableCommand`. Can you follow https://github.com/apache/spark/pull/34128 and simplify this command as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Peng-Lei commented on a change in pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
Peng-Lei commented on a change in pull request #34096:
URL: https://github.com/apache/spark/pull/34096#discussion_r717152083
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -107,6 +107,7 @@ statement
: query #statementDefault
| ctes? dmlStatementNoWith #dmlStatement
| USE NAMESPACE? multipartIdentifier #use
+ | SET CATALOG multipartIdentifier #setCatalog
Review comment:
Thank you. I get it.
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala
##########
@@ -2916,6 +2916,22 @@ class DataSourceV2SQLSuite
}
}
+ test("SPARK-36481: Test for SET CATALOG statement") {
Review comment:
ok.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #34096:
URL: https://github.com/apache/spark/pull/34096#discussion_r718245828
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala
##########
@@ -404,4 +404,13 @@ class DDLParserSuite extends AnalysisTest with SharedSparkSession {
assert(fileFormat6.locationUri.isEmpty)
assert(provider6 == Some("ORC"))
}
+
+ test("SET CATALOG") {
+ comparePlans(
+ parser.parsePlan("SET CATALOG abc"),
+ SetCatalogCommand("abc"))
+ comparePlans(
+ parser.parsePlan("SET CATALOG 'a b c'"),
Review comment:
let's test this as well
```
SET CATALOG `a b c`
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Peng-Lei commented on a change in pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
Peng-Lei commented on a change in pull request #34096:
URL: https://github.com/apache/spark/pull/34096#discussion_r718271842
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -239,6 +239,21 @@ class SparkSqlAstBuilder extends AstBuilder {
ShowCurrentNamespaceCommand()
}
+ /**
+ * Create a [[SetCatalogCommand]] logical command.
+ */
+ override def visitSetCatalog(ctx: SetCatalogContext): LogicalPlan = withOrigin(ctx) {
+ val name =
+ if (ctx.catalogIdentifier().identifier() != null) {
+ ctx.catalogIdentifier().identifier().getText
+ } else if (ctx.catalogIdentifier.STRING() != null) {
+ string(ctx.catalogIdentifier().STRING())
+ } else {
+ ctx.catalogIdentifier().getText
Review comment:
OK, How about `QueryParsingErrors.invalidCatalogNameError(ctx)` with `ParseException` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #34096:
URL: https://github.com/apache/spark/pull/34096#discussion_r716472854
##########
File path: sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4
##########
@@ -107,6 +107,7 @@ statement
: query #statementDefault
| ctes? dmlStatementNoWith #dmlStatement
| USE NAMESPACE? multipartIdentifier #use
+ | SET CATALOG multipartIdentifier #setCatalog
Review comment:
multipartIdentifier? I think it should be `identifier | STRING`, so that people can write `SET CATALOG abc` or `SET CATALOG '_ 56'`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #34096:
URL: https://github.com/apache/spark/pull/34096#discussion_r718273251
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
##########
@@ -239,6 +239,21 @@ class SparkSqlAstBuilder extends AstBuilder {
ShowCurrentNamespaceCommand()
}
+ /**
+ * Create a [[SetCatalogCommand]] logical command.
+ */
+ override def visitSetCatalog(ctx: SetCatalogContext): LogicalPlan = withOrigin(ctx) {
+ val name =
+ if (ctx.catalogIdentifier().identifier() != null) {
+ ctx.catalogIdentifier().identifier().getText
+ } else if (ctx.catalogIdentifier.STRING() != null) {
+ string(ctx.catalogIdentifier().STRING())
+ } else {
+ ctx.catalogIdentifier().getText
Review comment:
It's not a user-facing error, so we shouldn't use `QueryParsingErrors`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Peng-Lei commented on a change in pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
Peng-Lei commented on a change in pull request #34096:
URL: https://github.com/apache/spark/pull/34096#discussion_r718381821
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala
##########
@@ -404,4 +404,13 @@ class DDLParserSuite extends AnalysisTest with SharedSparkSession {
assert(fileFormat6.locationUri.isEmpty)
assert(provider6 == Some("ORC"))
}
+
+ test("SET CATALOG") {
+ comparePlans(
+ parser.parsePlan("SET CATALOG abc"),
+ SetCatalogCommand("abc"))
+ comparePlans(
+ parser.parsePlan("SET CATALOG 'a b c'"),
Review comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #34096:
URL: https://github.com/apache/spark/pull/34096#issuecomment-926507530
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #34096: [SPARK-36841][SQL] Add ansi syntax `set catalog xxx` to change the current catalog
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #34096:
URL: https://github.com/apache/spark/pull/34096#issuecomment-934172694
> Could I continue to update after holiday?
Yes thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org