[SPARK-32356][SQL] Forbid create view with null type #29152

ulysses-you · 2020-07-18T10:47:22Z

What changes were proposed in this pull request?

After #28833, we forbid create table with null type, but not include the view. This pr aims to forbid creat view with null type.

Why are the changes needed?

Currently, we support create view v as select null with in-memory but failed in hive. It's better to have same behavior of them.

inclued 4 command:

create view v as select null
alter view v as select null
create temporary view v as select null
create global temporary view v as select null

Does this PR introduce any user-facing change?

Yes, user will get exception when create view with null type .

How was this patch tested?

Add ut.

SparkQA · 2020-07-18T15:41:40Z

Test build #126106 has finished for PR 29152 at commit d5a5575.

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

dongjoon-hyun · 2020-07-19T19:21:58Z

Thank you, @ulysses-you .
cc @cloud-fan

cloud-fan · 2020-07-20T05:57:48Z

sql/core/src/test/scala/org/apache/spark/sql/execution/command/PlanResolutionSuite.scala

@@ -1557,6 +1557,17 @@ class PlanResolutionSuite extends AnalysisTest {
    checkFailure("testcat.tab", "foo")
  }

+  test("SPARK-32356: forbid null type in create view") {
+    val sql1 = "create view v as select null as c"
+    val sql2 = "alter view v as select null as c"


can we test temp view as well? also the df api df.createTempView

cloud-fan · 2020-07-20T05:58:19Z

sql/core/src/test/scala/org/apache/spark/sql/execution/command/PlanResolutionSuite.scala

+      val msg = intercept[AnalysisException] {
+        parseAndResolve(sql)
+      }.getMessage
+      assert(msg.contains(s"Cannot create tables with ${NullType.simpleString} type."))


shall we update the error message to be tables/views?

SparkQA · 2020-07-20T11:36:50Z

Test build #126175 has finished for PR 29152 at commit 2e8d9f3.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-07-20T11:50:07Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala

@@ -2540,6 +2540,17 @@ class DataFrameSuite extends QueryTest
      assert(df.schema === new StructType().add(StructField("d", DecimalType(38, 0))))
    }
  }
