holdenk commented on code in PR #59640:
URL: https://github.com/apache/spark/pull/59640#discussion_r4461450584
##########
sql/api/src/main/scala/org/apache/spark/sql/types/FileType.scala:
##########
@@ -25,30 +25,32 @@ 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 planned to
be required when
- * declaring a table column, as in `CREATE TABLE t (f FILE EXTERNAL)`; until
then a bare `FILE` is
- * accepted wherever a type is. Its JSON representation is `"file external"`
for
- * `FileType.external` and `"file unknown"` for `FileType()`, except that an
external file in a
- * struct field is the plain `"file"`. A bare `"file"` is read as external
inside a struct field
- * and as unknown elsewhere.
+ * In SQL the type is named `FILE`. A table column must be declared with the
`EXTERNAL` keyword,
+ * as in `CREATE TABLE t (f FILE EXTERNAL)`, while the signature of a function
declares a bare
+ * `FILE`. Its JSON representation is `"file external"` for
`FileType.external` and
+ * `"file unknown"` for `FileType()`, except that an external file in a struct
field is the plain
+ * `"file"`. A bare `"file"` is read as external inside a struct field and as
unknown elsewhere.
*
* 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
+ * - uri (STRING): the URI of the referenced file, which may be absolute or
relative
* - 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}"
+ * - size (BIGINT): the length of the referenced byte range
+ * - content_type (STRING): the MIME/content type of the referenced bytes
+ * - checksum (STRING): an algorithm-tagged integrity token for the
referenced bytes, e.g.
+ * "MD5:{digest}", or "ETAG:{etag}" for the whole file
* - 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`.
*
- * 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.
+ * FileType has no defined ordering, so an operation that compares, sorts,
groups or hashes a FILE
Review Comment:
hashing and ordering are distinct.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -1170,6 +1234,13 @@ trait CheckAnalysis extends LookupCatalog with
QueryErrorsBase with PlanToString
origin = operator.origin
)
+ // FILE has no defined ordering, so it cannot be compared in a set
operation.
+ case o if fileColumnInSetOperation(o).isDefined =>
+ throw QueryCompilationErrors.unsupportedSetOperationOnFileType(
+ fileCol = fileColumnInSetOperation(o).get,
+ origin = operator.origin
+ )
+
Review Comment:
I agree we don't have ordering on filetype but do we not/ will we not have
equality + hashing? It seems like this is something we might want to support in
the future in the same way as map/variant (no ordering per-se but equality).
ALternatively maybe we should also just never support these for map/variant.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/OperatorWithUncomparableTypeValidator.scala:
##########
@@ -20,20 +20,21 @@ package org.apache.spark.sql.catalyst.analysis.resolver
import org.apache.spark.sql.catalyst.expressions.Attribute
import org.apache.spark.sql.catalyst.plans.logical.{Distinct, LogicalPlan,
SetOperation}
import org.apache.spark.sql.errors.QueryCompilationErrors
-import org.apache.spark.sql.types.{DataType, MapType, VariantType}
+import org.apache.spark.sql.types.{DataType, FileType, MapType, VariantType}
/**
* [[OperatorWithUncomparableTypeValidator]] performs the validation of a
logical plan to ensure
* that it (if it is [[Distinct]] or [[SetOperation]]) does not contain any
uncomparable types:
- * [[VariantType]] or [[MapType]].
+ * [[VariantType]], [[MapType]] or [[FileType]].
Review Comment:
VariantType is listed as maybe should support some day?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/TypeUtils.scala:
##########
@@ -70,7 +72,8 @@ object TypeUtils extends QueryErrorsBase {
}
def checkForMapKeyType(keyType: DataType): TypeCheckResult = {
- if (keyType.existsRecursively(dt => dt.isInstanceOf[MapType] ||
dt.isInstanceOf[VariantType])) {
+ if (keyType.existsRecursively(dt => dt.isInstanceOf[MapType] ||
dt.isInstanceOf[VariantType] ||
Review Comment:
For ordering could we just do like OrderedRDDFunctions.scala /
PairRDDFunctions.scala? There we say we can grab an Ordering of the item
instead of hard coding a bunch of random types as not supported?
--
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]
