[GitHub] [spark] imback82 commented on a change in pull request #30229: [SPARK-33321][SQL] Migrate ANALYZE TABLE commands to use UnresolvedTable/UnresolvedTableOrView to resolve the identifier

[GitBox <gi...@apache.org>]

imback82 commented on a change in pull request #30229:
URL: https://github.com/apache/spark/pull/30229#discussion_r516433011



##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala
##########
@@ -175,7 +175,10 @@ abstract class SQLViewSuite extends QueryTest with SQLTestUtils {
       }.getMessage
       assert(e2.contains("SHOW CREATE TABLE is not supported on a temporary view"))
       assertNoSuchTable(s"SHOW PARTITIONS $viewName")
-      assertNoSuchTable(s"ANALYZE TABLE $viewName COMPUTE STATISTICS")
+      val e3 = intercept[AnalysisException] {
+        sql(s"ANALYZE TABLE $viewName COMPUTE STATISTICS")
+      }.getMessage
+      assert(e3.contains("ANALYZE TABLE is not supported on a temporary view"))
       assertNoSuchTable(s"ANALYZE TABLE $viewName COMPUTE STATISTICS FOR COLUMNS id")

Review comment:
       Weirdly, `AnalyzeColumnCommand` throws `NoSuchTableException` if the temp view is not cached (even if it exists):
   https://github.com/apache/spark/blob/3959f0d9879fa7fa9e8f2e8ed8c8b12003d21788/sql/core/src/main/scala/org/apache/spark/sql/execution/command/AnalyzeColumnCommand.scala#L72-L78
   So, this line doesn't need to be updated.

##########
File path: sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala
##########
@@ -255,19 +254,11 @@ class ResolveSessionCatalog(
     case RenameTableStatement(TempViewOrV1Table(oldName), newName, isView) =>
       AlterTableRenameCommand(oldName.asTableIdentifier, newName.asTableIdentifier, isView)
 
-    case DescribeRelation(r @ ResolvedTable(_, ident, _: V1Table), partitionSpec, isExtended)
-        if isSessionCatalog(r.catalog) =>
-      DescribeTableCommand(ident.asTableIdentifier, partitionSpec, isExtended)
-
     // Use v1 command to describe (temp) view, as v2 catalog doesn't support view yet.
-    case DescribeRelation(ResolvedView(ident, _), partitionSpec, isExtended) =>
+    case DescribeRelation(ResolvedV1TableOrViewIdentifier(ident), partitionSpec, isExtended) =>
       DescribeTableCommand(ident.asTableIdentifier, partitionSpec, isExtended)
 
-    case DescribeColumn(r @ ResolvedTable(_, _, _: V1Table), colNameParts, isExtended)
-        if isSessionCatalog(r.catalog) =>
-      DescribeColumnCommand(r.identifier.asTableIdentifier, colNameParts, isExtended)
-
-    case DescribeColumn(ResolvedView(ident, _), colNameParts, isExtended) =>
+    case DescribeColumn(ResolvedV1TableOrViewIdentifier(ident), colNameParts, isExtended) =>
       DescribeColumnCommand(ident.asTableIdentifier, colNameParts, isExtended)

Review comment:
       Applying `ResolvedV1TableOrViewIdentifier` extractor to existing code to simplify.

##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/command/AnalyzePartitionCommand.scala
##########
@@ -75,6 +75,9 @@ case class AnalyzePartitionCommand(
 
   override def run(sparkSession: SparkSession): Seq[Row] = {
     val sessionState = sparkSession.sessionState
+    if (sessionState.catalog.getTempView(tableIdent.identifier).isDefined) {
+      throw new AnalysisException("ANALYZE TABLE is not supported on a temporary view.")
+    }

Review comment:
       `ANALYZE TABLE` supports both table and permanent view (if cached), but not a temporary view. Thus, we need to check this in commands to make sure temporary views are resolved first. Another approach is to introduce `UnresolvedTableOrPermanentView` such that if a temporary view is found first, it fails at the `Analyzer`. WDYT @cloud-fan?




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org