+
+  test("SPARK-32356: forbid null type in create view") {


can we put test in one place SQLViewSuite?

ulysses-you · 2020-07-20T12:34:54Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala

@@ -2314,6 +2314,7 @@ class HiveDDLSuite
  }

  test("SPARK-20680: do not support for null column datatype") {
+    val errMsg = s"Cannot create tables/views with ${NullType.simpleString} type."


changed old error msg tables -> tables/views.

SparkQA · 2020-07-20T14:25:18Z

Test build #126181 has finished for PR 29152 at commit e1d92ae.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-07-20T14:52:34Z

sql/core/src/test/scala/org/apache/spark/sql/execution/command/PlanResolutionSuite.scala

@@ -1557,6 +1557,19 @@ class PlanResolutionSuite extends AnalysisTest {
    checkFailure("testcat.tab", "foo")
  }

+  test("SPARK-32356: forbid null type in create view") {


can we move this to SQLViewSuite as well?

ulysses-you · 2020-07-21T00:16:44Z

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

@@ -96,6 +97,7 @@ case class CreateViewCommand(
    qe.assertAnalyzed()
    val analyzedPlan = qe.analyzed

+    assertNoNullTypeInSchema(analyzedPlan.schema)


This check is for df.createTempView. Note that, we support select null as c but fail in create view with null type plan.

SparkQA · 2020-07-21T02:23:16Z

Test build #126208 has finished for PR 29152 at commit 94fb546.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-07-21T04:19:28Z

Test build #126211 has finished for PR 29152 at commit 4d8d088.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-07-21T05:51:03Z

LGTM if tests pass.

One last question: Hive also fails to create view if the column is null type?

ulysses-you · 2020-07-21T12:29:09Z

@cloud-fan Hive support this

create view v as select null as c from (select * from t1)t1

That said, we will get c null if run desc v.

cloud-fan · 2020-07-21T12:34:52Z

Currently, we support create view v as select null with in-memory but failed in hive.

how does this happen if Hive supports it?

ulysses-you · 2020-07-21T12:48:06Z

@cloud-fan
The reason is NullType simpleString is null but hive need a void, after #28833 we only support parse void -> NullType.
So when we use NullType to create a hive view, it failed.

cloud-fan · 2020-07-21T13:07:42Z

And it works if we change NullType.toString to void?

To clarify, this case never works in Spark?

ulysses-you · 2020-07-21T13:23:22Z

NullType.toString you mean NullType.simpleString ? yes it works if return void.

To clarify, this case never works in Spark?

Yes, never works.

cloud-fan · 2020-07-21T13:51:38Z

cc @maropu @viirya do you have any opinions? This PR looks fine if it never worked and this just produces a better error message. But I'm wondering if we should support it to follow hive.

maropu · 2020-07-21T14:40:16Z

hm, at least, most databases can accpet it create view v as select null, but null is regared as another type, e.g., a text in postgresql;

postgres=# create view v as select null;
CREATE VIEW
postgres=# \d v
                 View "public.v"
  Column  | Type | Collation | Nullable | Default 
----------+------+-----------+----------+---------
 ?column? | text |           |          |

MySQL: http://sqlfiddle.com/#!9/42febe/1/0
SQLite: http://sqlfiddle.com/#!7/42feb/1/0

So, IMO accepting it looks somewhat meaningful to follow other databases, but we don't need to handle it as a null type. Either way, I think its better to update the migration guide if we change thsi behaivour.

viirya · 2020-07-21T19:03:01Z

But create view v as select null with in-memory works before, right, according to the description? This changes the behavior.

cloud-fan · 2020-07-22T17:52:14Z

I'd treat it as not-working if it ONLY works with in-memory catalog. In production no one use it. The official release of Spark always include the hive dependencies.

ulysses-you · 2020-07-23T00:25:50Z

How about give user a config that convertNullAsString, then we convert null as StringType if the config is enable ?

In my practice, most time select null as c means c is a meaningless string. Just like Java String str = null.

viirya · 2020-07-23T00:41:09Z

I'd treat it as not-working if it ONLY works with in-memory catalog. In production no one use it. The official release of Spark always include the hive dependencies.

Oh ok, it makes sense. For a better error message, this PR is fine.

maropu · 2020-07-23T00:47:13Z

Looks okay to me, but (this is trivial though) we might need to choose either action?;

jira type: improvement -> bug, then backport this to branch-3.0/branch-2.4, or
update the migration guide for the behaviour change

cc: @dongjoon-hyun

cloud-fan · 2020-07-23T07:59:37Z

If it just improves error message, I think we don't need to backport. My question is if we want to support it. It seems it's easy to support it, and hive supports it as well.

maropu · 2020-07-23T09:25:33Z

Forbitting it in in-memory catalog, too, looks ok to me. If we get a user's request about the behaivour, we can easily fix it.

ulysses-you · 2020-07-24T00:54:50Z

It seems df.createTempView is a use case of NullType with in-memory.

During resolve failed test, I find many place use create temp view as select null or df.createTempView. The result is, we can't or hard to test anything about NullType if we forbid create view with it.

e.g.,

test function/udf that accept null
test sizeInBytes in memory that include null column

I'm not confident to remove all that test.

SparkQA · 2020-07-24T03:22:47Z

Test build #126459 has finished for PR 29152 at commit 3cb224f.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-07-24T04:36:57Z

It seems df.createTempView is a use case of NullType with in-memory.

That's a good point. Then I think we should support NullType in permanent views as well. @ulysses-you can you open a PR to do it?

ulysses-you · 2020-07-24T05:37:07Z

@cloud-fan ok may be tomorrow.

SparkQA · 2020-07-24T07:05:02Z

Test build #126471 has finished for PR 29152 at commit d02986c.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2020-07-24T18:47:03Z

@cloud-fan . Just for my understanding, do you mean to reverting [SPARK-20680][SQL] Spark-sql do not support for creating table with void column datatype, too? Or, it's only limited on Views (temp/permanant/global_temp)?

Then I think we should support NullType in permanent views as well.

cc @LantaoJin

cloud-fan · 2020-07-27T02:56:09Z

do you mean to reverting [SPARK-20680][SQL] Spark-sql do not support for creating table with void column datatype, too?

I mean to override NullType.simpleString as void, which was reverted before.

init

d5a5575

probot-autolabeler bot added the SQL label Jul 18, 2020

ulysses-you mentioned this pull request Jul 18, 2020

[SPARK-20680][SQL] Spark-sql do not support for creating table with void column datatype #28833

Closed

cloud-fan reviewed Jul 20, 2020

View reviewed changes

ulysses-you added 3 commits July 20, 2020 17:17

update error msg

95dd3d2

add temp view test

3780ccd

add dataset ut

2e8d9f3

cloud-fan reviewed Jul 20, 2020

View reviewed changes

ulysses-you added 2 commits July 20, 2020 20:26

move to SQLViewSuite

9d77cb5

update ut

e1d92ae

ulysses-you commented Jul 20, 2020

View reviewed changes

cloud-fan reviewed Jul 20, 2020

View reviewed changes

ulysses-you added 2 commits July 21, 2020 08:10

add view command check

32ec11a

mv to SQLViewSuite

94fb546

ulysses-you commented Jul 21, 2020

View reviewed changes

fix ut name

4d8d088

maropu approved these changes Jul 23, 2020

View reviewed changes

remove null

3cb224f

remove null

d02986c

ulysses-you mentioned this pull request Jul 26, 2020

[SPARK-20680][SQL][FOLLOW-UP] Make NullType.simpleString as void to support hive #29244

Closed

ulysses-you closed this Aug 21, 2020

linhongliu-db mentioned this pull request Jul 20, 2021

[SPARK-36224][SQL] Use Void as the type name of NullType #33437

Closed

[SPARK-32356][SQL] Forbid create view with null type #29152

[SPARK-32356][SQL] Forbid create view with null type #29152

Conversation

ulysses-you commented Jul 18, 2020 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

SparkQA commented Jul 18, 2020

dongjoon-hyun commented Jul 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 21, 2020

SparkQA commented Jul 21, 2020

cloud-fan commented Jul 21, 2020

ulysses-you commented Jul 21, 2020

cloud-fan commented Jul 21, 2020

ulysses-you commented Jul 21, 2020

cloud-fan commented Jul 21, 2020

ulysses-you commented Jul 21, 2020

cloud-fan commented Jul 21, 2020

maropu commented Jul 21, 2020

viirya commented Jul 21, 2020

cloud-fan commented Jul 22, 2020 • edited

ulysses-you commented Jul 23, 2020

viirya commented Jul 23, 2020 • edited

maropu commented Jul 23, 2020

cloud-fan commented Jul 23, 2020

maropu commented Jul 23, 2020

ulysses-you commented Jul 24, 2020

SparkQA commented Jul 24, 2020

cloud-fan commented Jul 24, 2020

ulysses-you commented Jul 24, 2020

SparkQA commented Jul 24, 2020

dongjoon-hyun commented Jul 24, 2020 • edited

cloud-fan commented Jul 27, 2020

ulysses-you commented Jul 18, 2020 •

edited

cloud-fan commented Jul 22, 2020 •

edited

viirya commented Jul 23, 2020 •

edited

dongjoon-hyun commented Jul 24, 2020 •

edited