Skip to content

Conversation

@ulysses-you
Copy link
Contributor

What changes were proposed in this pull request?

Discussion with 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.

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.

val columnType = getSparkSQLDataType(hc)
val metadata = if (hc.getType != columnType.catalogString) {
val metadata = if (hc.getType != columnType.catalogString &&
hc.getType != HiveVoidType.catalogString) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Avoid void type add to Metadata.

Without this, Hive void type will be put in Metadata when we load hive column.

Copy link
Contributor

Choose a reason for hiding this comment

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

what's the drawback of it?

BTW this check only works for top-level void columns, not inner fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No sure it will be, just think we do not need to use void type in other place.

}
}

test("Add HiveVoidType to compatible with Hive void type") {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add the JIRA ticket ID in the beginning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added.

override def simpleString: String = "void"
}

case object HiveVoidType extends HiveVoidType {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use one single object instead of one class + one object?

@SparkQA
Copy link

SparkQA commented Aug 13, 2020

Test build #127399 has finished for PR 29423 at commit ad890a4.

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


test("SPARK-20680: Add HiveVoidType to compatible with Hive void type") {
withView("v1") {
sql("drop view if exists v1")
Copy link
Contributor

Choose a reason for hiding this comment

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

can you check which test creates view v1 but forget to clear it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@SparkQA
Copy link

SparkQA commented Aug 13, 2020

Test build #127408 has finished for PR 29423 at commit 5150279.

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

sql("CREATE DATABASE IF NOT EXISTS db2")
sql("USE db2")
checkAnswer(spark.table("default.v1"), Row(1))
sql("DROP VIEW v1")
Copy link
Contributor Author

@ulysses-you ulysses-you Aug 13, 2020

Choose a reason for hiding this comment

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

Just a patch, the view v1 create in default db, but current db is db2.

Copy link
Contributor

Choose a reason for hiding this comment

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

can we use withView("default.v1")

@SparkQA
Copy link

SparkQA commented Aug 13, 2020

Test build #127414 has finished for PR 29423 at commit b2c152a.

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

@SparkQA
Copy link

SparkQA commented Aug 14, 2020

Test build #127427 has finished for PR 29423 at commit 57d8fd8.

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

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in 339eec5 Aug 14, 2020
@ulysses-you
Copy link
Contributor Author

thanks for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants