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

jiangxb1987 · 2017-03-01T16:58:03Z

What changes were proposed in this pull request?

Currently we don't explicitly forbid the following behaviors:

The statement CREATE VIEW AS INSERT INTO throws the following exception:

scala> spark.sql("CREATE VIEW testView AS INSERT INTO tab VALUES (1, \"a\")")
org.apache.spark.sql.AnalysisException: org.apache.hadoop.hive.ql.metadata.HiveException: org.apache.hadoop.hive.ql.metadata.HiveException: at least one column must be specified for the table;
 scala> spark.sql("CREATE VIEW testView(a, b) AS INSERT INTO tab VALUES (1, \"a\")")
org.apache.spark.sql.AnalysisException: The number of columns produced by the SELECT clause (num: `0`) does not match the number of column names specified by CREATE VIEW (num: `2`).;

The statement INSERT INTO view VALUES throws the following exception from checkAnalysis:

scala> spark.sql("INSERT INTO testView VALUES (1, \"a\")")
org.apache.spark.sql.AnalysisException: Inserting into an RDD-based table is not allowed.;;
'InsertIntoTable View (`default`.`testView`, [a#16,b#17]), false, false
+- LocalRelation [col1#14, col2#15]

After this PR, the behavior changes to:

scala> spark.sql("CREATE VIEW testView AS INSERT INTO tab VALUES (1, \"a\")")
org.apache.spark.sql.catalyst.parser.ParseException: Operation not allowed: CREATE VIEW ... AS INSERT INTO;

scala> spark.sql("CREATE VIEW testView(a, b) AS INSERT INTO tab VALUES (1, \"a\")")
org.apache.spark.sql.catalyst.parser.ParseException: Operation not allowed: CREATE VIEW ... AS INSERT INTO;

scala> spark.sql("INSERT INTO testView VALUES (1, \"a\")")
org.apache.spark.sql.AnalysisException: `default`.`testView` is a view, inserting into a view is not allowed;

How was this patch tested?

Add a new test case in SparkSqlParserSuite;
Update the corresponding test case in SQLViewSuite.

SparkQA · 2017-03-01T19:07:16Z

Test build #73696 has finished for PR 17125 at commit ae28e4b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

jiangxb1987 · 2017-03-02T01:33:59Z

cc @gatorsmile @cloud-fan Please have a look at this when you have time, thanks!

gatorsmile · 2017-03-02T06:24:25Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

+        // Inserting into a view is not allowed, we should throw an AnalysisException.
+        newTable match {
+          case v: View =>
+            u.failAnalysis(s"${v.desc.identifier} is a view, inserting into a view is not allowed")


Can we move this to PreprocessTableInsertion?

gatorsmile · 2017-03-02T06:28:47Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala


+    // CREATE VIEW AS INSERT INTO ... is not allowed, we should throw an AnalysisException.
+    analyzedPlan match {
+      case i: InsertIntoHadoopFsRelationCommand =>


_: InsertIntoHadoopFsRelationCommand

shall we forbid all commands? e.g. CREATE VIEW xxx AS CREATE TABLE ... should also be disallowed right?

The sql parser only allows CREATE VIEW AS query here, a query can only be a SELECT ... or INSERT INTO ... or a CTE, so perhaps we don't have to consider other commands here.

hmmmm, why INSERT INTO ... is a query?

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

Seems we have mixed them together.

can we fix it at parser side? cc @hvanhovell

Hmmm... fixing this in grammar itself would a little bit of work (we also support multi-insert and that makes this harder). I suppose we could try to add a check in the AstBuilder.

Why not check this in the CreateViewCommand? That seems safer to me.

gatorsmile · 2017-03-02T06:28:59Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

+    analyzedPlan match {
+      case i: InsertIntoHadoopFsRelationCommand =>
+        throw new AnalysisException("Creating a view as insert into a table is not allowed")
+      case i: InsertIntoDataSourceCommand =>


The same here

gatorsmile · 2017-03-02T06:29:43Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

+    // CREATE VIEW AS INSERT INTO ... is not allowed, we should throw an AnalysisException.
+    analyzedPlan match {
+      case i: InsertIntoHadoopFsRelationCommand =>
+        throw new AnalysisException("Creating a view as insert into a table is not allowed")


It will be nice to put a view name in the error message.

hvanhovell · 2017-03-02T10:21:01Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

-        i.copy(table = EliminateSubqueryAliases(lookupTableFromCatalog(u)))
+        val newTable = EliminateSubqueryAliases(lookupTableFromCatalog(u))
+        // Inserting into a view is not allowed, we should throw an AnalysisException.
+        newTable match {


NIT In this case if would just write the following:

if (newTable.isInstanceOf[View]) { u.failAnalysis(s"${v.desc.identifier} is a view, inserting into a view is not allowed") }

hvanhovell · 2017-03-02T10:22:42Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

    val analyzedPlan = qe.analyzed

+    // CREATE VIEW AS INSERT INTO ... is not allowed, we should throw an AnalysisException.
+    analyzedPlan match {


This pattern match does not work with multi-inserts (hive feature). Those are represented using a Union of inserts.

jiangxb1987 · 2017-03-02T17:16:13Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

-        i.copy(table = EliminateSubqueryAliases(lookupTableFromCatalog(u)))
+        val newTable = EliminateSubqueryAliases(lookupTableFromCatalog(u))
+        // Inserting into a view is not allowed, we should throw an AnalysisException.
+        if (newTable.isInstanceOf[View]) {


The rule ResolveRelations executes before PreprocessTableInsertion, so we can fail early here.

We prefer to doing all the error handling in the same rule. It can help us find the hole and maintain the codes. cc @cloud-fan

In fact I'm neutral with which rule this code block should be placed, but I feel this is not a typical "Preprocess" since the query failed and the process ends. @cloud-fan @hvanhovell any suggestions?

Just move it to preprocess insert.

Discussed with @cloud-fan and the concern is that in case the child of the view node is invalid(e.g. exists cyclic view reference or exceed max reference depth), we should still indicate that INSERT INTO VIEW is not allowed(instead of other error messages), so we should not resolve the child of the view, instead we throw an Exception here.

SparkQA · 2017-03-02T19:15:02Z

Test build #73780 has finished for PR 17125 at commit 792cca9.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-03-03T18:05:26Z

Test build #73857 has finished for PR 17125 at commit 57b64ad.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-03-03T18:09:24Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala

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


Nit: remove string interpolator.

gatorsmile · 2017-03-03T18:09:35Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala

+      // 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")


Nit: remove the above two string interpolators

gatorsmile · 2017-03-03T18:12:50Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewSuite.scala

    }
  }

+  test("create view as insert into table") {


Move it to SparkSqlParserSuite?

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

gatorsmile · 2017-03-03T18:16:12Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

    } else {
+      // CREATE VIEW ... AS INSERT INTO is not allowed.
+      val query = ctx.query.queryNoWith
+      query match {


Nit: ctx.query.queryNoWith match {

gatorsmile · 2017-03-03T18:16:17Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

+      query match {
+        case s: SingleInsertQueryContext if s.insertInto != null =>
+          operationNotAllowed("CREATE VIEW ... AS INSERT INTO", ctx)
+        case m: MultiInsertQueryContext =>


Nit: case _: MultiInsertQueryContext =>

cloud-fan · 2017-03-04T05:47:57Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

    } else {
+      // CREATE VIEW ... AS INSERT INTO is not allowed.
+      ctx.query.queryNoWith match {
+        case s: SingleInsertQueryContext if s.insertInto != null =>


when s.insertInto will be null?

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

hmm, a select query is SingleInsertQueryContext?

Yeah... You can see that in:

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

cloud-fan · 2017-03-04T05:49:19Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

    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))))


why this change?

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.

so we will resolve something to View by doing this?

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

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.

cloud-fan · 2017-03-04T05:50:37Z

LGTM except 2 questions

SparkQA · 2017-03-04T07:45:54Z

Test build #73894 has finished for PR 17125 at commit 68cee40.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-03-06T05:07:07Z

Test build #73958 has finished for PR 17125 at commit 9af2d7e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-03-06T05:17:16Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

+        // Inserting into a view is not allowed, we should throw an AnalysisException.
+        if (newTable.isInstanceOf[View]) {
+          u.failAnalysis(s"${newTable.asInstanceOf[View].desc.identifier} is a view, inserting " +
+            s"into a view is not allowed")


Nit: s"into a view is not allowed" -> "into a view is not allowed"

gatorsmile · 2017-03-06T05:24:54Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

+          u.failAnalysis(s"${newTable.asInstanceOf[View].desc.identifier} is a view, inserting " +
+            s"into a view is not allowed")
+        }
+        i.copy(table = newTable)


How about?

lookupTableFromCatalog(u).canonicalized match { case v: View => u.failAnalysis(s"Inserting into a view is not allowed. View: ${v.desc.identifier}.") case other => i.copy(table = other) }

SparkQA · 2017-03-06T07:32:32Z

Test build #73978 has started for PR 17125 at commit 8d4be05.

cloud-fan · 2017-03-06T08:08:29Z

LGTM, pending tests

cloud-fan · 2017-03-06T08:08:53Z

retest this please

SparkQA · 2017-03-06T10:11:00Z

Test build #73981 has finished for PR 17125 at commit 8d4be05.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-03-06T20:35:29Z

LGTM too.

gatorsmile · 2017-03-06T20:35:37Z

Thanks! Merging to master.

jiangxb1987 added 2 commits March 1, 2017 23:51

explictly prevent insert into view or create view as insert into table.

d1484f9

update error message.

ae28e4b

gatorsmile reviewed Mar 2, 2017

View reviewed changes

hvanhovell reviewed Mar 2, 2017

View reviewed changes

detect CREATE VIEW AS INSERT INTO statement in parser.

792cca9

jiangxb1987 commented Mar 2, 2017

View reviewed changes

check for INSERT INTO VIEW in PreprocessTableInsertion.

57b64ad

gatorsmile reviewed Mar 3, 2017

View reviewed changes

update test cases.

68cee40

cloud-fan reviewed Mar 4, 2017

View reviewed changes

check insert into view in ResolveRelations

9af2d7e

gatorsmile reviewed Mar 6, 2017

View reviewed changes

refactor.

8d4be05

asfgit closed this in 9991c2d Mar 6, 2017

jiangxb1987 deleted the insert-with-view branch March 7, 2017 02:05

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

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

Uh oh!

Conversation

jiangxb1987 commented Mar 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Mar 1, 2017

Uh oh!

jiangxb1987 commented Mar 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiangxb1987 Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 2, 2017

Uh oh!

SparkQA commented Mar 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiangxb1987 Mar 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jiangxb1987 commented Mar 1, 2017 •

edited

Loading

jiangxb1987 Mar 3, 2017 •

edited

Loading

gatorsmile Mar 3, 2017 •

edited

Loading

jiangxb1987 Mar 4, 2017 •

edited

Loading