HyukjinKwon commented on code in PR #59305:
URL: https://github.com/apache/spark/pull/59305#discussion_r4266608466
##########
sql/api/src/main/scala/org/apache/spark/sql/types/StructField.scala:
##########
@@ -77,7 +77,8 @@ case class StructField(
}
private[sql] def dataTypeJsonValue: JValue = {
- if (stringCollationMetadata.isEmpty &&
charVarcharCollationMetadata.isEmpty) {
+ if (stringCollationMetadata.isEmpty &&
charVarcharCollationMetadata.isEmpty &&
+ !dataType.existsRecursively(_.isInstanceOf[FileType])) {
Review Comment:
**Nit (P3):** This clause sends every field whose type contains a `FileType`
through `removeCollations`, even when the field has no collation metadata.
`removeCollations` maps any plain `StringType` it reaches through
`ArrayType`/`MapType` to the `StringType` companion, including an explicit
`STRING COLLATE UTF8_BINARY`, and `isCollatedPlainString` skips UTF8_BINARY, so
that collation is recorded neither in the type string nor in `__COLLATIONS`.
For example, `StructField("m", MapType(StringType("UTF8_BINARY"),
FileType.external))` now serializes its key as `"string"`, and after
`DataType.fromJson` the column renders as `m MAP<STRING, FILE EXTERNAL>`
instead of `m MAP<STRING COLLATE UTF8_BINARY, FILE EXTERNAL>`. The same field
with an `INT` value still takes the fast path and keeps `"string collate
UTF8_BINARY"`. So any JSON round trip, such as a schema persisted in table
properties, silently turns the explicit collation into a default-collated
string: `SHOW CREATE TABLE`/`DESCRIBE` show plain `STRING`, and DDL replayed
under a schema or table `DEFAULT COLLATION` applies that collation to the key
instead of UTF8_BINARY.
The same mapping already happens on the existing slow path when a field also
has non-UTF8_BINARY collations; this change extends it to FILE-bearing fields
with no collation metadata, where only the file mode was meant to be dropped.
Fields without a FileType and nested struct fields are unaffected.
**Verification:**
- **Regression:** A struct field of type MAP<STRING COLLATE UTF8_BINARY,
FILE EXTERNAL> serializes its key as "string collate UTF8_BINARY" and its value
as "file". After DataType.fromJson, the key is an explicit (non-companion)
UTF8_BINARY string, and toDDL still renders STRING COLLATE UTF8_BINARY.
- **Compatibility:** The existing FileType JSON forms (top-level, in-field,
array/map elements, nested structs, field metadata) and the JSON of
collated-string fields without FILE are unchanged.
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/parser/parsers.scala:
##########
@@ -499,6 +499,7 @@ object AbstractParser extends Logging {
parser.parameter_substitution_enabled =
!conf.legacyParameterSubstitutionConstantsOnly
parser.legacy_identifier_clause_only = conf.legacyIdentifierClauseOnly
parser.single_character_pipe_operator_enabled =
conf.singleCharacterPipeOperatorEnabled
+ parser.file_type_enabled = conf.isFileTypeEnabled
Review Comment:
Thanks, verified. `configureSplitterParser` now sets `file_type_enabled`
from the conf like the session parser, and the new `SqlStatementSplitterSuite`
test expects the `BEGIN ... END` block with `FILE EXTERNAL` to stay a single
statement.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:4254684259","thread_id":"inline:4254684259","verdict_sha256":"fe20246ec1d53acf9c1fbd2c1cd0c9bf38bfb18bbed290ee0f9b9b4facc6784a"}
-->
##########
sql/api/src/main/scala/org/apache/spark/sql/types/FileType.scala:
##########
@@ -0,0 +1,110 @@
+/*
+ * 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
+
+import org.apache.spark.annotation.Unstable
+
+/**
+ * The data type representing a reference to a file, with metadata describing
the file.
+ *
+ * In SQL the type is named `FILE`, and the `EXTERNAL` keyword is required
when declaring a
Review Comment:
Thanks, verified. The class doc now states the `EXTERNAL` column rule as
planned, says a bare `FILE` is accepted until then, and gives the standalone
JSON form of each mode, which matches `typeName` and the `DataTypeSuite`
assertions.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:4254684272","thread_id":"inline:4254684272","verdict_sha256":"fe20246ec1d53acf9c1fbd2c1cd0c9bf38bfb18bbed290ee0f9b9b4facc6784a"}
-->
##########
sql/api/src/main/scala/org/apache/spark/sql/types/FileType.scala:
##########
@@ -0,0 +1,110 @@
+/*
+ * 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
+
+import org.apache.spark.annotation.Unstable
+
+/**
+ * The data type representing a reference to a file, with metadata describing
the file.
+ *
+ * In SQL the type is named `FILE`, and the `EXTERNAL` keyword is required
when declaring a
+ * column, as in `CREATE TABLE t (f FILE EXTERNAL)`. Its JSON representation
is `"file"` inside
+ * a struct field and `"file external"` for a standalone type.
+ *
+ * FileType is planned to be physically stored as a struct with the following
fields (see
+ * `FileType.FIELDS`), but it deliberately does not extend [[StructType]]:
+ * - uri (STRING): the opaque URI to the file
+ * - offset (BIGINT): the start of the referenced byte range
+ * - size (BIGINT): the size of the file in bytes
+ * - content_type (STRING): the MIME/content type of the file
+ * - checksum (STRING): an algorithm-tagged integrity token for the file,
e.g. "ETAG:<etag>"
+ * - inline (BINARY): the referenced bytes stored inline in the value
+ *
+ * FileType may be EXTERNAL (simple references, lifecycle independent of the
table) or UNKNOWN
+ * (mode not yet determined, such as the result of a cast or of a UDF). Mode
is part of type
+ * identity - files with different modes are distinct under `equals`. Table
column definitions
+ * are to require `EXTERNAL`.
+ *
+ * FileType is planned to be not comparable -- it cannot be used in join
expressions, grouping
+ * expressions, or as a partition/clustering column. Users can extract the uri
field for such
+ * operations. This type currently only guarantees the schema-level pieces
(DDL and JSON); the
+ * physical representation and most of the restrictions are added by later
parts.
+ *
+ * @param mode the mode for this FileType column
+ * @since 4.4.0
+ */
+@Unstable
+case class FileType(mode: FileTypeMode = FileTypeMode.UNKNOWN) extends
DataType {
+
+ override def defaultSize: Int = FileType.asStruct.defaultSize
+
+ override def typeName: String = mode match {
+ case FileTypeMode.EXTERNAL => "file external"
+ case _ => "file"
+ }
+
+ /**
+ * `acceptsType` ignores mode so `ExpectsInputTypes.inputTypes =
Seq(FileType())` (e.g.
+ * `GetStructField`) accepts FileType(EXTERNAL). `sameType` / `equals` still
distinguish modes.
Review Comment:
Thanks, verified. The comment now states the general `acceptsType` contract
without the `GetStructField` example.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:4254684281","thread_id":"inline:4254684281","verdict_sha256":"fe20246ec1d53acf9c1fbd2c1cd0c9bf38bfb18bbed290ee0f9b9b4facc6784a"}
-->
--
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]
