[GitHub] [spark] cloud-fan commented on a change in pull request #27550: [SPARK-30799][SQL] "spark_catalog.t" should not be resolved to temp view
cloud-fan commented on a change in pull request #27550: [SPARK-30799][SQL]
"spark_catalog.t" should not be resolved to temp view
URL: https://github.com/apache/spark/pull/27550#discussion_r381082389
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/LookupCatalog.scala
##
@@ -94,6 +94,10 @@ private[sql] trait LookupCatalog extends Logging {
* Extract catalog and identifier from a multi-part name with the current
catalog if needed.
* Catalog name takes precedence over identifier, but for a single-part
name, identifier takes
* precedence over catalog name.
+ *
+ * Note that, this pattern is used to look up permanent catalog objects like
table, view,
+ * function, etc. If you need to look up temp objects like temp view, please
do it separately
+ * before calling this pattern, as temp objects don't belong to any catalog.
Review comment:
> our internal SessionCatalog also manages temp objects
We can move them out and put it in a temp view manager like the
`GlobalTempViewManager`. I'm talking more about the theory.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #27550: [SPARK-30799][SQL] "spark_catalog.t" should not be resolved to temp view
cloud-fan commented on a change in pull request #27550: [SPARK-30799][SQL]
"spark_catalog.t" should not be resolved to temp view
URL: https://github.com/apache/spark/pull/27550#discussion_r379233286
##
File path:
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala
##
@@ -584,7 +632,29 @@ class ResolveSessionCatalog(
object SessionCatalogAndTable {
def unapply(nameParts: Seq[String]): Option[(CatalogPlugin, Seq[String])]
= nameParts match {
case SessionCatalogAndIdentifier(catalog, ident) =>
-Some(catalog -> ident.asMultipartIdentifier)
+if (nameParts.length == 1) {
+ // If there is only one name part, it means the current catalog is
the session catalog.
+ // Here we return the original name part, to keep the error message
unchanged for
+ // v1 commands.
Review comment:
We can remove
https://github.com/apache/spark/pull/27550/files#diff-2e07be4d73605cb1941153441a0c0c14R572
and
https://github.com/apache/spark/pull/27550/files#diff-2e07be4d73605cb1941153441a0c0c14R651
if we return qualified name 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #27550: [SPARK-30799][SQL] "spark_catalog.t" should not be resolved to temp view
cloud-fan commented on a change in pull request #27550: [SPARK-30799][SQL]
"spark_catalog.t" should not be resolved to temp view
URL: https://github.com/apache/spark/pull/27550#discussion_r379371879
##
File path:
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala
##
@@ -496,56 +510,94 @@ class ResolveSessionCatalog(
case ShowTableProperties(r: ResolvedTable, propertyKey) if
isSessionCatalog(r.catalog) =>
ShowTablePropertiesCommand(r.identifier.asTableIdentifier, propertyKey)
-case DescribeFunctionStatement(CatalogAndIdentifier(catalog, ident),
extended) =>
+case DescribeFunctionStatement(nameParts, extended) =>
val functionIdent =
-parseSessionCatalogFunctionIdentifier("DESCRIBE FUNCTION", catalog,
ident)
+parseSessionCatalogFunctionIdentifier(nameParts, "DESCRIBE FUNCTION")
DescribeFunctionCommand(functionIdent, extended)
case ShowFunctionsStatement(userScope, systemScope, pattern, fun) =>
val (database, function) = fun match {
-case Some(CatalogAndIdentifier(catalog, ident)) =>
+case Some(nameParts) =>
val FunctionIdentifier(fn, db) =
-parseSessionCatalogFunctionIdentifier("SHOW FUNCTIONS", catalog,
ident)
+parseSessionCatalogFunctionIdentifier(nameParts, "SHOW FUNCTIONS")
(db, Some(fn))
case None => (None, pattern)
}
ShowFunctionsCommand(database, function, userScope, systemScope)
-case DropFunctionStatement(CatalogAndIdentifier(catalog, ident), ifExists,
isTemp) =>
+case DropFunctionStatement(nameParts, ifExists, isTemp) =>
val FunctionIdentifier(function, database) =
-parseSessionCatalogFunctionIdentifier("DROP FUNCTION", catalog, ident)
+parseSessionCatalogFunctionIdentifier(nameParts, "DROP FUNCTION")
DropFunctionCommand(database, function, ifExists, isTemp)
-case CreateFunctionStatement(CatalogAndIdentifier(catalog, ident),
+case CreateFunctionStatement(nameParts,
className, resources, isTemp, ignoreIfExists, replace) =>
- val FunctionIdentifier(function, database) =
-parseSessionCatalogFunctionIdentifier("CREATE FUNCTION", catalog,
ident)
- CreateFunctionCommand(database, function, className, resources, isTemp,
ignoreIfExists,
-replace)
+ if (isTemp) {
+// temp func doesn't belong to any catalog and we shouldn't resolve
catalog in the name.
+val database = if (nameParts.length > 2) {
+ throw new AnalysisException(s"Unsupported function name
'${nameParts.quoted}'")
Review comment:
This is consistent with
https://github.com/apache/spark/pull/27550/files#diff-2e07be4d73605cb1941153441a0c0c14L536
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #27550: [SPARK-30799][SQL] "spark_catalog.t" should not be resolved to temp view
cloud-fan commented on a change in pull request #27550: [SPARK-30799][SQL]
"spark_catalog.t" should not be resolved to temp view
URL: https://github.com/apache/spark/pull/27550#discussion_r379245778
##
File path:
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala
##
@@ -584,7 +632,29 @@ class ResolveSessionCatalog(
object SessionCatalogAndTable {
def unapply(nameParts: Seq[String]): Option[(CatalogPlugin, Seq[String])]
= nameParts match {
case SessionCatalogAndIdentifier(catalog, ident) =>
-Some(catalog -> ident.asMultipartIdentifier)
+if (nameParts.length == 1) {
+ // If there is only one name part, it means the current catalog is
the session catalog.
+ // Here we return the original name part, to keep the error message
unchanged for
+ // v1 commands.
+ Some(catalog -> nameParts)
+} else {
+ Some(catalog -> ident.asMultipartIdentifier)
+}
+ case _ => None
+}
+ }
+
+ object TempViewOrV1Table {
+def unapply(nameParts: Seq[String]): Option[Seq[String]] = nameParts match
{
+ case _ if isTempView(nameParts) => Some(nameParts)
+ case SessionCatalogAndTable(_, tbl) =>
+if (nameParts.head == CatalogManager.SESSION_CATALOG_NAME &&
tbl.length == 1) {
+ // For name parts like `spark_catalog.t`, we need to fill in the
default database so
Review comment:
I noticed this well and think this is not intentional. But this is a
different topic and we have many tests using `spark_catalog.t`.
We can open another PR to forbid it if we think we shouldn't support this
feature.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #27550: [SPARK-30799][SQL] "spark_catalog.t" should not be resolved to temp view
cloud-fan commented on a change in pull request #27550: [SPARK-30799][SQL]
"spark_catalog.t" should not be resolved to temp view
URL: https://github.com/apache/spark/pull/27550#discussion_r379245392
##
File path:
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala
##
@@ -228,10 +230,12 @@ class ResolveSessionCatalog(
case DescribeColumnStatement(
SessionCatalogAndTable(catalog, tbl), colNameParts, isExtended) =>
loadTable(catalog, tbl.asIdentifier).collect {
+// `V1Table` also includes permanent views.
case v1Table: V1Table =>
DescribeColumnCommand(tbl.asTableIdentifier, colNameParts,
isExtended)
}.getOrElse {
-if (isView(tbl)) {
+if (isTempView(tbl)) {
Review comment:
You have a good point here. I think this is an existing bug. Here we look up
table first (call `loadTable`) then look up temp view. We should look up temp
view first to retain the behavior of Spark 2.4. @imback82 can you help fix it
later?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #27550: [SPARK-30799][SQL] "spark_catalog.t" should not be resolved to temp view
cloud-fan commented on a change in pull request #27550: [SPARK-30799][SQL]
"spark_catalog.t" should not be resolved to temp view
URL: https://github.com/apache/spark/pull/27550#discussion_r379233286
##
File path:
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala
##
@@ -584,7 +632,29 @@ class ResolveSessionCatalog(
object SessionCatalogAndTable {
def unapply(nameParts: Seq[String]): Option[(CatalogPlugin, Seq[String])]
= nameParts match {
case SessionCatalogAndIdentifier(catalog, ident) =>
-Some(catalog -> ident.asMultipartIdentifier)
+if (nameParts.length == 1) {
+ // If there is only one name part, it means the current catalog is
the session catalog.
+ // Here we return the original name part, to keep the error message
unchanged for
+ // v1 commands.
Review comment:
We can remove
https://github.com/apache/spark/pull/27550/files#diff-2e07be4d73605cb1941153441a0c0c14R572
and
https://github.com/apache/spark/pull/27550/files#diff-2e07be4d73605cb1941153441a0c0c14R651
if we return qualified name 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #27550: [SPARK-30799][SQL] "spark_catalog.t" should not be resolved to temp view
cloud-fan commented on a change in pull request #27550: [SPARK-30799][SQL]
"spark_catalog.t" should not be resolved to temp view
URL: https://github.com/apache/spark/pull/27550#discussion_r379232848
##
File path:
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala
##
@@ -584,7 +632,29 @@ class ResolveSessionCatalog(
object SessionCatalogAndTable {
def unapply(nameParts: Seq[String]): Option[(CatalogPlugin, Seq[String])]
= nameParts match {
case SessionCatalogAndIdentifier(catalog, ident) =>
-Some(catalog -> ident.asMultipartIdentifier)
+if (nameParts.length == 1) {
+ // If there is only one name part, it means the current catalog is
the session catalog.
+ // Here we return the original name part, to keep the error message
unchanged for
+ // v1 commands.
Review comment:
I can remove this, but then I need to update many tests slightly to update
the error message. We can leave it to 3.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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
