[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
GitHub user gengliangwang opened a pull request: https://github.com/apache/spark/pull/21667 [SPARK-24691][SQL]Add new API `supportDataType` in FileFormat ## What changes were proposed in this pull request? In https://github.com/apache/spark/pull/21389, data source schema is validated before actual read/write. However, 1. Putting all the validations together in `DataSourceUtils` is tricky and hard to maintain. On second thought after review, I find that the `OrcFileFormat` in hive package is not matched, so that its validation wrong. 2. `DataSourceUtils.verifyWriteSchema` and `DataSourceUtils.verifyReadSchema` is not supposed to be called in every file format. We can move them to some upper entry. So, I propose we can add a new API `supportDataType` in FileFormat. Each file format can override the method to specify its supported/non-supported data types. ## How was this patch tested? Unit test You can merge this pull request into a Git repository by running: $ git pull https://github.com/gengliangwang/spark refactorSchemaValidate Alternatively you can review and apply these changes as the patch at: https://github.com/apache/spark/pull/21667.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #21667 commit 7fdf6033b6778d06850e6ae5a0fd6e3fde76a5c2 Author: Gengliang Wang Date: 2018-06-28T16:32:44Z refactor schema validation --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user gengliangwang commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199070174
--- Diff:
sql/hive/src/test/scala/org/apache/spark/sql/hive/orc/HiveOrcSourceSuite.scala
---
@@ -156,28 +156,6 @@ class HiveOrcSourceSuite extends OrcSuite with
TestHiveSingleton {
sql("select testType()").write.mode("overwrite").orc(orcDir)
}.getMessage
assert(msg.contains("ORC data source does not support
calendarinterval data type."))
-
- // read path
--- End diff --
In read path, ORC should support `CalendarIntervalType` and `NullType`.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199075466
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala
---
@@ -152,6 +152,16 @@ trait FileFormat {
}
}
+ /**
+ * Returns whether this format supports the given [[DataType]] in
read/write path.
+ *
+ * By default all data types are supported except
[[CalendarIntervalType]] in write path.
+ */
+ def supportDataType(dataType: DataType, isReadPath: Boolean): Boolean =
dataType match {
--- End diff --
Hm, shouldn't we better whitelist them rather then blacklist?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199076368
--- Diff:
sql/hive/src/test/scala/org/apache/spark/sql/hive/orc/HiveOrcSourceSuite.scala
---
@@ -156,28 +156,6 @@ class HiveOrcSourceSuite extends OrcSuite with
TestHiveSingleton {
sql("select testType()").write.mode("overwrite").orc(orcDir)
}.getMessage
assert(msg.contains("ORC data source does not support
calendarinterval data type."))
-
- // read path
--- End diff --
Is there any write path test already?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user gengliangwang commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199079744
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala
---
@@ -152,6 +152,16 @@ trait FileFormat {
}
}
+ /**
+ * Returns whether this format supports the given [[DataType]] in
read/write path.
+ *
+ * By default all data types are supported except
[[CalendarIntervalType]] in write path.
+ */
+ def supportDataType(dataType: DataType, isReadPath: Boolean): Boolean =
dataType match {
--- End diff --
Blacklist is easier.
With whitelist , we will have to validate
```
BooleanType | ByteType | ShortType | IntegerType | LongType | FloatType |
DoubleType |
StringType | BinaryType | DateType | TimestampType | DecimalType
```
Of course we can have a default function to process these. But if we add a
new data source which didn't support all of them, the implementation will be
verbose.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user gengliangwang commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199081671
--- Diff:
sql/hive/src/test/scala/org/apache/spark/sql/hive/orc/HiveOrcSourceSuite.scala
---
@@ -156,28 +156,6 @@ class HiveOrcSourceSuite extends OrcSuite with
TestHiveSingleton {
sql("select testType()").write.mode("overwrite").orc(orcDir)
}.getMessage
assert(msg.contains("ORC data source does not support
calendarinterval data type."))
-
- // read path
--- End diff --
@HyukjinKwon No, the unit test is about unsupported data types, and ORC
supports all data types in read path.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199094171
--- Diff:
sql/hive/src/test/scala/org/apache/spark/sql/hive/orc/HiveOrcSourceSuite.scala
---
@@ -156,28 +156,6 @@ class HiveOrcSourceSuite extends OrcSuite with
TestHiveSingleton {
sql("select testType()").write.mode("overwrite").orc(orcDir)
}.getMessage
assert(msg.contains("ORC data source does not support
calendarinterval data type."))
-
- // read path
--- End diff --
I mean the tests were negative tests. so I was expecting that we'd have
positive tests.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199095666
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala
---
@@ -152,6 +152,16 @@ trait FileFormat {
}
}
+ /**
+ * Returns whether this format supports the given [[DataType]] in
read/write path.
+ *
+ * By default all data types are supported except
[[CalendarIntervalType]] in write path.
+ */
+ def supportDataType(dataType: DataType, isReadPath: Boolean): Boolean =
dataType match {
--- End diff --
Might be easier to write but it doesn't consider if we happened to have
some more types on the other hand. It should better be explicit on what we
support on the other hand.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199096057
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala
---
@@ -152,6 +152,16 @@ trait FileFormat {
}
}
+ /**
+ * Returns whether this format supports the given [[DataType]] in
read/write path.
+ *
+ * By default all data types are supported except
[[CalendarIntervalType]] in write path.
+ */
+ def supportDataType(dataType: DataType, isReadPath: Boolean): Boolean =
dataType match {
--- End diff --
I wrote CSV's with whitelisting before per @hvanhovell's comment long time
ago. I was (am still) okay either way but might be good to leave a cc for him.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199416587
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/DataSourceScanExec.scala
---
@@ -306,6 +306,7 @@ case class FileSourceScanExec(
}
private lazy val inputRDD: RDD[InternalRow] = {
+DataSourceUtils.verifyReadSchema(relation.fileFormat,
relation.dataSchema)
--- End diff --
In some formats, is this verification applied two times, right?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199418036
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala
---
@@ -152,6 +152,16 @@ trait FileFormat {
}
}
+ /**
+ * Returns whether this format supports the given [[DataType]] in
read/write path.
+ *
+ * By default all data types are supported except
[[CalendarIntervalType]] in write path.
+ */
+ def supportDataType(dataType: DataType, isReadPath: Boolean): Boolean =
dataType match {
--- End diff --
I like the whilelist, too. As @HyukjinKwon said, if someone implements a
new type, the blacklist pass through it...
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199418986
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/json/JsonFileFormat.scala
---
@@ -30,7 +30,7 @@ import
org.apache.spark.sql.catalyst.json.{JacksonGenerator, JacksonParser, JSON
import org.apache.spark.sql.catalyst.util.CompressionCodecs
import org.apache.spark.sql.execution.datasources._
import org.apache.spark.sql.sources._
-import org.apache.spark.sql.types.{StringType, StructType}
+import org.apache.spark.sql.types._
--- End diff --
If we employ the blacklist, I think it'd be better that you don't fold
these imports.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user gengliangwang commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199494578
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala
---
@@ -152,6 +152,16 @@ trait FileFormat {
}
}
+ /**
+ * Returns whether this format supports the given [[DataType]] in
read/write path.
+ *
+ * By default all data types are supported except
[[CalendarIntervalType]] in write path.
+ */
+ def supportDataType(dataType: DataType, isReadPath: Boolean): Boolean =
dataType match {
--- End diff --
Whitelist for all file formats is behavior change. There are external file
sources like https://github.com/databricks/spark-avro , which we probably have
to update the code to make it compatible.
Currently exceptions are thrown in `buildReader` /
`buildReaderWithPartitionValues`/ `prepareWrite` for unsupported types.
So overall I prefer blacklist.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199511369
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala
---
@@ -152,6 +152,16 @@ trait FileFormat {
}
}
+ /**
+ * Returns whether this format supports the given [[DataType]] in
read/write path.
+ *
+ * By default all data types are supported except
[[CalendarIntervalType]] in write path.
+ */
+ def supportDataType(dataType: DataType, isReadPath: Boolean): Boolean =
dataType match {
--- End diff --
I meant the `case`s in the match in each implementation within Spark. I
didn't mean about the semantic about the API itself.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199515518
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala
---
@@ -152,6 +152,16 @@ trait FileFormat {
}
}
+ /**
+ * Returns whether this format supports the given [[DataType]] in
read/write path.
+ *
+ * By default all data types are supported except
[[CalendarIntervalType]] in write path.
+ */
+ def supportDataType(dataType: DataType, isReadPath: Boolean): Boolean =
dataType match {
--- End diff --
Also, do we really need this API? All what it does it is just to check the
type and throw an exception.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199515847
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceUtils.scala
---
@@ -42,63 +38,27 @@ object DataSourceUtils {
/**
* Verify if the schema is supported in datasource. This verification
should be done
- * in a driver side, e.g., `prepareWrite`, `buildReader`, and
`buildReaderWithPartitionValues`
- * in `FileFormat`.
- *
- * Unsupported data types of csv, json, orc, and parquet are as follows;
- * csv -> R/W: Interval, Null, Array, Map, Struct
- * json -> W: Interval
- * orc -> W: Interval, Null
- * parquet -> R/W: Interval, Null
+ * in a driver side.
*/
private def verifySchema(format: FileFormat, schema: StructType,
isReadPath: Boolean): Unit = {
-def throwUnsupportedException(dataType: DataType): Unit = {
- throw new UnsupportedOperationException(
-s"$format data source does not support ${dataType.simpleString}
data type.")
-}
-
-def verifyType(dataType: DataType): Unit = dataType match {
- case BooleanType | ByteType | ShortType | IntegerType | LongType |
FloatType | DoubleType |
- StringType | BinaryType | DateType | TimestampType | _:
DecimalType =>
-
- // All the unsupported types for CSV
- case _: NullType | _: CalendarIntervalType | _: StructType | _:
ArrayType | _: MapType
- if format.isInstanceOf[CSVFileFormat] =>
-throwUnsupportedException(dataType)
-
- case st: StructType => st.foreach { f => verifyType(f.dataType) }
-
- case ArrayType(elementType, _) => verifyType(elementType)
-
- case MapType(keyType, valueType, _) =>
-verifyType(keyType)
-verifyType(valueType)
-
- case udt: UserDefinedType[_] => verifyType(udt.sqlType)
-
- // Interval type not supported in all the write path
- case _: CalendarIntervalType if !isReadPath =>
-throwUnsupportedException(dataType)
-
- // JSON and ORC don't support an Interval type, but we pass it in
read pass
- // for back-compatibility.
- case _: CalendarIntervalType if format.isInstanceOf[JsonFileFormat]
||
-format.isInstanceOf[OrcFileFormat] =>
+def verifyType(dataType: DataType): Unit = {
+ if (!format.supportDataType(dataType, isReadPath)) {
+throw new UnsupportedOperationException(
+ s"$format data source does not support ${dataType.simpleString}
data type.")
+ }
+ dataType match {
--- End diff --
Wait .. why we do the recursive thing here? What if the top level type is
supported but nested is not? For example, Arrow doesn't currently support
nested timestamp conversion for localization issue.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user gengliangwang commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199519380
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala
---
@@ -152,6 +152,16 @@ trait FileFormat {
}
}
+ /**
+ * Returns whether this format supports the given [[DataType]] in
read/write path.
+ *
+ * By default all data types are supported except
[[CalendarIntervalType]] in write path.
+ */
+ def supportDataType(dataType: DataType, isReadPath: Boolean): Boolean =
dataType match {
--- End diff --
@HyukjinKwon Sorry I don't understand. Do you mean the default case is not
supported?
```
case _ => false
```
But how to make all the external formats work?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199523308
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala
---
@@ -152,6 +152,16 @@ trait FileFormat {
}
}
+ /**
+ * Returns whether this format supports the given [[DataType]] in
read/write path.
+ *
+ * By default all data types are supported except
[[CalendarIntervalType]] in write path.
+ */
+ def supportDataType(dataType: DataType, isReadPath: Boolean): Boolean =
dataType match {
--- End diff --
OK. I meant, leaving the default case `true`
```scala
def supportDataType(...): Boolean = dataType match {
case _ => true
}
```
and whitelist each type within each implementation, for example, in
`CSVFileFormat.scala`
```scala
def supportDataType(...) ...
case _: StringType | ... => true
case _ => false
```
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user gengliangwang commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199524768
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceUtils.scala
---
@@ -42,63 +38,27 @@ object DataSourceUtils {
/**
* Verify if the schema is supported in datasource. This verification
should be done
- * in a driver side, e.g., `prepareWrite`, `buildReader`, and
`buildReaderWithPartitionValues`
- * in `FileFormat`.
- *
- * Unsupported data types of csv, json, orc, and parquet are as follows;
- * csv -> R/W: Interval, Null, Array, Map, Struct
- * json -> W: Interval
- * orc -> W: Interval, Null
- * parquet -> R/W: Interval, Null
+ * in a driver side.
*/
private def verifySchema(format: FileFormat, schema: StructType,
isReadPath: Boolean): Unit = {
-def throwUnsupportedException(dataType: DataType): Unit = {
- throw new UnsupportedOperationException(
-s"$format data source does not support ${dataType.simpleString}
data type.")
-}
-
-def verifyType(dataType: DataType): Unit = dataType match {
- case BooleanType | ByteType | ShortType | IntegerType | LongType |
FloatType | DoubleType |
- StringType | BinaryType | DateType | TimestampType | _:
DecimalType =>
-
- // All the unsupported types for CSV
- case _: NullType | _: CalendarIntervalType | _: StructType | _:
ArrayType | _: MapType
- if format.isInstanceOf[CSVFileFormat] =>
-throwUnsupportedException(dataType)
-
- case st: StructType => st.foreach { f => verifyType(f.dataType) }
-
- case ArrayType(elementType, _) => verifyType(elementType)
-
- case MapType(keyType, valueType, _) =>
-verifyType(keyType)
-verifyType(valueType)
-
- case udt: UserDefinedType[_] => verifyType(udt.sqlType)
-
- // Interval type not supported in all the write path
- case _: CalendarIntervalType if !isReadPath =>
-throwUnsupportedException(dataType)
-
- // JSON and ORC don't support an Interval type, but we pass it in
read pass
- // for back-compatibility.
- case _: CalendarIntervalType if format.isInstanceOf[JsonFileFormat]
||
-format.isInstanceOf[OrcFileFormat] =>
+def verifyType(dataType: DataType): Unit = {
+ if (!format.supportDataType(dataType, isReadPath)) {
+throw new UnsupportedOperationException(
+ s"$format data source does not support ${dataType.simpleString}
data type.")
+ }
+ dataType match {
--- End diff --
This is for general purpose, so that developer can skip matching
arrays/maps/structs.
I don't know about nested timestamp, but we can override supportDataType to
make sure the case is unsupported, right?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199525496
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceUtils.scala
---
@@ -42,63 +38,27 @@ object DataSourceUtils {
/**
* Verify if the schema is supported in datasource. This verification
should be done
- * in a driver side, e.g., `prepareWrite`, `buildReader`, and
`buildReaderWithPartitionValues`
- * in `FileFormat`.
- *
- * Unsupported data types of csv, json, orc, and parquet are as follows;
- * csv -> R/W: Interval, Null, Array, Map, Struct
- * json -> W: Interval
- * orc -> W: Interval, Null
- * parquet -> R/W: Interval, Null
+ * in a driver side.
*/
private def verifySchema(format: FileFormat, schema: StructType,
isReadPath: Boolean): Unit = {
-def throwUnsupportedException(dataType: DataType): Unit = {
- throw new UnsupportedOperationException(
-s"$format data source does not support ${dataType.simpleString}
data type.")
-}
-
-def verifyType(dataType: DataType): Unit = dataType match {
- case BooleanType | ByteType | ShortType | IntegerType | LongType |
FloatType | DoubleType |
- StringType | BinaryType | DateType | TimestampType | _:
DecimalType =>
-
- // All the unsupported types for CSV
- case _: NullType | _: CalendarIntervalType | _: StructType | _:
ArrayType | _: MapType
- if format.isInstanceOf[CSVFileFormat] =>
-throwUnsupportedException(dataType)
-
- case st: StructType => st.foreach { f => verifyType(f.dataType) }
-
- case ArrayType(elementType, _) => verifyType(elementType)
-
- case MapType(keyType, valueType, _) =>
-verifyType(keyType)
-verifyType(valueType)
-
- case udt: UserDefinedType[_] => verifyType(udt.sqlType)
-
- // Interval type not supported in all the write path
- case _: CalendarIntervalType if !isReadPath =>
-throwUnsupportedException(dataType)
-
- // JSON and ORC don't support an Interval type, but we pass it in
read pass
- // for back-compatibility.
- case _: CalendarIntervalType if format.isInstanceOf[JsonFileFormat]
||
-format.isInstanceOf[OrcFileFormat] =>
+def verifyType(dataType: DataType): Unit = {
+ if (!format.supportDataType(dataType, isReadPath)) {
+throw new UnsupportedOperationException(
+ s"$format data source does not support ${dataType.simpleString}
data type.")
+ }
+ dataType match {
--- End diff --
Yea.. then this code bit is not really general purpose anymore ...
developers should check the codes inside and see if the nested types are
automatically checked or not ..
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199526695
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala
---
@@ -152,6 +152,16 @@ trait FileFormat {
}
}
+ /**
+ * Returns whether this format supports the given [[DataType]] in
read/write path.
+ *
+ * By default all data types are supported except
[[CalendarIntervalType]] in write path.
--- End diff --
FYI, `CalendarIntervalType` isn't completely public yet .. cc @cloud-fan.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user gengliangwang commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199529548
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceUtils.scala
---
@@ -42,63 +38,27 @@ object DataSourceUtils {
/**
* Verify if the schema is supported in datasource. This verification
should be done
- * in a driver side, e.g., `prepareWrite`, `buildReader`, and
`buildReaderWithPartitionValues`
- * in `FileFormat`.
- *
- * Unsupported data types of csv, json, orc, and parquet are as follows;
- * csv -> R/W: Interval, Null, Array, Map, Struct
- * json -> W: Interval
- * orc -> W: Interval, Null
- * parquet -> R/W: Interval, Null
+ * in a driver side.
*/
private def verifySchema(format: FileFormat, schema: StructType,
isReadPath: Boolean): Unit = {
-def throwUnsupportedException(dataType: DataType): Unit = {
- throw new UnsupportedOperationException(
-s"$format data source does not support ${dataType.simpleString}
data type.")
-}
-
-def verifyType(dataType: DataType): Unit = dataType match {
- case BooleanType | ByteType | ShortType | IntegerType | LongType |
FloatType | DoubleType |
- StringType | BinaryType | DateType | TimestampType | _:
DecimalType =>
-
- // All the unsupported types for CSV
- case _: NullType | _: CalendarIntervalType | _: StructType | _:
ArrayType | _: MapType
- if format.isInstanceOf[CSVFileFormat] =>
-throwUnsupportedException(dataType)
-
- case st: StructType => st.foreach { f => verifyType(f.dataType) }
-
- case ArrayType(elementType, _) => verifyType(elementType)
-
- case MapType(keyType, valueType, _) =>
-verifyType(keyType)
-verifyType(valueType)
-
- case udt: UserDefinedType[_] => verifyType(udt.sqlType)
-
- // Interval type not supported in all the write path
- case _: CalendarIntervalType if !isReadPath =>
-throwUnsupportedException(dataType)
-
- // JSON and ORC don't support an Interval type, but we pass it in
read pass
- // for back-compatibility.
- case _: CalendarIntervalType if format.isInstanceOf[JsonFileFormat]
||
-format.isInstanceOf[OrcFileFormat] =>
+def verifyType(dataType: DataType): Unit = {
+ if (!format.supportDataType(dataType, isReadPath)) {
+throw new UnsupportedOperationException(
+ s"$format data source does not support ${dataType.simpleString}
data type.")
+ }
+ dataType match {
--- End diff --
I know..But if developers didn't read inside and process the case of
arrays/maps/structs, the code should still work.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199531608
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceUtils.scala
---
@@ -42,63 +38,27 @@ object DataSourceUtils {
/**
* Verify if the schema is supported in datasource. This verification
should be done
- * in a driver side, e.g., `prepareWrite`, `buildReader`, and
`buildReaderWithPartitionValues`
- * in `FileFormat`.
- *
- * Unsupported data types of csv, json, orc, and parquet are as follows;
- * csv -> R/W: Interval, Null, Array, Map, Struct
- * json -> W: Interval
- * orc -> W: Interval, Null
- * parquet -> R/W: Interval, Null
+ * in a driver side.
*/
private def verifySchema(format: FileFormat, schema: StructType,
isReadPath: Boolean): Unit = {
-def throwUnsupportedException(dataType: DataType): Unit = {
- throw new UnsupportedOperationException(
-s"$format data source does not support ${dataType.simpleString}
data type.")
-}
-
-def verifyType(dataType: DataType): Unit = dataType match {
- case BooleanType | ByteType | ShortType | IntegerType | LongType |
FloatType | DoubleType |
- StringType | BinaryType | DateType | TimestampType | _:
DecimalType =>
-
- // All the unsupported types for CSV
- case _: NullType | _: CalendarIntervalType | _: StructType | _:
ArrayType | _: MapType
- if format.isInstanceOf[CSVFileFormat] =>
-throwUnsupportedException(dataType)
-
- case st: StructType => st.foreach { f => verifyType(f.dataType) }
-
- case ArrayType(elementType, _) => verifyType(elementType)
-
- case MapType(keyType, valueType, _) =>
-verifyType(keyType)
-verifyType(valueType)
-
- case udt: UserDefinedType[_] => verifyType(udt.sqlType)
-
- // Interval type not supported in all the write path
- case _: CalendarIntervalType if !isReadPath =>
-throwUnsupportedException(dataType)
-
- // JSON and ORC don't support an Interval type, but we pass it in
read pass
- // for back-compatibility.
- case _: CalendarIntervalType if format.isInstanceOf[JsonFileFormat]
||
-format.isInstanceOf[OrcFileFormat] =>
+def verifyType(dataType: DataType): Unit = {
+ if (!format.supportDataType(dataType, isReadPath)) {
+throw new UnsupportedOperationException(
+ s"$format data source does not support ${dataType.simpleString}
data type.")
+ }
+ dataType match {
--- End diff --
In that case, this code bit becomes rather obsolete .. To me Spark's dev
API is too difficult for me to understand :-) .. Personally, I don't like to be
too clever when it comes to API thing.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199694805
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceUtils.scala
---
@@ -42,63 +38,27 @@ object DataSourceUtils {
/**
* Verify if the schema is supported in datasource. This verification
should be done
- * in a driver side, e.g., `prepareWrite`, `buildReader`, and
`buildReaderWithPartitionValues`
- * in `FileFormat`.
- *
- * Unsupported data types of csv, json, orc, and parquet are as follows;
- * csv -> R/W: Interval, Null, Array, Map, Struct
- * json -> W: Interval
- * orc -> W: Interval, Null
- * parquet -> R/W: Interval, Null
+ * in a driver side.
*/
private def verifySchema(format: FileFormat, schema: StructType,
isReadPath: Boolean): Unit = {
-def throwUnsupportedException(dataType: DataType): Unit = {
- throw new UnsupportedOperationException(
-s"$format data source does not support ${dataType.simpleString}
data type.")
-}
-
-def verifyType(dataType: DataType): Unit = dataType match {
- case BooleanType | ByteType | ShortType | IntegerType | LongType |
FloatType | DoubleType |
- StringType | BinaryType | DateType | TimestampType | _:
DecimalType =>
-
- // All the unsupported types for CSV
- case _: NullType | _: CalendarIntervalType | _: StructType | _:
ArrayType | _: MapType
- if format.isInstanceOf[CSVFileFormat] =>
-throwUnsupportedException(dataType)
-
- case st: StructType => st.foreach { f => verifyType(f.dataType) }
-
- case ArrayType(elementType, _) => verifyType(elementType)
-
- case MapType(keyType, valueType, _) =>
-verifyType(keyType)
-verifyType(valueType)
-
- case udt: UserDefinedType[_] => verifyType(udt.sqlType)
-
- // Interval type not supported in all the write path
- case _: CalendarIntervalType if !isReadPath =>
-throwUnsupportedException(dataType)
-
- // JSON and ORC don't support an Interval type, but we pass it in
read pass
- // for back-compatibility.
- case _: CalendarIntervalType if format.isInstanceOf[JsonFileFormat]
||
-format.isInstanceOf[OrcFileFormat] =>
+def verifyType(dataType: DataType): Unit = {
+ if (!format.supportDataType(dataType, isReadPath)) {
+throw new UnsupportedOperationException(
+ s"$format data source does not support ${dataType.simpleString}
data type.")
+ }
+ dataType match {
--- End diff --
It's tricky to rely on 2 places to correctly determine the unsupported
type. `format.supportDataType` should handle complex types themselves, to make
the code clearer and easier to maintain.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199695118
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala
---
@@ -152,6 +152,16 @@ trait FileFormat {
}
}
+ /**
+ * Returns whether this format supports the given [[DataType]] in
read/write path.
+ *
+ * By default all data types are supported except
[[CalendarIntervalType]] in write path.
--- End diff --
yea, it's not by default, we can't write out interval type. This check is
in `DataSource.planForWriting`
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199695311
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala
---
@@ -152,6 +152,16 @@ trait FileFormat {
}
}
+ /**
+ * Returns whether this format supports the given [[DataType]] in
read/write path.
+ *
+ * By default all data types are supported except
[[CalendarIntervalType]] in write path.
+ */
+ def supportDataType(dataType: DataType, isReadPath: Boolean): Boolean =
dataType match {
--- End diff --
blacklist is easier, but whitelist is safer.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21667: [SPARK-24691][SQL]Add new API `supportDataType` i...
Github user gengliangwang commented on a diff in the pull request:
https://github.com/apache/spark/pull/21667#discussion_r199705148
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceUtils.scala
---
@@ -42,63 +38,27 @@ object DataSourceUtils {
/**
* Verify if the schema is supported in datasource. This verification
should be done
- * in a driver side, e.g., `prepareWrite`, `buildReader`, and
`buildReaderWithPartitionValues`
- * in `FileFormat`.
- *
- * Unsupported data types of csv, json, orc, and parquet are as follows;
- * csv -> R/W: Interval, Null, Array, Map, Struct
- * json -> W: Interval
- * orc -> W: Interval, Null
- * parquet -> R/W: Interval, Null
+ * in a driver side.
*/
private def verifySchema(format: FileFormat, schema: StructType,
isReadPath: Boolean): Unit = {
-def throwUnsupportedException(dataType: DataType): Unit = {
- throw new UnsupportedOperationException(
-s"$format data source does not support ${dataType.simpleString}
data type.")
-}
-
-def verifyType(dataType: DataType): Unit = dataType match {
- case BooleanType | ByteType | ShortType | IntegerType | LongType |
FloatType | DoubleType |
- StringType | BinaryType | DateType | TimestampType | _:
DecimalType =>
-
- // All the unsupported types for CSV
- case _: NullType | _: CalendarIntervalType | _: StructType | _:
ArrayType | _: MapType
- if format.isInstanceOf[CSVFileFormat] =>
-throwUnsupportedException(dataType)
-
- case st: StructType => st.foreach { f => verifyType(f.dataType) }
-
- case ArrayType(elementType, _) => verifyType(elementType)
-
- case MapType(keyType, valueType, _) =>
-verifyType(keyType)
-verifyType(valueType)
-
- case udt: UserDefinedType[_] => verifyType(udt.sqlType)
-
- // Interval type not supported in all the write path
- case _: CalendarIntervalType if !isReadPath =>
-throwUnsupportedException(dataType)
-
- // JSON and ORC don't support an Interval type, but we pass it in
read pass
- // for back-compatibility.
- case _: CalendarIntervalType if format.isInstanceOf[JsonFileFormat]
||
-format.isInstanceOf[OrcFileFormat] =>
+def verifyType(dataType: DataType): Unit = {
+ if (!format.supportDataType(dataType, isReadPath)) {
+throw new UnsupportedOperationException(
+ s"$format data source does not support ${dataType.simpleString}
data type.")
+ }
+ dataType match {
--- End diff --
I see. I will update it.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
