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

ulysses-you · 2020-07-26T13:39:01Z

What changes were proposed in this pull request?

For better compatible with Hive in create view with null.

Change NullType.simpleString to void, then Hive can parse void to it's own type.

Why are the changes needed?

Currently, we support create view v as select null with in-memory but failed in hive. We try to forbid this in #29152, however df.createTempView is already a use case of NullType with in-memory. So it seems better to keep this behavior consistent with hive.

Some error msg with create view v as select null.

org.apache.spark.SparkException: Cannot recognize hive type string: null, column: NULL
	at org.apache.spark.sql.hive.client.HiveClientImpl$.getSparkSQLDataType(HiveClientImpl.scala:999)
	at org.apache.spark.sql.hive.client.HiveClientImpl$.$anonfun$verifyColumnDataType$1(HiveClientImpl.scala:1021)
	at scala.collection.Iterator.foreach(Iterator.scala:941)
	at scala.collection.Iterator.foreach$(Iterator.scala:941)
	at scala.collection.AbstractIterator.foreach(Iterator.scala:1429)
	at scala.collection.IterableLike.foreach(IterableLike.scala:74)
	at scala.collection.IterableLike.foreach$(IterableLike.scala:73)
	at org.apache.spark.sql.types.StructType.foreach(StructType.scala:102)

Does this PR introduce any user-facing change?

Yes, user can create view with null type in Hive.

How was this patch tested?

New UT with just Hive. in-memory test is already exists.

SparkQA · 2020-07-26T14:46:53Z

Test build #126572 has finished for PR 29244 at commit 2fea01c.

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

