-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-20460][SQL] Make it more consistent to handle column name duplication #17758
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
1e647ee
Share code to check column name duplication
maropu 4467077
Apply reviews
maropu 33ab217
Make code more consistent
maropu d8efb9d
Apply review comments
maropu 11d1818
Apply xiao's reviews
maropu 22e1e4f
Apply more xiao's reviews
maropu 743a069
Replace map with foreach
maropu f6eab2d
Add tests for data schema + parititon schema
maropu 09da8d6
Drop name dplication checks in HiveMetastoreCatalog.scala
maropu 6d03f31
Modify exception messages
maropu a0b9b05
Revert logic to check name duplication
maropu 91b6424
Add tests for write paths
maropu 37ad3f3
Add tests for stream sink paths
maropu d0d9d3e
Burhs up code and adds more tests
maropu cbe9c71
Apply reviews
maropu c69270f
Apply more comments
maropu af959f6
Add more tests in create.sql
maropu 8d3e10a
Move duplication checks in constructor
maropu 9b386d5
Brush up code
maropu a878510
[WIP] Add DataSourceValidator trait to validate schema in write path
maropu be20127
Revert "Brush up code"
maropu f41bf80
Fix more issues
maropu 0526391
Revert DataSourceValidator
maropu 9e199bc
Add the check for external relation providers
maropu 1ae132d
[WIP] Handle DataSource name duplication in one place
maropu 5c29a75
Fix more
maropu 5ed2c0d
Move some tests to DDLSuite
maropu File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Brush up code
- Loading branch information
commit 9b386d56fd2ee12278bc2cfc4166a4eef0b9919c
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,13 +21,11 @@ import scala.collection.mutable | |
|
|
||
| import org.apache.spark.sql.{AnalysisException, Row, SparkSession} | ||
| import org.apache.spark.sql.catalyst.TableIdentifier | ||
| import org.apache.spark.sql.catalyst.analysis.{UnresolvedFunction, UnresolvedRelation} | ||
| import org.apache.spark.sql.catalyst.catalog.{CatalogStorageFormat, CatalogTable, CatalogTableType} | ||
| import org.apache.spark.sql.catalyst.expressions.{Alias, SubqueryExpression} | ||
| import org.apache.spark.sql.catalyst.expressions.Alias | ||
| import org.apache.spark.sql.catalyst.plans.QueryPlan | ||
| import org.apache.spark.sql.catalyst.plans.logical.{LogicalPlan, Project, View} | ||
| import org.apache.spark.sql.catalyst.plans.logical.{LogicalPlan, Project} | ||
| import org.apache.spark.sql.types.MetadataBuilder | ||
| import org.apache.spark.sql.util.SchemaUtils | ||
|
|
||
|
|
||
| /** | ||
|
|
@@ -124,28 +122,19 @@ case class CreateViewCommand( | |
| } | ||
|
|
||
| override def run(sparkSession: SparkSession): Seq[Row] = { | ||
| // If the plan cannot be analyzed, throw an exception and don't proceed. | ||
| val qe = sparkSession.sessionState.executePlan(child) | ||
| qe.assertAnalyzed() | ||
| val analyzedPlan = qe.analyzed | ||
|
|
||
| if (userSpecifiedColumns.nonEmpty && | ||
| userSpecifiedColumns.length != analyzedPlan.output.length) { | ||
| userSpecifiedColumns.length != child.output.length) { | ||
| throw new AnalysisException(s"The number of columns produced by the SELECT clause " + | ||
| s"(num: `${analyzedPlan.output.length}`) does not match the number of column names " + | ||
| s"(num: `${child.output.length}`) does not match the number of column names " + | ||
| s"specified by CREATE VIEW (num: `${userSpecifiedColumns.length}`).") | ||
| } | ||
|
|
||
| // When creating a permanent view, not allowed to reference temporary objects. | ||
| // This should be called after `qe.assertAnalyzed()` (i.e., `child` can be resolved) | ||
| verifyTemporaryObjectsNotExists(sparkSession) | ||
|
|
||
| val catalog = sparkSession.sessionState.catalog | ||
| if (viewType == LocalTempView) { | ||
| val aliasedPlan = aliasPlan(sparkSession, analyzedPlan) | ||
| val aliasedPlan = aliasPlan(sparkSession, child) | ||
| catalog.createTempView(name.table, aliasedPlan, overrideIfExists = replace) | ||
| } else if (viewType == GlobalTempView) { | ||
| val aliasedPlan = aliasPlan(sparkSession, analyzedPlan) | ||
| val aliasedPlan = aliasPlan(sparkSession, child) | ||
| catalog.createGlobalTempView(name.table, aliasedPlan, overrideIfExists = replace) | ||
| } else if (catalog.tableExists(name)) { | ||
| val tableMetadata = catalog.getTableMetadata(name) | ||
|
|
@@ -155,14 +144,11 @@ case class CreateViewCommand( | |
| } else if (tableMetadata.tableType != CatalogTableType.VIEW) { | ||
| throw new AnalysisException(s"$name is not a view") | ||
| } else if (replace) { | ||
| // Detect cyclic view reference on CREATE OR REPLACE VIEW. | ||
| val viewIdent = tableMetadata.identifier | ||
| checkCyclicViewReference(analyzedPlan, Seq(viewIdent), viewIdent) | ||
|
||
|
|
||
| // Handles `CREATE OR REPLACE VIEW v0 AS SELECT ...` | ||
| // Nothing we need to retain from the old view, so just drop and create a new one | ||
| val viewIdent = tableMetadata.identifier | ||
| catalog.dropTable(viewIdent, ignoreIfNotExists = false, purge = false) | ||
| catalog.createTable(prepareTable(sparkSession, analyzedPlan), ignoreIfExists = false) | ||
| catalog.createTable(prepareTable(sparkSession, child), ignoreIfExists = false) | ||
| } else { | ||
| // Handles `CREATE VIEW v0 AS SELECT ...`. Throws exception when the target view already | ||
| // exists. | ||
|
|
@@ -172,39 +158,11 @@ case class CreateViewCommand( | |
| } | ||
| } else { | ||
| // Create the view if it doesn't exist. | ||
| catalog.createTable(prepareTable(sparkSession, analyzedPlan), ignoreIfExists = false) | ||
| catalog.createTable(prepareTable(sparkSession, child), ignoreIfExists = false) | ||
| } | ||
| Seq.empty[Row] | ||
| } | ||
|
|
||
| /** | ||
| * Permanent views are not allowed to reference temp objects, including temp function and views | ||
| */ | ||
| private def verifyTemporaryObjectsNotExists(sparkSession: SparkSession): Unit = { | ||
| if (!isTemporary) { | ||
| // This func traverses the unresolved plan `child`. Below are the reasons: | ||
| // 1) Analyzer replaces unresolved temporary views by a SubqueryAlias with the corresponding | ||
| // logical plan. After replacement, it is impossible to detect whether the SubqueryAlias is | ||
| // added/generated from a temporary view. | ||
| // 2) The temp functions are represented by multiple classes. Most are inaccessible from this | ||
| // package (e.g., HiveGenericUDF). | ||
| child.collect { | ||
| // Disallow creating permanent views based on temporary views. | ||
| case s: UnresolvedRelation | ||
| if sparkSession.sessionState.catalog.isTemporaryTable(s.tableIdentifier) => | ||
| throw new AnalysisException(s"Not allowed to create a permanent view $name by " + | ||
| s"referencing a temporary view ${s.tableIdentifier}") | ||
| case other if !other.resolved => other.expressions.flatMap(_.collect { | ||
| // Disallow creating permanent views based on temporary UDFs. | ||
| case e: UnresolvedFunction | ||
| if sparkSession.sessionState.catalog.isTemporaryFunction(e.name) => | ||
| throw new AnalysisException(s"Not allowed to create a permanent view $name by " + | ||
| s"referencing a temporary function `${e.name}`") | ||
| }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * If `userSpecifiedColumns` is defined, alias the analyzed plan to the user specified columns, | ||
| * else return the analyzed plan directly. | ||
|
|
@@ -270,15 +228,10 @@ case class AlterViewAsCommand( | |
| override def innerChildren: Seq[QueryPlan[_]] = Seq(query) | ||
|
|
||
| override def run(session: SparkSession): Seq[Row] = { | ||
| // If the plan cannot be analyzed, throw an exception and don't proceed. | ||
| val qe = session.sessionState.executePlan(query) | ||
| qe.assertAnalyzed() | ||
| val analyzedPlan = qe.analyzed | ||
|
|
||
| if (session.sessionState.catalog.alterTempViewDefinition(name, analyzedPlan)) { | ||
| if (session.sessionState.catalog.alterTempViewDefinition(name, query)) { | ||
| // a local/global temp view has been altered, we are done. | ||
| } else { | ||
| alterPermanentView(session, analyzedPlan) | ||
| alterPermanentView(session, query) | ||
| } | ||
|
|
||
| Seq.empty[Row] | ||
|
|
@@ -290,10 +243,6 @@ case class AlterViewAsCommand( | |
| throw new AnalysisException(s"${viewMeta.identifier} is not a view.") | ||
| } | ||
|
|
||
| // Detect cyclic view reference on ALTER VIEW. | ||
| val viewIdent = viewMeta.identifier | ||
| checkCyclicViewReference(analyzedPlan, Seq(viewIdent), viewIdent) | ||
|
|
||
| val newProperties = generateViewProperties(viewMeta.properties, session, analyzedPlan) | ||
|
|
||
| val updatedViewMeta = viewMeta.copy( | ||
|
|
@@ -356,66 +305,12 @@ object ViewHelper { | |
| properties: Map[String, String], | ||
| session: SparkSession, | ||
| analyzedPlan: LogicalPlan): Map[String, String] = { | ||
| val queryOutput = analyzedPlan.schema.fieldNames | ||
|
|
||
| // Generate the query column names, throw an AnalysisException if there exists duplicate column | ||
| // names. | ||
| SchemaUtils.checkColumnNameDuplication( | ||
| queryOutput, "the view", session.sessionState.conf.resolver) | ||
|
|
||
| // Generate the view default database name. | ||
| val queryOutput = analyzedPlan.schema.fieldNames | ||
| val viewDefaultDatabase = session.sessionState.catalog.getCurrentDatabase | ||
|
|
||
| removeQueryColumnNames(properties) ++ | ||
| generateViewDefaultDatabase(viewDefaultDatabase) ++ | ||
| generateQueryColumnNames(queryOutput) | ||
| } | ||
|
|
||
| /** | ||
| * Recursively search the logical plan to detect cyclic view references, throw an | ||
| * AnalysisException if cycle detected. | ||
| * | ||
| * A cyclic view reference is a cycle of reference dependencies, for example, if the following | ||
| * statements are executed: | ||
| * CREATE VIEW testView AS SELECT id FROM tbl | ||
| * CREATE VIEW testView2 AS SELECT id FROM testView | ||
| * ALTER VIEW testView AS SELECT * FROM testView2 | ||
| * The view `testView` references `testView2`, and `testView2` also references `testView`, | ||
| * therefore a reference cycle (testView -> testView2 -> testView) exists. | ||
| * | ||
| * @param plan the logical plan we detect cyclic view references from. | ||
| * @param path the path between the altered view and current node. | ||
| * @param viewIdent the table identifier of the altered view, we compare two views by the | ||
| * `desc.identifier`. | ||
| */ | ||
| def checkCyclicViewReference( | ||
| plan: LogicalPlan, | ||
| path: Seq[TableIdentifier], | ||
| viewIdent: TableIdentifier): Unit = { | ||
| plan match { | ||
| case v: View => | ||
| val ident = v.desc.identifier | ||
| val newPath = path :+ ident | ||
| // If the table identifier equals to the `viewIdent`, current view node is the same with | ||
| // the altered view. We detect a view reference cycle, should throw an AnalysisException. | ||
| if (ident == viewIdent) { | ||
| throw new AnalysisException(s"Recursive view $viewIdent detected " + | ||
| s"(cycle: ${newPath.mkString(" -> ")})") | ||
| } else { | ||
| v.children.foreach { child => | ||
| checkCyclicViewReference(child, newPath, viewIdent) | ||
| } | ||
| } | ||
| case _ => | ||
| plan.children.foreach(child => checkCyclicViewReference(child, path, viewIdent)) | ||
| } | ||
|
|
||
| // Detect cyclic references from subqueries. | ||
| plan.expressions.foreach { expr => | ||
| expr match { | ||
| case s: SubqueryExpression => | ||
| checkCyclicViewReference(s.plan, path, viewIdent) | ||
| case _ => // Do nothing. | ||
| } | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved
verifyTemporaryObjectsNotExiststorulesbecauseqe.assertAnalyzed()is called inrulesand a resolved plan is passed here.