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 2022/09/14 14:48:43 UTC
[GitHub] [spark] cloud-fan opened a new pull request, #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
cloud-fan opened a new pull request, #37879:
URL: https://github.com/apache/spark/pull/37879
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
This PR updates `DropTable`/`DropView` to use `UnresolvedIdentifier` instead of `UnresolvedTableOrView`/`UnresolvedView`. This has several benefits:
1. Simplify the `ifExits` handling. No need to handle `DropTable` in `ResolveCommandsWithIfExists` anymore.
2. Avoid one table lookup if we eventually fallback to v1 command (v1 `DropTableCommand` will look up table again)
3. v2 catalogs can avoid table lookup entirely if possible.
This PR also improves table uncaching to match by table name directly, so that we don't need to look up the table and resolve to table relations.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
5. If you fix a bug, you can clarify why it is a bug.
-->
Save table lookup.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
No
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
existing tests
--
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 diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r972562054
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/CacheManager.scala:
##########
@@ -159,11 +159,51 @@ class CacheManager extends Logging with AdaptiveSparkPlanHelper {
plan: LogicalPlan,
cascade: Boolean,
blocking: Boolean = false): Unit = {
+ uncacheQuery(spark, _.sameResult(plan), cascade, blocking)
+ }
+
+ def uncacheTableOrView(spark: SparkSession, name: Seq[String], cascade: Boolean): Unit = {
+ uncacheQuery(
+ spark,
+ isMatchedTableOrView(_, name, spark.sessionState.conf),
+ cascade,
+ blocking = false)
+ }
+
+ private def isMatchedTableOrView(plan: LogicalPlan, name: Seq[String], conf: SQLConf): Boolean = {
+ def isSameName(nameInCache: Seq[String]): Boolean = {
+ nameInCache.length == name.length && nameInCache.zip(name).forall(conf.resolver.tupled)
+ }
+
+ plan match {
+ case SubqueryAlias(ident, LogicalRelation(_, _, Some(catalogTable), _)) =>
+ val v1Ident = catalogTable.identifier
+ isSameName(ident.qualifier :+ ident.name) &&
+ isSameName(v1Ident.catalog.toSeq ++ v1Ident.database :+ v1Ident.table)
+
+ case SubqueryAlias(ident, DataSourceV2Relation(_, _, Some(catalog), Some(v2Ident), _)) =>
+ isSameName(ident.qualifier :+ ident.name) &&
+ isSameName(catalog.name() +: v2Ident.namespace() :+ v2Ident.name())
Review Comment:
yes, see `ResolveRelations.createRelation`
--
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
Re: [PR] [SPARK-40425][SQL] DROP TABLE does not need to do table lookup [spark]
Posted by "gaecoli (via GitHub)" <gi...@apache.org>.
gaecoli commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r1722628022
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
Review Comment:
If you create a management table in spark 3.5.1, you cannot delete the path when dropping the table. I think this is a bug. It can be deleted correctly in spark 3.3.
--
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 #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
URL: https://github.com/apache/spark/pull/37879
--
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] aokolnychyi commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r1172865541
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
Review Comment:
I'll have time, probably, on Monday. I'll do that then unless someone gets there earlier.
--
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] aokolnychyi commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r1171761321
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
Review Comment:
The reason I asked that question is because the PR mentions that V2 tables may now skip table lookup. I am not sure how it is possible given the current `DropTableExec` implementation that calls `tableExists` before `drop`. The same is true for `DropTableCommand`. If I understand correctly, the goal of the PR is to avoid one extra lookup. Can we achieve that by referencing tables inside `DropTableExec`, instead of an identifier?Removing V1 DROP is also an option that would work. We may need to check `V2SessionCatalog`. It does an extra lookup in drop, which may be not needed.
I created SPARK-43203 for this.
--
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] aokolnychyi commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r1170544486
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
Review Comment:
Seems like @viirya is right and we need a call to `isV2Provider`.
--
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] viirya commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r972437078
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/CacheManager.scala:
##########
@@ -159,11 +159,51 @@ class CacheManager extends Logging with AdaptiveSparkPlanHelper {
plan: LogicalPlan,
cascade: Boolean,
blocking: Boolean = false): Unit = {
+ uncacheQuery(spark, _.sameResult(plan), cascade, blocking)
+ }
+
+ def uncacheTableOrView(spark: SparkSession, name: Seq[String], cascade: Boolean): Unit = {
+ uncacheQuery(
+ spark,
+ isMatchedTableOrView(_, name, spark.sessionState.conf),
+ cascade,
+ blocking = false)
+ }
+
+ private def isMatchedTableOrView(plan: LogicalPlan, name: Seq[String], conf: SQLConf): Boolean = {
+ def isSameName(nameInCache: Seq[String]): Boolean = {
+ nameInCache.length == name.length && nameInCache.zip(name).forall(conf.resolver.tupled)
+ }
+
+ plan match {
+ case SubqueryAlias(ident, LogicalRelation(_, _, Some(catalogTable), _)) =>
+ val v1Ident = catalogTable.identifier
+ isSameName(ident.qualifier :+ ident.name) &&
+ isSameName(v1Ident.catalog.toSeq ++ v1Ident.database :+ v1Ident.table)
+
+ case SubqueryAlias(ident, DataSourceV2Relation(_, _, Some(catalog), Some(v2Ident), _)) =>
+ isSameName(ident.qualifier :+ ident.name) &&
+ isSameName(catalog.name() +: v2Ident.namespace() :+ v2Ident.name())
+
+ case SubqueryAlias(ident, View(catalogTable, _, _)) =>
+ val v1Ident = catalogTable.identifier
+ isSameName(ident.qualifier :+ ident.name) &&
+ isSameName(v1Ident.catalog.toSeq ++ v1Ident.database :+ v1Ident.table)
+
+ case _ => false
+ }
+ }
+
+ def uncacheQuery(
+ spark: SparkSession,
+ isMatchedPlan: LogicalPlan => Boolean,
+ cascade: Boolean,
+ blocking: Boolean): Unit = {
val shouldRemove: LogicalPlan => Boolean =
if (cascade) {
- _.exists(_.sameResult(plan))
Review Comment:
`sameResult` doesn't work?
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/CacheManager.scala:
##########
@@ -159,11 +159,51 @@ class CacheManager extends Logging with AdaptiveSparkPlanHelper {
plan: LogicalPlan,
cascade: Boolean,
blocking: Boolean = false): Unit = {
+ uncacheQuery(spark, _.sameResult(plan), cascade, blocking)
+ }
+
+ def uncacheTableOrView(spark: SparkSession, name: Seq[String], cascade: Boolean): Unit = {
+ uncacheQuery(
+ spark,
+ isMatchedTableOrView(_, name, spark.sessionState.conf),
+ cascade,
+ blocking = false)
+ }
+
+ private def isMatchedTableOrView(plan: LogicalPlan, name: Seq[String], conf: SQLConf): Boolean = {
+ def isSameName(nameInCache: Seq[String]): Boolean = {
+ nameInCache.length == name.length && nameInCache.zip(name).forall(conf.resolver.tupled)
+ }
+
+ plan match {
+ case SubqueryAlias(ident, LogicalRelation(_, _, Some(catalogTable), _)) =>
+ val v1Ident = catalogTable.identifier
+ isSameName(ident.qualifier :+ ident.name) &&
+ isSameName(v1Ident.catalog.toSeq ++ v1Ident.database :+ v1Ident.table)
+
+ case SubqueryAlias(ident, DataSourceV2Relation(_, _, Some(catalog), Some(v2Ident), _)) =>
+ isSameName(ident.qualifier :+ ident.name) &&
+ isSameName(catalog.name() +: v2Ident.namespace() :+ v2Ident.name())
Review Comment:
Does SubqueryAlias have same name as the underlying relation?
--
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] viirya commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r972452739
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala:
##########
@@ -247,7 +247,28 @@ case class DropTableCommand(
} else if (ifExists) {
// no-op
} else {
- throw QueryCompilationErrors.tableOrViewNotFoundError(tableName.identifier)
+ throw QueryCompilationErrors.noSuchTableError(
+ tableName.catalog.toSeq ++ tableName.database :+ tableName.table)
+ }
+ Seq.empty[Row]
+ }
+}
+
+case class DropTempViewCommand(ident: Identifier) extends LeafRunnableCommand {
Review Comment:
v1 only, right?
--
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 diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r973878652
##########
core/src/test/scala/org/apache/spark/SparkFunSuite.scala:
##########
@@ -299,10 +299,15 @@ abstract class SparkFunSuite
parameters: Map[String, String] = Map.empty,
matchPVals: Boolean = false,
queryContext: Array[QueryContext] = Array.empty): Unit = {
- assert(exception.getErrorClass === errorClass)
+ val mainErrorClass :: tail = errorClass.split("\\.").toList
+ assert(tail.isEmpty || tail.length == 1)
+ // TODO: remove the `errorSubClass` parameter.
Review Comment:
I didn't create a JIRA for this TODO because @MaxGekk will fix it shortly (we talked offline) :)
--
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] ryan-johnson-databricks commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r1171695600
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
Review Comment:
> catalogs can override `tableExists` and that would be cheaper than `loadTable`?
Once you make an RPC, the latency hit is there. I would expect that exists vs. load on the backend would be a distant second.
--
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] viirya commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r1170527110
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
Review Comment:
I checked the difference between `ResolvedV1TableIdentifier` and `ResolvedV1Identifier`. So do you mean `ResolvedV1Identifier` could wrongly apply on a V2 table? I.e.,
> case ResolvedIdentifier(catalog, ident) if isSessionCatalog(catalog)
If `catalog` is a custom session catalog which is capable for V1 and V2 tables?
--
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] aokolnychyi commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r1171761321
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
Review Comment:
The reason I asked that question is because the PR mentions that V2 tables may now skip table lookup. I am not sure how it is possible given the current `DropTableExec` implementation that calls `tableExists` before `drop`. The same is true for `DropTableCommand`. If I understand correctly, the goal of the PR is to avoid one extra lookup. Can we achieve that by referencing tables inside `DropTableExec`, instead of an identifier? I guess removing V1 DROP is also an option that would work. We may need to check `V2SessionCatalog`. It does an extra lookup in drop, which may be not needed.
I created SPARK-43203 for this.
--
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 #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on PR #37879:
URL: https://github.com/apache/spark/pull/37879#issuecomment-1250738084
thanks for review, 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] cloud-fan commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r974211151
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala:
##########
@@ -685,15 +685,15 @@ class DDLParserSuite extends AnalysisTest {
val cmd = "DROP VIEW"
val hint = Some("Please use DROP TABLE instead.")
parseCompare(s"DROP VIEW testcat.db.view",
- DropView(UnresolvedView(Seq("testcat", "db", "view"), cmd, true, hint), ifExists = false))
Review Comment:
it's still used by commands like `SetViewProperties`
--
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] viirya commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r972574338
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/CacheManager.scala:
##########
@@ -159,11 +159,51 @@ class CacheManager extends Logging with AdaptiveSparkPlanHelper {
plan: LogicalPlan,
cascade: Boolean,
blocking: Boolean = false): Unit = {
+ uncacheQuery(spark, _.sameResult(plan), cascade, blocking)
+ }
+
+ def uncacheTableOrView(spark: SparkSession, name: Seq[String], cascade: Boolean): Unit = {
+ uncacheQuery(
+ spark,
+ isMatchedTableOrView(_, name, spark.sessionState.conf),
+ cascade,
+ blocking = false)
+ }
+
+ private def isMatchedTableOrView(plan: LogicalPlan, name: Seq[String], conf: SQLConf): Boolean = {
+ def isSameName(nameInCache: Seq[String]): Boolean = {
+ nameInCache.length == name.length && nameInCache.zip(name).forall(conf.resolver.tupled)
+ }
+
+ plan match {
+ case SubqueryAlias(ident, LogicalRelation(_, _, Some(catalogTable), _)) =>
+ val v1Ident = catalogTable.identifier
+ isSameName(ident.qualifier :+ ident.name) &&
+ isSameName(v1Ident.catalog.toSeq ++ v1Ident.database :+ v1Ident.table)
+
+ case SubqueryAlias(ident, DataSourceV2Relation(_, _, Some(catalog), Some(v2Ident), _)) =>
+ isSameName(ident.qualifier :+ ident.name) &&
+ isSameName(catalog.name() +: v2Ident.namespace() :+ v2Ident.name())
+
+ case SubqueryAlias(ident, View(catalogTable, _, _)) =>
+ val v1Ident = catalogTable.identifier
+ isSameName(ident.qualifier :+ ident.name) &&
+ isSameName(v1Ident.catalog.toSeq ++ v1Ident.database :+ v1Ident.table)
+
+ case _ => false
+ }
+ }
+
+ def uncacheQuery(
+ spark: SparkSession,
+ isMatchedPlan: LogicalPlan => Boolean,
+ cascade: Boolean,
+ blocking: Boolean): Unit = {
val shouldRemove: LogicalPlan => Boolean =
if (cascade) {
- _.exists(_.sameResult(plan))
Review Comment:
nvm
--
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] aokolnychyi commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r1170501180
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
Review Comment:
I am afraid this breaks the session catalog delegation. Previously, we checked the table was `V1Table`. Right now, we simply check the identifier looks like a V1 table identifier, which still may point to a valid V2 table. If I have a custom session catalog, it may be able to load both V1 and V2 tables. After this change, the V1 drop code is invoked for V2 tables in custom session catalogs. That means I can't drop tables correctly in custom session catalogs.
--
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] aokolnychyi commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r1170501180
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
Review Comment:
I am afraid this breaks the session catalog delegation. Previously, we checked the table was `V1Table`. Right now, we simply check the identifier looks like a V1 table identifier, which still may point to a valid V2 table. If I have a custom session catalog, it may be able to load both V1 and V2 tables. After this change, the V1 drop code is invoked for V2 tables in custom session catalogs.
--
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 diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r1170689458
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
Review Comment:
The change here is necessary as we don't look up the table for DROP TABLE anymore, so we can only "guess" it's a v1 or v2 table by the identifier.
I don't have a good solution here unless we add back the table lookup for DROP TABLE. Or shall we remove v1 DROP TABLE entirely? This is a very simple command and I think the v2 command covers it very 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] aokolnychyi commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r1171549193
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
Review Comment:
I would be fine either way. I understand the goal to avoid an extra table lookup. Is the idea here that catalogs can override `tableExists` and that would be cheaper than `loadTable`? The default `tableExists` in V2 catalogs still relies on loading tables.
--
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 diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r1172106159
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
Review Comment:
> Shall we switch to V2 DROP path for all cases to fix [SPARK-43203](https://issues.apache.org/jira/browse/SPARK-43203)?
Yea we should. Can you create a PR? 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
[GitHub] [spark] viirya commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r1172167784
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
Review Comment:
Sounds good to me to switch to V2 DROP.
--
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 diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r972562339
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala:
##########
@@ -247,7 +247,28 @@ case class DropTableCommand(
} else if (ifExists) {
// no-op
} else {
- throw QueryCompilationErrors.tableOrViewNotFoundError(tableName.identifier)
+ throw QueryCompilationErrors.noSuchTableError(
+ tableName.catalog.toSeq ++ tableName.database :+ tableName.table)
+ }
+ Seq.empty[Row]
+ }
+}
+
+case class DropTempViewCommand(ident: Identifier) extends LeafRunnableCommand {
Review Comment:
temp view is a Spark internal thing and is unrelated to data source, so it's neither v1 nor v2.
--
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] aokolnychyi commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r1170501996
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
Review Comment:
@cloud-fan @viirya @dongjoon-hyun, could you double check if I missed anything?
--
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] aokolnychyi commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r1170542563
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
Review Comment:
I think `ResolvedV1Identifier` simply means it is an identifier in the session catalog that has only db and table name. In custom session catalogs, it may point to a valid V2 table.
--
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] aokolnychyi commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r1172058529
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
Review Comment:
Sounds good to me. Shall we switch to V2 DROP path for all cases to fix [SPARK-43203](https://issues.apache.org/jira/browse/SPARK-43203)?
--
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 diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r974211738
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala:
##########
@@ -28,8 +28,14 @@ class ResolveCatalogs(val catalogManager: CatalogManager)
extends Rule[LogicalPlan] with LookupCatalog {
override def apply(plan: LogicalPlan): LogicalPlan = plan resolveOperators {
- case UnresolvedIdentifier(CatalogAndIdentifier(catalog, identifier)) =>
- ResolvedIdentifier(catalog, identifier)
+ case UnresolvedIdentifier(nameParts, allowTemp) =>
+ if (allowTemp && catalogManager.v1SessionCatalog.isTempView(nameParts)) {
+ val ident = Identifier.of(nameParts.dropRight(1).toArray, nameParts.last)
Review Comment:
good idea!
--
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 diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r972607760
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala:
##########
@@ -247,7 +247,28 @@ case class DropTableCommand(
} else if (ifExists) {
// no-op
} else {
- throw QueryCompilationErrors.tableOrViewNotFoundError(tableName.identifier)
+ throw QueryCompilationErrors.noSuchTableError(
Review Comment:
good point, we can simplify v1 DROP TABLE command now
--
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] dongjoon-hyun commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r973544556
##########
core/src/test/scala/org/apache/spark/SparkFunSuite.scala:
##########
@@ -299,10 +299,15 @@ abstract class SparkFunSuite
parameters: Map[String, String] = Map.empty,
matchPVals: Boolean = false,
queryContext: Array[QueryContext] = Array.empty): Unit = {
- assert(exception.getErrorClass === errorClass)
+ val mainErrorClass :: tail = errorClass.split("\\.").toList
+ assert(tail.isEmpty || tail.length == 1)
+ // TODO: remove the `errorSubClass` parameter.
Review Comment:
just nit. If we use IDed TODO, some contributor can pick up the item more easily.
##########
core/src/test/scala/org/apache/spark/SparkFunSuite.scala:
##########
@@ -299,10 +299,15 @@ abstract class SparkFunSuite
parameters: Map[String, String] = Map.empty,
matchPVals: Boolean = false,
queryContext: Array[QueryContext] = Array.empty): Unit = {
- assert(exception.getErrorClass === errorClass)
+ val mainErrorClass :: tail = errorClass.split("\\.").toList
+ assert(tail.isEmpty || tail.length == 1)
+ // TODO: remove the `errorSubClass` parameter.
Review Comment:
just nit. If we use IDed TODO with JIRA id, some contributor can pick up the item more easily.
--
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] viirya commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r972453705
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/v2ResolutionPlans.scala:
##########
@@ -244,3 +246,9 @@ case class ResolvedIdentifier(
identifier: Identifier) extends LeafNodeWithoutStats {
override def output: Seq[Attribute] = Nil
}
+
+// A fake v2 catalog to hold temp views.
+object FakeSystemCatalog extends CatalogPlugin {
+ override def initialize(name: String, options: CaseInsensitiveStringMap): Unit = {}
+ override def name(): String = "SYSTEM"
Review Comment:
FAKE_SYSTEM?
--
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] viirya commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r972576281
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala:
##########
@@ -247,7 +247,28 @@ case class DropTableCommand(
} else if (ifExists) {
// no-op
} else {
- throw QueryCompilationErrors.tableOrViewNotFoundError(tableName.identifier)
+ throw QueryCompilationErrors.noSuchTableError(
Review Comment:
`DropTableCommand` now won't be used to drop temp view, right? If so, there is some logic around ` val isTempView = catalog.isTempView(tableName)`, do we need update 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] viirya commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r972575165
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala:
##########
@@ -247,7 +247,28 @@ case class DropTableCommand(
} else if (ifExists) {
// no-op
} else {
- throw QueryCompilationErrors.tableOrViewNotFoundError(tableName.identifier)
+ throw QueryCompilationErrors.noSuchTableError(
+ tableName.catalog.toSeq ++ tableName.database :+ tableName.table)
+ }
+ Seq.empty[Row]
+ }
+}
+
+case class DropTempViewCommand(ident: Identifier) extends LeafRunnableCommand {
Review Comment:
Oh, I see. I read the comment "v1 DROP TABLE supports temp view." wrongly. There is also other pattern statement for v2 going to `DropTempViewCommand`.
--
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] ryan-johnson-databricks commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by GitBox <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r974198718
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveCatalogs.scala:
##########
@@ -28,8 +28,14 @@ class ResolveCatalogs(val catalogManager: CatalogManager)
extends Rule[LogicalPlan] with LookupCatalog {
override def apply(plan: LogicalPlan): LogicalPlan = plan resolveOperators {
- case UnresolvedIdentifier(CatalogAndIdentifier(catalog, identifier)) =>
- ResolvedIdentifier(catalog, identifier)
+ case UnresolvedIdentifier(nameParts, allowTemp) =>
+ if (allowTemp && catalogManager.v1SessionCatalog.isTempView(nameParts)) {
+ val ident = Identifier.of(nameParts.dropRight(1).toArray, nameParts.last)
Review Comment:
nit: `nameParts.init` is the counterpart to `nameParts.last`:
https://www.scala-lang.org/api/2.12.5/scala/collection/Seq.html#inits:Iterator[Repr]
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala:
##########
@@ -685,15 +685,15 @@ class DDLParserSuite extends AnalysisTest {
val cmd = "DROP VIEW"
val hint = Some("Please use DROP TABLE instead.")
parseCompare(s"DROP VIEW testcat.db.view",
- DropView(UnresolvedView(Seq("testcat", "db", "view"), cmd, true, hint), ifExists = false))
Review Comment:
why does `UnresolvedView` even continue to exist, if it's not useful for dropping? Do we still use it for add/select/etc?
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
DropTableCommand(ident, ifExists, isView = false, purge = purge)
- case DropTable(_: ResolvedPersistentView, ifExists, purge) =>
- throw QueryCompilationErrors.cannotDropViewWithDropTableError
-
// v1 DROP TABLE supports temp view.
- case DropTable(ResolvedTempView(ident, _), ifExists, purge) =>
- DropTableCommand(ident.asTableIdentifier, ifExists, isView = false, purge = purge)
+ case DropTable(ResolvedIdentifier(FakeSystemCatalog, ident), _, _) =>
+ DropTempViewCommand(ident)
- case DropView(ResolvedViewIdentifier(ident), ifExists) =>
+ case DropView(ResolvedV1Identifier(ident), ifExists) =>
DropTableCommand(ident, ifExists, isView = true, purge = false)
+ case DropView(r @ ResolvedIdentifier(catalog, ident), _) =>
+ if (catalog == FakeSystemCatalog) {
+ DropTempViewCommand(ident)
+ } else {
+ throw QueryCompilationErrors.catalogOperationNotSupported(catalog, "views")
+ }
Review Comment:
nit: `r` not used? And if you make `FakeSystemCatalog` a `case object` it can participate directly in matching here:
```scala
case DropView(ResolvedIdentifier(FakeSystemCatalog, ident), _) =>
DropTempViewCommand(ident)
case DropView(ResolvedIdentifier(catalog, _), _) =>
throw ...
```
--
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] Hisoka-X commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by "Hisoka-X (via GitHub)" <gi...@apache.org>.
Hisoka-X commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r1206776117
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
Review Comment:
@aokolnychyi Any update for this? If you don't mind I can finish it this weekend.😄
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
Review Comment:
Hi @aokolnychyi Any update for this? If you don't mind I can finish it this weekend.😄
--
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 diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r1171982232
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
Review Comment:
`exists` should be slightly faster than `load` as the RPC message is smaller. I think we can further optimize out `exists` and send one RPC to drop the table. If the table doesn't exist, false will be returned and we can deal with it in the client (Spark) side to implement the `if exists` semantic.
This PR also makes the third-party access control service clearer: ideally people don't need SELECT permission to drop a table.
--
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] aokolnychyi commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r1170542563
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
Review Comment:
I think `ResolvedV1Identifier` simply means it is an identifier in the session catalog that has only db and table name (in other words it is a valid V1 identifier). In custom session catalogs, it may point to a valid V2 table.
--
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 diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r972560446
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/v2ResolutionPlans.scala:
##########
@@ -244,3 +246,9 @@ case class ResolvedIdentifier(
identifier: Identifier) extends LeafNodeWithoutStats {
override def output: Seq[Attribute] = Nil
}
+
+// A fake v2 catalog to hold temp views.
+object FakeSystemCatalog extends CatalogPlugin {
+ override def initialize(name: String, options: CaseInsensitiveStringMap): Unit = {}
+ override def name(): String = "SYSTEM"
Review Comment:
the name doesn't matter. We won't show it or look it up for now. But I think it's a good idea to add a system catalog officially, to host temp view, temp functions and builtin functions.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/v2ResolutionPlans.scala:
##########
@@ -244,3 +246,9 @@ case class ResolvedIdentifier(
identifier: Identifier) extends LeafNodeWithoutStats {
override def output: Seq[Attribute] = Nil
}
+
+// A fake v2 catalog to hold temp views.
+object FakeSystemCatalog extends CatalogPlugin {
+ override def initialize(name: String, options: CaseInsensitiveStringMap): Unit = {}
+ override def name(): String = "SYSTEM"
Review Comment:
the name doesn't matter. We won't show it or look it up for now. But later I think it's a good idea to add a system catalog officially, to host temp view, temp functions and builtin functions.
--
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 #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on PR #37879:
URL: https://github.com/apache/spark/pull/37879#issuecomment-1247801449
cc @MaxGekk @viirya
--
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] viirya commented on pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by GitBox <gi...@apache.org>.
viirya commented on PR #37879:
URL: https://github.com/apache/spark/pull/37879#issuecomment-1249565075
One pyspark error, although looks like a real failure, seems unrelated?
```
Traceback (most recent call last):
File "/__w/spark/spark/python/pyspark/pandas/tests/test_spark_functions.py", line 28, in test_repeat
self.assertTrue(spark_column_equals(SF.repeat(F.lit(1), 2), F.repeat(F.lit(1), 2)))
AssertionError: False is not true
```
--
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] viirya commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r1170529644
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
Review Comment:
I saw for many commands, there is a `isV2Provider` check, but `DropTable` doesn't. So seems we need 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] aokolnychyi commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r1172059420
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
Review Comment:
> I think we can further optimize out exists and send one RPC to drop the table
+1 as the public catalog API already requires catalogs to return false for `dropTable` if the table does not exist
--
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] aokolnychyi commented on a diff in pull request #37879: [SPARK-40425][SQL] DROP TABLE does not need to do table lookup
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #37879:
URL: https://github.com/apache/spark/pull/37879#discussion_r1172865541
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala:
##########
@@ -216,19 +216,23 @@ class ResolveSessionCatalog(val catalogManager: CatalogManager)
c
}
- case DropTable(ResolvedV1TableIdentifier(ident), ifExists, purge) =>
+ case DropTable(ResolvedV1Identifier(ident), ifExists, purge) =>
Review Comment:
I'll have time, probably, on Monday.
--
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