ulysses-you · 2020-07-27T00:02:24Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala

      c.metadata.getString(HIVE_TYPE_STRING)
    } else {
-      c.dataType.catalogString
+      c.dataType.sql


we should use .sql to hive type.

SparkQA · 2020-07-27T01:23:14Z

Test build #126594 has finished for PR 29244 at commit 9b12177.

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

cloud-fan · 2020-07-27T02:58:08Z

sql/catalyst/src/main/scala/org/apache/spark/sql/types/NullType.scala


  private[spark] override def asNullable: NullType = this
+
+  override def sql: String = "VOID"


why not override simpleString? sql calls simpleString

Just consider that simpleString/catalogString is a name of spark, and sql is used in other database ? We can still show something like unknown with NullType, If we can divide this.

I'm not sure the sql original meaning, but it seems used for Hive DDL e.g., StructType.toDDL use sql to build DDL string.

It's confusing if simpleString and sql is not consistent.

LantaoJin · 2020-07-27T03:25:19Z

Could we change this title to a FOLLOWUP of SPARK-20680, like #29041. It can help backport more clear

ulysses-you · 2020-07-27T05:54:20Z

@LantaoJin mostly we can, but I'm not sure whether this pr is only for revert simpleString.

ulysses-you · 2020-07-27T12:26:08Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

      case ("decimal" | "dec" | "numeric", precision :: scale :: Nil) =>
        DecimalType(precision.getText.toInt, scale.getText.toInt)
      case ("void", Nil) => NullType
+      case ("null", Nil) => NullType


this is for compatible, likefrom_csv.

In which case do we need it? If we have this PR, the valid string for null type should be void. Before this PR, it wasn't supported before.

Forgot it, we not support parse null before.

SparkQA · 2020-07-27T14:07:03Z

Test build #126643 has finished for PR 29244 at commit 9e56ff3.

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

SparkQA · 2020-07-27T15:26:38Z

Test build #126645 has finished for PR 29244 at commit 8a5b2e9.

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

SparkQA · 2020-07-27T15:42:04Z

Test build #126642 has finished for PR 29244 at commit 9deabe4.

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

SparkQA · 2020-07-28T05:00:54Z

Test build #126676 has finished for PR 29244 at commit 1c84fc8.

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

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

Test build #126697 has finished for PR 29244 at commit 65137da.

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

LantaoJin · 2020-07-28T07:07:22Z

retest this please

SparkQA · 2020-07-28T11:55:30Z

Test build #126702 has finished for PR 29244 at commit 65137da.

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

cloud-fan · 2020-07-28T13:25:52Z

Another safer fix is: In HiveClientImpl.createTable, replace NullType with a new HiveVoidType whose simpleString returns void, before generating the type string and create Hive FieldSchema.

The question is: do we think void is a better type name than null when doing EXPLAIN?

cc @dongjoon-hyun @maropu @viirya

dongjoon-hyun · 2020-07-28T16:30:06Z

For me, null is better inside Apache Spark EXPLAIN because I have been requesting to avoid to have a string literal void inside Apache Spark code base. However, this PR has a valid use case. After this PR, I'm okay for both cases because we cannot eliminate void completely.

cloud-fan · 2020-08-11T06:03:50Z

How about we be conservative and follow #29244 (comment) first? @ulysses-you

ulysses-you · 2020-08-13T00:46:14Z

@cloud-fan you mean something like this pr #28935 ?

As I know, we can just update HiveClientImpl.toHiveColumn and HiveClientImpl.fromHiveColumn to replace NullType to HiveVoidType if we choose this action. Since we already forbid NullType with table and void won't exist with view related functionality during parsing.

cloud-fan · 2020-08-13T03:33:42Z

Yea similar but more narrowed to HiveClientImpl. We can even define the class HiveVoidType in HiveClientImpl.

ulysses-you · 2020-08-13T05:12:56Z

Created #29423.

### What changes were proposed in this pull request? Discussion with [comment](#29244 (comment)). Add `HiveVoidType` class in `HiveClientImpl` then we can replace `NullType` to `HiveVoidType` before we call hive client. ### Why are the changes needed? Better compatible with hive. More details in [#29244](#29244). ### Does this PR introduce _any_ user-facing change? Yes, user can create view with null type in Hive. ### How was this patch tested? New test. Closes #29423 from ulysses-you/add-HiveVoidType. Authored-by: ulysses <youxiduo@weidian.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

ulysses-you · 2020-08-15T09:44:12Z

close this since #29423 merged.

init

2fea01c

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

fix toHiveColumn

9b12177

ulysses-you commented Jul 27, 2020

View reviewed changes

cloud-fan reviewed Jul 27, 2020

View reviewed changes

change to void

9deabe4

probot-autolabeler bot added the PYTHON label Jul 27, 2020

ulysses-you changed the title ~~[SPARK-32445][SQL] Make NullType.sql as VOID to support hive~~ [SPARK-32445][SQL] Make NullType.simpleString as VOID to support hive Jul 27, 2020

ulysses-you changed the title ~~[SPARK-32445][SQL] Make NullType.simpleString as VOID to support hive~~ [SPARK-32445][SQL] Make NullType.simpleString as void to support hive Jul 27, 2020

revert sql

9e56ff3

ulysses-you changed the title ~~[SPARK-32445][SQL] Make NullType.simpleString as void to support hive~~ [SPARK-20680][SQL][FOLLOW-UP] Make NullType.simpleString as void to support hive Jul 27, 2020

ulysses-you commented Jul 27, 2020

View reviewed changes

remove parse null

8a5b2e9

fix ut

1c84fc8

fix null

65137da

ulysses-you mentioned this pull request Aug 13, 2020

[SPARK-20680][SQL][FOLLOW-UP] Add HiveVoidType in HiveClientImpl #29423

Closed

ulysses-you closed this Aug 15, 2020


		private[spark] override def asNullable: NullType = this

		override def sql: String = "VOID"

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

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

Uh oh!

Conversation

ulysses-you commented Jul 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

SparkQA commented Jul 26, 2020

Uh oh!

ulysses-you Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 27, 2020

Uh oh!

cloud-fan Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

ulysses-you Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

LantaoJin commented Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ulysses-you commented Jul 27, 2020

Uh oh!

ulysses-you Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

ulysses-you Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 27, 2020

Uh oh!

SparkQA commented Jul 27, 2020

Uh oh!

SparkQA commented Jul 27, 2020

Uh oh!

SparkQA commented Jul 28, 2020

Uh oh!

SparkQA commented Jul 28, 2020

Uh oh!

LantaoJin commented Jul 28, 2020

Uh oh!

SparkQA commented Jul 28, 2020

Uh oh!

cloud-fan commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented Jul 28, 2020

Uh oh!

cloud-fan commented Aug 11, 2020

Uh oh!

ulysses-you commented Aug 13, 2020

Uh oh!

cloud-fan commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ulysses-you commented Aug 13, 2020

Uh oh!

ulysses-you commented Aug 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ulysses-you commented Jul 26, 2020 •

edited

Loading

LantaoJin commented Jul 27, 2020 •

edited

Loading

cloud-fan commented Jul 28, 2020 •

edited

Loading

cloud-fan commented Aug 13, 2020 •

edited

Loading