Skip to content

[SPARK-20680][SQL] Adding HiveVoidType in Spark to be compatible with Hive #28935

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
LantaoJin wants to merge 7 commits into from
Closed

[SPARK-20680][SQL] Adding HiveVoidType in Spark to be compatible with Hive #28935

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
17b1853
[SPARK-20680][SQL] Adding HiveNullType in Spark to be compatible with…
LantaoJin Jun 28, 2020
ba2ef06
remove useless import
LantaoJin Jun 28, 2020
17aace2
change HiveNullType to HiveVoidType
LantaoJin Jun 28, 2020
a3a1cef
address comments
LantaoJin Jun 29, 2020
9b9d021
address comments
LantaoJin Jun 29, 2020
fa853b1
revert previous commit
LantaoJin Jun 30, 2020
3fa76cf
make sure create table with VOID fail
LantaoJin Jun 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2184,7 +2184,7 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging
* Create a Spark DataType.
*/
private def visitSparkDataType(ctx: DataTypeContext): DataType = {
HiveStringType.replaceCharType(typedVisit(ctx))
HiveVoidType.replaceVoidType(HiveStringType.replaceCharType(typedVisit(ctx)))

@cloud-fan cloud-fan Jun 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't get it why we need HiveVoidType. What happens if we just parse void to NullType?

@LantaoJin LantaoJin Jun 30, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because that could indicate VOID is a Hive type, the handle processing is more unified. Or, we can just use the PR #28833

@LantaoJin LantaoJin Jun 30, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For example, below function will point the failure is due to the legacy hive void type. If we mix VOID and NULL, I am not sure it would be better than separation.

  def failVoidType(dt: DataType): Unit = {
    if (HiveVoidType.containsVoidType(dt)) {
      throw new AnalysisException(
        "Cannot create tables with Hive VOID type.")
    }
  }

@cloud-fan cloud-fan Jun 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

VOID and NULL are indeed the same type. We can just check null type and fail with error message: Cannot create tables with VOID type

@cloud-fan cloud-fan Jun 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The point is consistency: The VOID type in SQL statement should be the same as NullType specified by Scala API in spark.catalog.createTable.

@LantaoJin LantaoJin Jul 1, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@cloud-fan , ok. I will follow your suggestion to fix it in #28833 , since this PR is a refator with new type HiveVoidType. Now we don't need it.

}

/**
Expand Down Expand Up @@ -2212,6 +2212,7 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging
case ("decimal" | "dec" | "numeric", precision :: scale :: Nil) =>
DecimalType(precision.getText.toInt, scale.getText.toInt)
case ("interval", Nil) => CalendarIntervalType
case ("void", Nil) => HiveVoidType

@cloud-fan cloud-fan Jun 29, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can just reuse NullType. We should forbid creating table with null type completely, including spark.catalog.createTable.

case (dt, params) =>
val dtStr = if (params.nonEmpty) s"$dt(${params.mkString(",")})" else dt
throw new ParseException(s"DataType $dtStr is not supported.", ctx)
Expand Down Expand Up @@ -2258,9 +2259,9 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging
builder.putString("comment", _)
}

// Add Hive type string to metadata.
// Add Hive type 'string' and 'void' to metadata.
val rawDataType = typedVisit[DataType](ctx.dataType)
val cleanedDataType = HiveStringType.replaceCharType(rawDataType)
val cleanedDataType = HiveVoidType.replaceVoidType(HiveStringType.replaceCharType(rawDataType))
if (rawDataType != cleanedDataType) {
builder.putString(HIVE_TYPE_STRING, rawDataType.catalogString)
}
Expand Down
4 changes: 1 addition & 3 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/types/HiveStringType.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,7 @@ object HiveStringType {
case MapType(kt, vt, nullable) =>
MapType(replaceCharType(kt), replaceCharType(vt), nullable)
case StructType(fields) =>
StructType(fields.map { field =>
field.copy(dataType = replaceCharType(field.dataType))
})
StructType(fields.map(f => f.copy(dataType = replaceCharType(f.dataType))))
Comment thread
LantaoJin marked this conversation as resolved.
Outdated
case _: HiveStringType => StringType
case _ => dt
}
Expand Down
45 changes: 45 additions & 0 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/types/HiveVoidType$.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.spark.sql.types

/**
* A hive void type for compatibility. These datatypes should only used for parsing,
* and should NOT be used anywhere else. Any instance of these data types should be
* replaced by a [[NullType]] before analysis.
*/
class HiveVoidType private() extends DataType {

override def defaultSize: Int = 1

override private[spark] def asNullable: HiveVoidType = this

override def simpleString: String = "void"
}

