dejankrak-db commented on code in PR #59305:
URL: https://github.com/apache/spark/pull/59305#discussion_r4315570861
##########
sql/api/src/main/scala/org/apache/spark/sql/types/DataType.scala:
##########
@@ -133,6 +133,7 @@ object DataType {
private val STRING_WITH_COLLATION = """string\s+collate\s+(\w+)""".r
private val TIMESTAMP_LTZ_NANOS_TYPE = """timestamp_ltz\(\s*(\d+)\s*\)""".r
private val TIMESTAMP_NTZ_NANOS_TYPE = """timestamp_ntz\(\s*(\d+)\s*\)""".r
+ private val FILE_TYPE = """file(?:\s+(external|unknown))?""".r
Review Comment:
This regex is only the JSON decoder; the DDL grammar still accepts only
`FILE` and `FILE EXTERNAL`. JSON needs `"file unknown"` so that an unknown file
in a field, such as a UDF result `struct<f: FILE>`, round-trips: a bare
`"file"` in a field reads as external, following your table-vs-query split as
closely as the decoder can see it (see the next thread).
##########
sql/api/src/main/scala/org/apache/spark/sql/types/DataType.scala:
##########
@@ -267,7 +268,7 @@ object DataType {
}
/** Given the string representation of a type, return its DataType */
- private def nameToType(name: String): DataType = {
+ private def nameToType(name: String, inStructField: Boolean = false):
DataType = {
Review Comment:
`DataType.fromJson` does not know its caller: table schemas, UDF return
types, Connect plans and Parquet footers all decode through it. Being in a
struct field is the closest signal it has, since a table column is always a
field. A real table-schema context would need the catalog/Parquet/Delta call
sites to pass it in, which is a wider API change, so I will take it to the
follow-up you suggested.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DataTypeParserSuite.scala:
##########
@@ -88,6 +88,15 @@ class DataTypeParserSuite extends SparkFunSuite with
SQLHelper {
checkDataType("cHaR(27)", CharType(27))
checkDataType("BINARY", BinaryType)
checkDataType("void", NullType)
+ checkDataType("FILE EXTERNAL", FileType.external)
+ checkDataType("file external", FileType.external)
+ // A bare FILE is the type of an expression or a UDF result, not of a column
declaration.
Review Comment:
Fixed in 825ee735486: the comment now just says a bare FILE has the UNKNOWN
mode.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/StructField.scala:
##########
@@ -92,9 +94,26 @@ case class StructField(
case _ => dt
}
+ // An external file in a field is the plain "file", the form of a schema
written without a
+ // mode, so that a schema which carries other engine-specific metadata for
it is written back
+ // as it was read. Only array and map types are visited, as in
`removeCollations`.
+ def fileTypeJson(dt: DataType): JValue = dt match {
+ case at: ArrayType if at.existsRecursively(_.isInstanceOf[FileType]) =>
Review Comment:
Fixed in 825ee735486: `fileTypeJson` now recurses directly over arrays and
maps without the per-level `existsRecursively` checks. The output is unchanged,
since the array/map cases build the same JSON as
`ArrayType`/`MapType.jsonValue`; the single `hasFile` check for the early
return stays.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
