[SPARK-28443][SQL] Spark sql add exception when create field type NullType #25198

ulysses-you · 2019-07-19T02:12:44Z

What changes were proposed in this pull request?

More detail see 25085.
This PR is to discuss details that add exception when create field type NullType

Now, it's ok to create table like:

val schema = new StructType().add("c", NullType)
spark.catalog.createTable(
          tableName = "t",
          source = "json",
          schema = schema,
          options = Map.empty[String, String])

But it's not the spark idea

How was this patch tested?

UT

merge master

pull master

ulysses-you · 2019-07-19T02:13:22Z

cc @cloud-fan

maropu · 2019-07-19T05:14:32Z

ok to test

SparkQA · 2019-07-19T07:05:01Z

Test build #107883 has finished for PR 25198 at commit 791b3cb.

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

botekchristophe · 2019-07-19T16:43:28Z

Looks like a good idea to check this when creating a table with NullType

cloud-fan · 2019-07-22T07:49:25Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/rules.scala

+object CreateTableCheck extends Rule[LogicalPlan] {
+  override def apply(plan: LogicalPlan): LogicalPlan = {
+    plan match {
+      case ct: CreateTable if ct.tableDesc.schema.exists { f =>


can we move it to the rule PreWriteCheck?

I consider that it's a little different between create table(DDL) and insert data (DML). Maybe split in two rules ?

yea we can add a DDLCheck rule. Please follow PreWriteCheck to make it a pure-checking rule.

Also don't forget to handle DS v2 CREATE TABLE as well, which has a different logical plan.

CreateV2Table

CreateTableAsSelect

ReplaceTable

ReplaceTableAsSelect

also cc @rdblue

ok, I will check this later

SparkQA · 2019-07-23T17:24:21Z

Test build #108054 has finished for PR 25198 at commit fdaee89.

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

rdblue · 2019-07-24T18:48:54Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/rules.scala

+        failAnalysis("DataType NullType is not supported for create table")
+
+      // DataSourceStrategy will convert CreateTable to CreateDataSourceTableCommand before check
+      case cdstc: CreateDataSourceTableCommand if cdstc.table.schema.exists { f =>


I know this isn't much code, but I think it would be better to refactor this exists and the failAnalysis call into a method so it isn't repeated several times.

Thanks for your advise.

SparkQA · 2019-07-25T04:52:23Z

Test build #108135 has finished for PR 25198 at commit 9791914.

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

merge

…ARK-28443

SparkQA · 2019-08-07T13:15:42Z

Test build #108758 has finished for PR 25198 at commit 0ba1242.

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

cloud-fan · 2019-08-07T14:35:30Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/rules.scala

 }

+/**
+ * SPARK-28443: Spark sql add exception when create field type NullType


nit: SPARK-28443: fail the DDL command if it creates a table with null-type columns

cloud-fan · 2019-08-07T14:36:10Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/rules.scala

+  def failAnalysis(msg: String): Unit = { throw new AnalysisException(msg) }
+
+  def throwWhenExistsNullType(schema: StructType): Unit = {
+    if (schema.exists(f => DataTypes.NullType.sameType(f.dataType))) {


nit: f.dataType == NullType

what about nested filed? Do other databases forbid it as well?

Hive also forbid nested field with NullType

cloud-fan · 2019-08-07T14:37:49Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/rules.scala

+      case ReplaceTable(_, _, tableSchema, _, _, _) =>
+        throwWhenExistsNullType(tableSchema)
+
+      case _ => // OK


CreateTableAsSelect, ReplaceTableAsSelect are missing.

Spark allowed CreateTableAsSelect and ReplaceTableAsSelect with NullType, isn't it ?
E.g. create table test as select null as c1.

SparkQA · 2019-08-08T04:45:16Z

Test build #108791 has finished for PR 25198 at commit 517a1e6.

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

cloud-fan · 2019-08-08T07:16:51Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/rules.scala

+      case CreateTable(tableDesc, _, _) =>
+        checkSchema(tableDesc.schema)
+
+      case CreateV2Table(_, _, tableSchema, _, _, _) =>


I think the rationale here is that, we don't want to create tables with null type field. It should be same for both CREATE TABLE and CTAS.

OK, and also add AlterTable.

SparkQA · 2019-08-08T11:17:05Z

Test build #108811 has finished for PR 25198 at commit 138ac5e.

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

cloud-fan · 2019-08-08T12:23:28Z

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

+  test("SPARK-28443: fail the DDL command if it creates a table with null-type columns") {
+    withTable("t") {
+      val e = intercept[AnalysisException]{
+        sql(s"CREATE TABLE t as SELECT NULL AS c")


can we also test inner fields?

cloud-fan · 2019-08-08T12:24:15Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/rules.scala

+        fields.foreach{field => throwWhenExistsNullType(field.dataType)}
+
+      case other if other == NullType =>
+        failAnalysis("DataType NullType is not supported for create table.")


how about "cannot create tables with null-type column/field"

cloud-fan · 2019-08-08T12:24:44Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/rules.scala

+
+  def failAnalysis(msg: String): Unit = { throw new AnalysisException(msg) }
+
+  def throwWhenExistsNullType(dataType: DataType): Unit = {


failNullType

cloud-fan · 2019-08-08T12:25:50Z

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

    }
  }
+
+  test("SPARK-28443: fail the DDL command if it creates a table with null-type columns") {


BTW do we need this test? I think https://github.com/apache/spark/pull/25198/files#diff-85f240f8452fa64de6671e41801fe68fR550 is good enough.

I forget that spark not support create table t as select null as c now. Remove this.

SparkQA · 2019-08-08T17:26:22Z

Test build #108833 has finished for PR 25198 at commit 108b7f9.

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

SparkQA · 2019-08-09T02:52:26Z

Test build #108848 has finished for PR 25198 at commit fa22cf7.

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

ulysses-you · 2019-08-12T01:31:57Z

ping @cloud-fan

SparkQA · 2019-09-30T04:14:52Z

Test build #111595 has finished for PR 25198 at commit 6a1b4f5.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

…8443

SparkQA · 2019-09-30T10:07:21Z

Test build #111605 has finished for PR 25198 at commit 358e2a2.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

github-actions · 2020-01-09T00:06:17Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

ulysses-you and others added 4 commits July 9, 2019 14:38

Merge pull request #1 from apache/master

b259859

merge master

Merge pull request #2 from apache/master

33a8b0e

pull master

SPARK-28443

adadf3a

update scala style

791b3cb

ulysses-you mentioned this pull request Jul 19, 2019

[SPARK-28313][SQL] Spark sql null type incompatible with hive void type #25085

Closed

dongjoon-hyun added the SQL label Jul 20, 2019

cloud-fan reviewed Jul 22, 2019

View reviewed changes

update DDL check

fdaee89

rdblue requested changes Jul 24, 2019

View reviewed changes

make code better

9791914

ulysses-you and others added 3 commits August 2, 2019 08:51

Merge pull request #3 from apache/master

5c7e871

merge

Merge branch 'master' of https://github.com/ulysses-you/spark into SP…

1e3af37

…ARK-28443

update

0ba1242

cloud-fan reviewed Aug 7, 2019

View reviewed changes

add nested check

517a1e6

cloud-fan reviewed Aug 8, 2019

View reviewed changes

update rules

138ac5e

cloud-fan reviewed Aug 8, 2019

View reviewed changes

update

108b7f9

fix

fa22cf7

ulysses-you requested a review from rdblue September 30, 2019 04:06

ulysses-you added 2 commits September 30, 2019 17:52

Merge branch 'master' of https://github.com/apache/spark into SPARK-2…

f5a51a7

…8443

fix conflict

358e2a2

ulysses-you force-pushed the SPARK-28443 branch from 6a1b4f5 to 358e2a2 Compare September 30, 2019 09:56

github-actions bot added the Stale label Jan 9, 2020

github-actions bot closed this Jan 10, 2020

LantaoJin mentioned this pull request Jun 16, 2020

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

Closed


		def failAnalysis(msg: String): Unit = { throw new AnalysisException(msg) }

		def throwWhenExistsNullType(dataType: DataType): Unit = {

[SPARK-28443][SQL] Spark sql add exception when create field type NullType #25198

[SPARK-28443][SQL] Spark sql add exception when create field type NullType #25198

Uh oh!

Conversation

ulysses-you commented Jul 19, 2019 • 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!

ulysses-you commented Jul 19, 2019

Uh oh!

maropu commented Jul 19, 2019

Uh oh!

SparkQA commented Jul 19, 2019

Uh oh!

botekchristophe commented Jul 19, 2019

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!

SparkQA commented Jul 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 25, 2019

Uh oh!

SparkQA commented Aug 7, 2019

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!

ulysses-you Aug 8, 2019 • 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 Aug 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 8, 2019

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!

SparkQA commented Aug 8, 2019

Uh oh!

SparkQA commented Aug 9, 2019

Uh oh!

ulysses-you commented Aug 12, 2019

Uh oh!

SparkQA commented Sep 30, 2019

Uh oh!

SparkQA commented Sep 30, 2019

Uh oh!

github-actions bot commented Jan 9, 2020

Uh oh!

Reviewers

ulysses-you commented Jul 19, 2019 •

edited

Loading

ulysses-you Aug 8, 2019 •

edited

Loading