From 33c1034315af126b3fbcaab385a9c5e8561c1709 Mon Sep 17 00:00:00 2001 From: Terry Kim Date: Fri, 7 May 2021 06:07:53 +0000 Subject: [PATCH] [SPARK-34701][SQL][FOLLOW-UP] Children/innerChildren should be mutually exclusive for AnalysisOnlyCommand ### What changes were proposed in this pull request? This is a follow up to https://github.com/apache/spark/pull/32032#discussion_r620928086. Basically, `children`/`innerChildren` should be mutually exclusive for `AlterViewAsCommand` and `CreateViewCommand`, which extend `AnalysisOnlyCommand`. Otherwise, there could be an issue in the `EXPLAIN` command. Currently, this is not an issue, because these commands will be analyzed (children will always be empty) when the `EXPLAIN` command is run. ### Why are the changes needed? To be future-proof where these commands are directly used. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Added new tsts Closes #32447 from imback82/SPARK-34701-followup. Authored-by: Terry Kim Signed-off-by: Wenchen Fan --- .../sql/catalyst/plans/logical/Command.scala | 2 ++ .../spark/sql/execution/command/views.scala | 5 ----- .../command/PlanResolutionSuite.scala | 21 ++++++++++++++++++- 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/Command.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/Command.scala index 81ad92b3f9ecb..5af1e9c1bb8ea 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/Command.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/Command.scala @@ -18,6 +18,7 @@ package org.apache.spark.sql.catalyst.plans.logical import org.apache.spark.sql.catalyst.expressions.{Attribute, AttributeSet} +import org.apache.spark.sql.catalyst.plans.QueryPlan import org.apache.spark.sql.catalyst.trees.{BinaryLike, LeafLike, UnaryLike} /** @@ -46,5 +47,6 @@ trait AnalysisOnlyCommand extends Command { val isAnalyzed: Boolean def childrenToAnalyze: Seq[LogicalPlan] override final def children: Seq[LogicalPlan] = if (isAnalyzed) Nil else childrenToAnalyze + override def innerChildren: Seq[QueryPlan[_]] = if (isAnalyzed) childrenToAnalyze else Nil def markAsAnalyzed(): LogicalPlan } diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala index 10ec4be7d9df3..dd164d48e5e7b 100644 --- a/sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala +++ b/sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala @@ -28,7 +28,6 @@ import org.apache.spark.sql.catalyst.{FunctionIdentifier, SQLConfHelper, TableId import org.apache.spark.sql.catalyst.analysis.{GlobalTempView, LocalTempView, PersistedView, ViewType} import org.apache.spark.sql.catalyst.catalog.{CatalogStorageFormat, CatalogTable, CatalogTableType, SessionCatalog, TemporaryViewRelation} import org.apache.spark.sql.catalyst.expressions.{Alias, Attribute, SubqueryExpression, UserDefinedExpression} -import org.apache.spark.sql.catalyst.plans.QueryPlan import org.apache.spark.sql.catalyst.plans.logical.{AnalysisOnlyCommand, LogicalPlan, Project, View} import org.apache.spark.sql.catalyst.util.CharVarcharUtils import org.apache.spark.sql.connector.catalog.CatalogV2Implicits.NamespaceHelper @@ -77,8 +76,6 @@ case class CreateViewCommand( copy(plan = newChildren.head) } - override def innerChildren: Seq[QueryPlan[_]] = Seq(plan) - // `plan` needs to be analyzed, but shouldn't be optimized so that caching works correctly. override def childrenToAnalyze: Seq[LogicalPlan] = plan :: Nil @@ -256,8 +253,6 @@ case class AlterViewAsCommand( copy(query = newChildren.head) } - override def innerChildren: Seq[QueryPlan[_]] = Seq(query) - override def childrenToAnalyze: Seq[LogicalPlan] = query :: Nil def markAsAnalyzed(): LogicalPlan = copy(isAnalyzed = true) diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/PlanResolutionSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/PlanResolutionSuite.scala index e1e37f7652bbc..a87f977eb1602 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/execution/command/PlanResolutionSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/command/PlanResolutionSuite.scala @@ -30,7 +30,7 @@ import org.apache.spark.sql.catalyst.analysis.{AnalysisTest, Analyzer, EmptyFunc import org.apache.spark.sql.catalyst.catalog.{BucketSpec, CatalogStorageFormat, CatalogTable, CatalogTableType, InMemoryCatalog, SessionCatalog} import org.apache.spark.sql.catalyst.expressions.{AttributeReference, EqualTo, Expression, InSubquery, IntegerLiteral, ListQuery, Literal, StringLiteral} import org.apache.spark.sql.catalyst.parser.{CatalystSqlParser, ParseException} -import org.apache.spark.sql.catalyst.plans.logical.{AlterTable, AppendData, Assignment, CreateTableAsSelect, CreateTableStatement, CreateV2Table, DeleteAction, DeleteFromTable, DescribeRelation, DropTable, InsertAction, LocalRelation, LogicalPlan, MergeIntoTable, OneRowRelation, Project, SetTableLocation, SetTableProperties, ShowTableProperties, SubqueryAlias, UnsetTableProperties, UpdateAction, UpdateTable} +import org.apache.spark.sql.catalyst.plans.logical.{AlterTable, AnalysisOnlyCommand, AppendData, Assignment, CreateTableAsSelect, CreateTableStatement, CreateV2Table, DeleteAction, DeleteFromTable, DescribeRelation, DropTable, InsertAction, LocalRelation, LogicalPlan, MergeIntoTable, OneRowRelation, Project, SetTableLocation, SetTableProperties, ShowTableProperties, SubqueryAlias, UnsetTableProperties, UpdateAction, UpdateTable} import org.apache.spark.sql.catalyst.rules.Rule import org.apache.spark.sql.connector.FakeV2Provider import org.apache.spark.sql.connector.catalog.{CatalogManager, CatalogNotFoundException, Identifier, Table, TableCapability, TableCatalog, TableChange, V1Table} @@ -2192,6 +2192,15 @@ class PlanResolutionSuite extends AnalysisTest { assert(desc.comment == Some("no comment")) } + test("SPARK-34701: children/innerChildren should be mutually exclusive for AnalysisOnlyCommand") { + val cmdNotAnalyzed = DummyAnalysisOnlyCommand(isAnalyzed = false, childrenToAnalyze = Seq(null)) + assert(cmdNotAnalyzed.innerChildren.isEmpty) + assert(cmdNotAnalyzed.children.length == 1) + val cmdAnalyzed = cmdNotAnalyzed.markAsAnalyzed() + assert(cmdAnalyzed.innerChildren.length == 1) + assert(cmdAnalyzed.children.isEmpty) + } + // TODO: add tests for more commands. } @@ -2201,3 +2210,13 @@ object AsDataSourceV2Relation { case _ => None } } + +case class DummyAnalysisOnlyCommand( + isAnalyzed: Boolean, + childrenToAnalyze: Seq[LogicalPlan]) extends AnalysisOnlyCommand { + override def markAsAnalyzed(): LogicalPlan = copy(isAnalyzed = true) + override protected def withNewChildrenInternal( + newChildren: IndexedSeq[LogicalPlan]): LogicalPlan = { + copy(childrenToAnalyze = newChildren) + } +}