Skip to content

[SPARK-19211][SQL] Explicitly prevent Insert into View or Create View As Insert #17125

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.

Sign up for GitHub

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
jiangxb1987 wants to merge 7 commits into from
Closed

[SPARK-19211][SQL] Explicitly prevent Insert into View or Create View As Insert #17125

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d1484f9
explictly prevent insert into view or create view as insert into table.
jiangxb1987 Mar 1, 2017
ae28e4b
update error message.
jiangxb1987 Mar 1, 2017
792cca9
detect CREATE VIEW AS INSERT INTO statement in parser.
jiangxb1987 Mar 2, 2017
57b64ad
check for INSERT INTO VIEW in PreprocessTableInsertion.
jiangxb1987 Mar 3, 2017
68cee40
update test cases.
jiangxb1987 Mar 4, 2017
9af2d7e
check insert into view in ResolveRelations
jiangxb1987 Mar 6, 2017
8d4be05
refactor.
jiangxb1987 Mar 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,7 @@ class Analyzer(

def apply(plan: LogicalPlan): LogicalPlan = plan resolveOperators {
case i @ InsertIntoTable(u: UnresolvedRelation, parts, child, _, _) if child.resolved =>
i.copy(table = EliminateSubqueryAliases(lookupTableFromCatalog(u)))
i.copy(table = resolveRelation(EliminateSubqueryAliases(lookupTableFromCatalog(u))))
Copy link
Contributor

@cloud-fan cloud-fan Mar 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change?

Copy link
Contributor Author

@jiangxb1987 jiangxb1987 Mar 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we try to insert into a view, the logical plan that lookupTableFromCatalog() returns is not resolved, so we have to perform resolveRelation() over the node.

Copy link
Contributor

@cloud-fan cloud-fan Mar 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we will resolve something to View by doing this?

Copy link
Contributor Author

@jiangxb1987 jiangxb1987 Mar 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will resolve the child of the view by doing this I think.

Copy link
Contributor

@cloud-fan cloud-fan Mar 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to resolve the child of view? Once we see the pattern Insert(View, ...) we will throw exception, we don't care about whether the child of view is resolved or not.

case u: UnresolvedRelation => resolveRelation(u)
}

Expand Down
2 changes: 0 additions & 2 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,8 @@
package org.apache.spark.sql.catalyst.analysis

import org.apache.spark.sql.AnalysisException
import org.apache.spark.sql.catalyst.catalog.SimpleCatalogRelation
import org.apache.spark.sql.catalyst.expressions._
import org.apache.spark.sql.catalyst.expressions.aggregate.AggregateExpression
import org.apache.spark.sql.catalyst.plans.UsingJoin
import org.apache.spark.sql.catalyst.plans.logical._
import org.apache.spark.sql.types._

Expand Down
10 changes: 10 additions & 0 deletions sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1329,6 +1329,16 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder {
if (ctx.identifierList != null) {
operationNotAllowed("CREATE VIEW ... PARTITIONED ON", ctx)
} else {
// CREATE VIEW ... AS INSERT INTO is not allowed.
val query = ctx.query.queryNoWith
query match {
Copy link
Member

@gatorsmile gatorsmile Mar 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: ctx.query.queryNoWith match {

case s: SingleInsertQueryContext if s.insertInto != null =>
Copy link
Contributor

@cloud-fan cloud-fan Mar 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when s.insertInto will be null?

Copy link
Contributor Author

@jiangxb1987 jiangxb1987 Mar 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, CREATE VIEW v AS SELECT * FROM jt.

Copy link
Contributor

@cloud-fan cloud-fan Mar 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, a select query is SingleInsertQueryContext?

Copy link
Contributor Author

@jiangxb1987 jiangxb1987 Mar 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... You can see that in:

queryNoWith
    : insertInto? queryTerm queryOrganization                                              #singleInsertQuery
    | fromClause multiInsertQueryBody+                                                     #multiInsertQuery
    ;

operationNotAllowed("CREATE VIEW ... AS INSERT INTO", ctx)
case m: MultiInsertQueryContext =>
Copy link
Member

@gatorsmile gatorsmile Mar 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: case _: MultiInsertQueryContext =>

operationNotAllowed("CREATE VIEW ... AS FROM ... [INSERT INTO ...]+", ctx)
case _ => // OK
}

val userSpecifiedColumns = Option(ctx.identifierCommentList).toSeq.flatMap { icl =>
icl.identifierComment.asScala.map { ic =>
ic.identifier.getText -> Option(ic.STRING).map(string)
Expand Down
4 changes: 4 additions & 0 deletions sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/rules.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,10 @@ case class PreprocessTableInsertion(conf: SQLConf) extends Rule[LogicalPlan] {
case LogicalRelation(_: InsertableRelation, _, catalogTable) =>
val tblName = catalogTable.map(_.identifier.quotedString).getOrElse("unknown")
preprocess(i, tblName, Nil)
// Inserting into a view is not allowed, we should throw an AnalysisException.
case v: View =>
throw new AnalysisException(s"${v.desc.identifier} is a view, inserting " +
"into a view is not allowed")
case _ => i
}
}
Expand Down
20 changes: 19 additions & 1 deletion sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ package org.apache.spark.sql.execution
import org.apache.spark.sql._
import org.apache.spark.sql.catalyst.TableIdentifier
import org.apache.spark.sql.catalyst.analysis.NoSuchTableException
import org.apache.spark.sql.catalyst.parser.ParseException
import org.apache.spark.sql.test.{SharedSQLContext, SQLTestUtils}

class SimpleSQLViewSuite extends SQLViewSuite with SharedSQLContext
Expand Down Expand Up @@ -172,7 +173,7 @@ abstract class SQLViewSuite extends QueryTest with SQLTestUtils {
var e = intercept[AnalysisException] {
sql(s"INSERT INTO TABLE $viewName SELECT 1")
}.getMessage
assert(e.contains("Inserting into an RDD-based table is not allowed"))
assert(e.contains("inserting into a view is not allowed"))

val dataFilePath =
Thread.currentThread().getContextClassLoader.getResource("data/files/employee.dat")
Expand Down Expand Up @@ -484,6 +485,23 @@ abstract class SQLViewSuite extends QueryTest with SQLTestUtils {
}
}

test("create view as insert into table") {
Copy link
Member

@gatorsmile gatorsmile Mar 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move it to SparkSqlParserSuite?

If we do it here, it will be tested twice.

withView("testView") {
// Single insert query
val e1 = intercept[ParseException] {
sql(s"CREATE VIEW testView AS INSERT INTO jt VALUES(1, 1)")
Copy link
Member

@gatorsmile gatorsmile Mar 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: remove string interpolator.

}.getMessage
assert(e1.contains("Operation not allowed: CREATE VIEW ... AS INSERT INTO"))

// Multi insert query
val e2 = intercept[ParseException] {
sql(s"CREATE VIEW testView AS FROM jt INSERT INTO tbl1 SELECT * WHERE jt.id < 5 " +
s"INSERT INTO tbl2 SELECT * WHERE jt.id > 4")
Copy link
Member

@gatorsmile gatorsmile Mar 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: remove the above two string interpolators

}.getMessage
assert(e2.contains("Operation not allowed: CREATE VIEW ... AS FROM ... [INSERT INTO ...]+"))
}
}

test("CTE within view") {
withView("cte_view") {
sql("CREATE VIEW cte_view AS WITH w AS (SELECT 1 AS n) SELECT n FROM w")
Expand Down