HyukjinKwon commented on code in PR #59305:
URL: https://github.com/apache/spark/pull/59305#discussion_r4254684259
##########
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:
**Nit (P3):** This flag is set only for the session parser.
`SqlStatementSplitter.configureSplitterParser`
(SqlStatementSplitter.scala:450-463) builds its own `SqlBaseParser` and copies
the session grammar flags one by one, including
`single_character_pipe_operator_enabled`. That method is not updated here, so
the splitter always parses with `file_type_enabled = false`.
With `spark.sql.fileType.enabled=true`, suppose the spark-sql CLI
(`SparkSQLCLIDriver.splitStatements`) or `parse_sql` receives:
```sql
BEGIN CREATE TABLE t (f FILE EXTERNAL); SELECT 1; END;
```
The splitter tries the region up to each `;`. With the predicate false,
`FILE` parses as an unsupported type identifier and `EXTERNAL` becomes an
unexpected token. The result is `FailedNonEof`, and the splitter falls back to
splitting at every `;`. It emits `BEGIN CREATE TABLE t (f FILE EXTERNAL)`,
`SELECT 1` and `END` as separate statements, which then fail to parse. The
session parser accepts the whole block. The conf is internal and on by default
only in tests, so for now only preview users and tests hit this.
Setting `parser.file_type_enabled = conf.isFileTypeEnabled` in
`configureSplitterParser`, next to the other flags, would keep the two parsers
aligned. A splitter test for the block above would lock that in.
**Verification:**
- **Regression:** Splitting a BEGIN ... END block that contains FILE
EXTERNAL with the FILE type enabled returns a single statement instead of
fragments split at inner semicolons.
- **Compatibility:** Splitting of other inputs, and of FILE input with the
FILE type disabled, is unchanged.
##########
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:
**Nit (P3):** Two statements in this paragraph describe behavior this part
does not have yet:
- "the `EXTERNAL` keyword is required when declaring a column": with
`spark.sql.fileType.enabled=true`, `CatalystSqlParser.parseTableSchema("f
FILE")` returns a field of type `FileType()`, because `DataTypeAstBuilder` maps
a bare `FILE` to `FileType()` in every type context. The PR description defers
that check to the table DDL part, and lines 39-40 of this doc say column
definitions "are to require `EXTERNAL`", so the doc contradicts itself.
- "`"file external"` for a standalone type": only the EXTERNAL mode
serializes that way. A standalone `FileType()` is `"file"`, as DataTypeSuite
asserts.
Since this is the public doc of the new type, readers could assume bare
`FILE` columns are already rejected. Consider stating the column rule as
planned, like the layout and comparability paragraphs, and giving the
standalone JSON form for each mode.
See **Shared repair plan 1** in the review body.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DataTypeSuite.scala:
##########
@@ -1708,6 +1708,71 @@ class DataTypeSuite extends SparkFunSuite with SQLHelper
{
}
}
+ test("FileType: name, JSON and DDL round trip") {
+ // The mode is part of the type's identity.
+ assert(FileType.external != FileType())
+ assert(FileType() === FileType(FileTypeMode.UNKNOWN))
+ assert(DataTypes.FileType === FileType())
+ assert(FileType.external.typeName === "file external")
+ assert(FileType().typeName === "file")
+ // A type prints what was written: FILE EXTERNAL for an external file,
FILE for a bare FILE.
+ assert(FileType.external.sql === "FILE EXTERNAL")
+ assert(FileType().sql === "FILE")
+ assert(FileType.external.json === "\"file external\"")
+ assert(FileType().json === "\"file\"")
+ assert(DataType.fromJson("\"file external\"") === FileType.external)
+ // A bare "file" has no mode on its own, and is an external file inside a
struct field.
+ assert(DataType.fromJson("\"file\"") === FileType())
Review Comment:
**Nit (P3):** These assertions cover the top-level `"file"` and the
full-shape field cases. Three of the decoding branches added in
`DataType.scala` have no test, though, and every assertion here runs with the
flag on (the test default):
- Standalone recursion: suppose array or map recursion passed `inStructField
= true` unconditionally. `DataType.fromJson(ArrayType(FileType()).json)` would
then return `ArrayType(FileType.external)`, and every current assertion would
still pass. The same applies to `MapType`.
- Legacy field shapes: if either `parseStructField` branch without
`metadata` or `nullable` dropped `inStructField = true`, a field
`{"name":"f","type":"file"}` would read back as UNKNOWN and no test would fail.
- Flag off: the PR description says JSON parsing is not gated, so persisted
schemas stay readable, but nothing checks that. If the FILE branch of
`nameToType` started checking the flag, schemas written with the flag on would
become unreadable once it is off. This suite already guards the same contract
for the nanos types in "SPARK-57835: JSON parser reconstructs nanos timestamp
types when preview flag is off".
A few more assertions here would cover these, plus a
`withSQLConf(SQLConf.FILE_TYPE_ENABLED.key -> "false")` block that decodes a
nested FILE schema.
**Verification:**
- **Regression:** Standalone array and map JSON with a bare "file" element
decode to UNKNOWN, and legacy field shapes decode to EXTERNAL.
- **Compatibility:** FILE schemas, including nested ones, decode from JSON
with the FILE type disabled.
##########
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:
**Nit (P3):** `GetStructField` does not reach this path. Its `inputTypes` is
`Seq(StructType, IntegralType)` (complexTypeExtractors.scala), and `FileType`
deliberately does not extend `StructType`. No expression in this part declares
a FileType input type, so the example names a consumer that does not exist yet.
Someone adding field access later could read it as saying `GetStructField`
already handles FileType children. Perhaps state the general contract without
the example:
```suggestion
* `acceptsType` ignores mode, so an expression that declares `inputTypes
= Seq(FileType())`
* accepts FileType(EXTERNAL) as well. `sameType` / `equals` still
distinguish modes.
```
See **Shared repair plan 1** in the review body.
##########
sql/api/src/main/scala/org/apache/spark/sql/types/FileType.scala:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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 an external file, such as a PDF,
image, audio or
+ * video. A value holds the location of the file and a small amount of
metadata describing it
+ * (size, content type and checksum); the bytes of the file are never stored
in the value.
+ *
+ * In SQL the type is written `FILE EXTERNAL`, and its JSON representation is
`"file"`.
+ *
+ * The type is physically represented as a struct but deliberately does not
extend [[StructType]].
Review Comment:
Thanks, verified. The struct layout and the map-key, grouping and clustering
restrictions now read as planned, and the paragraph says this part only
provides the DDL and JSON pieces.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:4237227263","thread_id":"inline:4237227263","verdict_sha256":"c57df3415da184350093919a4d0cfa68ecaef3a3409a1884746fc2fbe99c2a0c"}
-->
--
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]