case object HiveVoidType extends HiveVoidType {
def replaceVoidType(dt: DataType): DataType = dt match {
case ArrayType(et, nullable) =>
ArrayType(replaceVoidType(et), nullable)
case MapType(kt, vt, nullable) =>
MapType(replaceVoidType(kt), replaceVoidType(vt), nullable)
case StructType(fields) =>
StructType(fields.map(f => f.copy(dataType = replaceVoidType(f.dataType))))
case _: HiveVoidType => NullType
case _ => dt
}
}
1 change: 1 addition & 0 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DataTypeParserSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ class DataTypeParserSuite extends SparkFunSuite {
checkDataType("cHaR(27)", StringType)
checkDataType("BINARY", BinaryType)
checkDataType("interval", CalendarIntervalType)
checkDataType("void", NullType)

checkDataType("array<doublE>", ArrayType(DoubleType, true))
checkDataType("Array<map<int, tinYint>>", ArrayType(MapType(IntegerType, ByteType, true), true))
Expand Down
10 changes: 6 additions & 4 deletions sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import org.apache.spark.sql.execution.command._
import org.apache.spark.sql.execution.datasources.{CreateTable, DataSource, RefreshTable}
import org.apache.spark.sql.execution.datasources.v2.FileDataSourceV2
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.types.{HIVE_TYPE_STRING, HiveStringType, MetadataBuilder, StructField, StructType}
import org.apache.spark.sql.types.{HIVE_TYPE_STRING, HiveStringType, HiveVoidType, MetadataBuilder, StructField, StructType}

/**
* Resolves catalogs from the multi-part identifiers in SQL statements, and convert the statements
Expand Down Expand Up @@ -131,8 +131,9 @@ class ResolveSessionCatalog(
s"Available: ${v1Table.schema.fieldNames.mkString(", ")}")
}
}
// Add Hive type string to metadata.
val cleanedDataType = HiveStringType.replaceCharType(dataType)
// Add Hive type 'string' and 'void' to metadata.

@cloud-fan cloud-fan Jun 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we can be more aggressive here: forbid void type in all cases, including hive tables.

val cleanedDataType =
HiveVoidType.replaceVoidType(HiveStringType.replaceCharType(dataType))
if (dataType != cleanedDataType) {
builder.putString(HIVE_TYPE_STRING, dataType.catalogString)
}
Expand Down Expand Up @@ -719,7 +720,8 @@ class ResolveSessionCatalog(
val builder = new MetadataBuilder
col.comment.foreach(builder.putString("comment", _))

val cleanedDataType = HiveStringType.replaceCharType(col.dataType)
val cleanedDataType =
HiveVoidType.replaceVoidType(HiveStringType.replaceCharType(col.dataType))
if (col.dataType != cleanedDataType) {
builder.putString(HIVE_TYPE_STRING, col.dataType.catalogString)
}
Expand Down
16 changes: 16 additions & 0 deletions sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2309,6 +2309,22 @@ class HiveDDLSuite
}
}

test("SPARK-20680: Spark-sql do not support for void column datatype of view") {
withTable("t") {
withView("tabNullType") {
Comment thread
LantaoJin marked this conversation as resolved.
Outdated
val client =
spark.sharedState.externalCatalog.unwrapped.asInstanceOf[HiveExternalCatalog].client
client.runSqlHive("CREATE TABLE t (t1 int)")
client.runSqlHive("INSERT INTO t VALUES (3)")
client.runSqlHive("CREATE VIEW tabNullType AS SELECT NULL AS col FROM t")
Comment thread
LantaoJin marked this conversation as resolved.
Outdated
checkAnswer(spark.table("tabNullType"), Row(null))
// No exception shows
val desc = spark.sql("DESC tabNullType").collect().toSeq
assert(desc.contains(Row("col", "null", null)))
}
}
}

test("SPARK-21216: join with a streaming DataFrame") {
import org.apache.spark.sql.execution.streaming.MemoryStream
import testImplicits._
Expand Down