You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org> on 2023/04/20 17:58:31 UTC
[GitHub] [spark] ryan-johnson-databricks opened a new pull request, #40885: [WIP][SPARK-XXXXX] Define extractors for file-constant metadata
ryan-johnson-databricks opened a new pull request, #40885:
URL: https://github.com/apache/spark/pull/40885
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
File-source constant metadata columns are often derived indirectly from file-level metadata values rather than exposing those values directly. Add support for metadata extractors, so that we can express such columns in a generic way.
### Why are the changes needed?
Allows to express the existing file-source metadata columns in a generic way (previously hard-wired), and also allows to lazily derive expensive metadata values only if the query actually selects them.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
New unit test. Plus, existing file-source metadata unit tests pass.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1175286323
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * By default, the value of a file-constant metadata column is obtained by looking up the column's
+ * name in the file's metadata column value map. However, implementations can override this method
+ * in order to provide an extractor that has access to the entire [[PartitionedFile]] when
+ * deriving the column's value.
+ *
+ * NOTE: Extractors are lazy, invoked only if the query actually selects their column at runtime.
+ *
+ * See also [[FileFormat.getFileConstantMetadataColumnValue]].
+ */
+ def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any] =
Review Comment:
Today's API is two pieces:
```scala
/** All fields of the _metadata struct (in declared order) */
def metadataSchemaFields: Seq[StructField]
/** Custom extractors for the subset of file-constant metadata columns that actually need them */
def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any]
```
Splitting the API would produce instead (at a minimum):
```scala
/** Extractor for file-constant metadata columns that just pull the value from the metadata map */
val DEFAULT_EXTRACTOR: PartitionedFile => Any
/** Defines the file-constant fields of the _metadata struct */
def constantMetadataCols: Seq[(StructField, PartitionedFile => Any)]
/** Defines the file-generated fields of the _metadata struct */
def generatedMetadataCols: Seq[StructField]
```
The only safety issue I thought of so far, is that it might be possible to get the name mapping wrong between columns and their extractors. But if that happened even the most basic unit test for the new column would fail. Meanwhile, name mapping is anyway needed internally because the list of metadata columns we have to work with is named attributes or even just column names extracted from the query plan itself. Thus, the pairing proposed above would anyway have to be reconstructed.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1175449282
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * By default, the value of a file-constant metadata column is obtained by looking up the column's
+ * name in the file's metadata column value map. However, implementations can override this method
+ * in order to provide an extractor that has access to the entire [[PartitionedFile]] when
+ * deriving the column's value.
+ *
+ * NOTE: Extractors are lazy, invoked only if the query actually selects their column at runtime.
+ *
+ * See also [[FileFormat.getFileConstantMetadataColumnValue]].
+ */
+ def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any] =
Review Comment:
> Basically, we get the value of a constant metadata column from two maps: one map is filled by FileIndex and contains the value directly. The other map is filled by the FileFormat implementation and contains extractors.
Nit: The extractors don't "contain" the value at all. They just tell how to extract a value from the `PartitionedFile` that the `FileIndex` provides. I don't know a cleaner way to do this, given that the extraction policy should probably be consistent across all files, rather than re-defined by each file? (prior to this PR, the "policy" was hard-wired as "look it up in the map"; now it's allowed to also be "do arbitrary computation on `PartitionedFile`, if the user prefers)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1175418393
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * By default, the value of a file-constant metadata column is obtained by looking up the column's
+ * name in the file's metadata column value map. However, implementations can override this method
+ * in order to provide an extractor that has access to the entire [[PartitionedFile]] when
+ * deriving the column's value.
+ *
+ * NOTE: Extractors are lazy, invoked only if the query actually selects their column at runtime.
+ *
+ * See also [[FileFormat.getFileConstantMetadataColumnValue]].
+ */
+ def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any] =
Review Comment:
My preference is to have a simple and clean framework to generate constant metadata columns. I think the framework is a bit complicated right now:
1. FileFormat implementations can define a few constant metadata columns (name and data type).
2. The constant metadata columns should either match a built-in name in `FileScanRDD`, or be filled by a custom `FileIndex`. With this PR, there is no built-in name. It's either filled by `FileIndex` or has an extractor.
3. With this PR, FileFormat implementations can provide extractors for certain constant metadata columns.
Basically, we get the value of a constant metadata column from two maps: one map is filled by `FileIndex` and contains the value directly. The other map is filled by the `FileFormat` implementation and contains extractors.
It will be great if we can replace the first map with the second one. But if we can't, I'm fine with the current API as we can't avoid name lookup anyway. Can we add more comments in the new API, saying that it may only handle part of the constant metadata columns? The rest of them should be handled by a custom `FileIndex`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1175286323
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * By default, the value of a file-constant metadata column is obtained by looking up the column's
+ * name in the file's metadata column value map. However, implementations can override this method
+ * in order to provide an extractor that has access to the entire [[PartitionedFile]] when
+ * deriving the column's value.
+ *
+ * NOTE: Extractors are lazy, invoked only if the query actually selects their column at runtime.
+ *
+ * See also [[FileFormat.getFileConstantMetadataColumnValue]].
+ */
+ def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any] =
Review Comment:
Today's API is two pieces:
```scala
/** All fields of the _metadata struct (in declared order) */
def metadataSchemaFields: Seq[StructField]
/** Custom extractors for the subset of file-constant metadata columns that actually need them */
def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any]
```
Splitting the API would produce instead (at a minimum):
```scala
/** Extractor for file-constant metadata columns whose value comes from the metadata map */
val DEFAULT_EXTRACTOR: PartitionedFile => Any
/** File-constant fields of the _metadata struct */
def constantMetadataCols: Seq[(StructField, PartitionedFile => Any)]
/** File-generated fields of the _metadata struct */
def generatedMetadataCols: Seq[StructField]
```
The only safety issue I thought of so far, is that it might be possible to get the name mapping wrong between columns and their extractors. But if that happened even the most basic unit test for the new column would fail. Meanwhile, name mapping is anyway needed internally because the list of metadata columns we have to work with is named attributes or even just column names extracted from the query plan itself. Thus, the pairing proposed above would anyway have to be reconstructed.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1173759666
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -241,47 +256,74 @@ object FileFormat {
FileSourceConstantMetadataStructField(FILE_BLOCK_LENGTH, LongType, nullable = false),
FileSourceConstantMetadataStructField(FILE_MODIFICATION_TIME, TimestampType, nullable = false))
+ /**
+ * All [[BASE_METADATA_FIELDS]] require custom extractors because they are derived directly from
+ * fields of the [[PartitionedFile]], and do have entries in the file's metadata map.
+ */
+ val BASE_METADATA_EXTRACTORS: Map[String, PartitionedFile => Any] = Map(
+ FILE_PATH -> { pf: PartitionedFile => pf.toPath.toString },
+ FILE_NAME -> { pf: PartitionedFile => pf.toPath.getName },
+ FILE_SIZE -> { pf: PartitionedFile => pf.fileSize },
+ FILE_BLOCK_START -> { pf: PartitionedFile => pf.start },
+ FILE_BLOCK_LENGTH -> { pf: PartitionedFile => pf.length },
+ // The modificationTime from the file has millisecond granularity, but the TimestampType for
+ // `file_modification_time` has microsecond granularity.
+ FILE_MODIFICATION_TIME -> { pf: PartitionedFile => pf.modificationTime * 1000 }
+ )
+
+ /**
+ * Extracts the [[Literal]] value of a file-constant metadata column from a [[PartitionedFile]].
+ *
+ * If an extractor is available, use it. Otherwise, attempt to fetch the value directly from the
+ * file's metadata map, returning null if not found.
+ *
+ * Raw values (including null) are automatically converted to literals as a courtesy.
+ */
+ def getFileConstantMetadataColumnValue(
+ name: String,
+ file: PartitionedFile,
+ metadataExtractors: Map[String, PartitionedFile => Any]): Literal = {
+ val extractor = metadataExtractors.get(name).getOrElse {
+ (_: PartitionedFile).otherConstantMetadataColumnValues.get(name).orNull
Review Comment:
Agreed. I think it's already fixed locally but I didn't push yet in case there were other comments.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1175286323
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * By default, the value of a file-constant metadata column is obtained by looking up the column's
+ * name in the file's metadata column value map. However, implementations can override this method
+ * in order to provide an extractor that has access to the entire [[PartitionedFile]] when
+ * deriving the column's value.
+ *
+ * NOTE: Extractors are lazy, invoked only if the query actually selects their column at runtime.
+ *
+ * See also [[FileFormat.getFileConstantMetadataColumnValue]].
+ */
+ def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any] =
Review Comment:
Today's API is two pieces:
```scala
/** All fields of the _metadata struct */
def metadataSchemaFields: Seq[StructField]
/** Custom extractors for the subset of file-constant metadata columns that actually need them */
def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any]
```
Splitting the API would produce instead (at a minimum):
```scala
/** Extractor for file-constant metadata columns that just pull the value from the metadata map */
val DEFAULT_EXTRACTOR: PartitionedFile => Any
/** Defines the file-constant fields of the _metadata struct */
def constantMetadataCols: Seq[(StructField, PartitionedFile => Any)]
/** Defines the file-generated fields of the _metadata struct */
def generatedMetadataCols: Seq[StructField]
```
The only safety issue I thought of so far, is that it might be possible to get the name mapping wrong between columns and their extractors. But if that happened even the most basic unit test for the new column would fail. Meanwhile, name mapping is anyway needed internally because the list of metadata columns we have to work with is named attributes or even just column names extracted from the query plan itself. Thus, the pairing proposed above would anyway have to be reconstructed.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1175623971
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * By default, the value of a file-constant metadata column is obtained by looking up the column's
+ * name in the file's metadata column value map. However, implementations can override this method
+ * in order to provide an extractor that has access to the entire [[PartitionedFile]] when
+ * deriving the column's value.
+ *
+ * NOTE: Extractors are lazy, invoked only if the query actually selects their column at runtime.
+ *
+ * See also [[FileFormat.getFileConstantMetadataColumnValue]].
+ */
+ def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any] =
Review Comment:
doc comments updated, PTAL
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1173799650
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * A scanner must derive each file-constant metadata field's value from each [[PartitionedFile]]
+ * it processes. By default, the value is obtained by a direct lookup of the column's name on
+ * [[PartitionedFile.otherConstantMetadataColumnValues]] (see
+ * [[FileFormat.getFileConstantMetadataColumnValue]]). However, implementations can override this
+ * method in order to provide more sophisticated lazy extractors (e.g. in case the column value is
+ * complicated or expensive to compute).
Review Comment:
Reworded, PTAL
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1175286323
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * By default, the value of a file-constant metadata column is obtained by looking up the column's
+ * name in the file's metadata column value map. However, implementations can override this method
+ * in order to provide an extractor that has access to the entire [[PartitionedFile]] when
+ * deriving the column's value.
+ *
+ * NOTE: Extractors are lazy, invoked only if the query actually selects their column at runtime.
+ *
+ * See also [[FileFormat.getFileConstantMetadataColumnValue]].
+ */
+ def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any] =
Review Comment:
Today's API is two pieces:
```scala
/** All fields of the _metadata struct (in declared order) */
def metadataSchemaFields: Seq[StructField]
/** Custom extractors for the subset of file-constant metadata columns that actually need them */
def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any]
```
Splitting the API would produce instead (at a minimum):
```scala
/** Extractor for file-constant metadata columns that just pull the value from the metadata map */
val DEFAULT_EXTRACTOR: PartitionedFile => Any
/** File-constant fields of the _metadata struct */
def constantMetadataCols: Seq[(StructField, PartitionedFile => Any)]
/** File-generated fields of the _metadata struct */
def generatedMetadataCols: Seq[StructField]
```
The only safety issue I thought of so far, is that it might be possible to get the name mapping wrong between columns and their extractors. But if that happened even the most basic unit test for the new column would fail. Meanwhile, name mapping is anyway needed internally because the list of metadata columns we have to work with is named attributes or even just column names extracted from the query plan itself. Thus, the pairing proposed above would anyway have to be reconstructed.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1175452917
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * By default, the value of a file-constant metadata column is obtained by looking up the column's
+ * name in the file's metadata column value map. However, implementations can override this method
+ * in order to provide an extractor that has access to the entire [[PartitionedFile]] when
+ * deriving the column's value.
+ *
+ * NOTE: Extractors are lazy, invoked only if the query actually selects their column at runtime.
+ *
+ * See also [[FileFormat.getFileConstantMetadataColumnValue]].
+ */
+ def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any] =
Review Comment:
> Can we add more comments in the new API, saying that it may only handle part of the constant metadata columns? The rest of them should be handled by a custom FileIndex.
There was already one comment to that effect in the PR that added custom metadata column support:
https://github.com/apache/spark/pull/40677/files#diff-2d7ec6604810e1be58ca7852ec389853c143b684894f2390ffe07f96701d2219R29
And the unit tests also are a pretty clear demonstration.
Good point that the comments are buried pretty deep tho -- I'll try to add/expand them in FileFormat.scala as well.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1174277575
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * By default, the value of a file-constant metadata column is obtained by looking up the column's
+ * name in the file's metadata column value map. However, implementations can override this method
+ * in order to provide an extractor that has access to the entire [[PartitionedFile]] when
+ * deriving the column's value.
+ *
+ * NOTE: Extractors are lazy, invoked only if the query actually selects their column at runtime.
+ *
+ * See also [[FileFormat.getFileConstantMetadataColumnValue]].
+ */
+ def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any] =
Review Comment:
I have 2 questions about the API design:
1. File source implementations now need to define constant metadata columns in both `metadataSchemaFields` and `fileConstantMetadataExtractors`. Shall we have a single API to define constant metadata columns? The API should define both column name, type, and extractor.
2. Should the extractor return a `Literal` instead of `Any`?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1176151834
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -241,47 +260,74 @@ object FileFormat {
FileSourceConstantMetadataStructField(FILE_BLOCK_LENGTH, LongType, nullable = false),
FileSourceConstantMetadataStructField(FILE_MODIFICATION_TIME, TimestampType, nullable = false))
+ /**
+ * All [[BASE_METADATA_FIELDS]] require custom extractors because they are derived directly from
+ * fields of the [[PartitionedFile]], and do have entries in the file's metadata map.
Review Comment:
```suggestion
* fields of the [[PartitionedFile]], and don't have entries in the file's metadata map.
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #40885:
URL: https://github.com/apache/spark/pull/40885#issuecomment-1521356890
thanks, merging to master!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1175364533
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * By default, the value of a file-constant metadata column is obtained by looking up the column's
+ * name in the file's metadata column value map. However, implementations can override this method
+ * in order to provide an extractor that has access to the entire [[PartitionedFile]] when
+ * deriving the column's value.
+ *
+ * NOTE: Extractors are lazy, invoked only if the query actually selects their column at runtime.
+ *
+ * See also [[FileFormat.getFileConstantMetadataColumnValue]].
+ */
+ def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any] =
Review Comment:
I just realized a potential difference of underlying assumptions that could be making this discussion more difficult:
The reviewer comments so far are focused on what the changes in this PR look like. However, this PR isn't representative of the experience somebody would have while trying to define custom metadata columns for their file format. By definition all of the standard columns `FileFormat` natively provides are _NOT_ custom and come directly from the fields of `PartitionedFile` itself, rather than indirectly from the metadata values map which users have control over.
My thought process for designing the API was more focused on ease of use for the former case (custom file formats), since the latter (base file format) is a one-time effort within in this PR.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1173755343
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -241,47 +256,74 @@ object FileFormat {
FileSourceConstantMetadataStructField(FILE_BLOCK_LENGTH, LongType, nullable = false),
FileSourceConstantMetadataStructField(FILE_MODIFICATION_TIME, TimestampType, nullable = false))
+ /**
+ * All [[BASE_METADATA_FIELDS]] require custom extractors because they are derived directly from
+ * fields of the [[PartitionedFile]], and do have entries in the file's metadata map.
+ */
+ val BASE_METADATA_EXTRACTORS: Map[String, PartitionedFile => Any] = Map(
+ FILE_PATH -> { pf: PartitionedFile => pf.toPath.toString },
+ FILE_NAME -> { pf: PartitionedFile => pf.toPath.getName },
+ FILE_SIZE -> { pf: PartitionedFile => pf.fileSize },
+ FILE_BLOCK_START -> { pf: PartitionedFile => pf.start },
+ FILE_BLOCK_LENGTH -> { pf: PartitionedFile => pf.length },
+ // The modificationTime from the file has millisecond granularity, but the TimestampType for
+ // `file_modification_time` has microsecond granularity.
+ FILE_MODIFICATION_TIME -> { pf: PartitionedFile => pf.modificationTime * 1000 }
+ )
+
+ /**
+ * Extracts the [[Literal]] value of a file-constant metadata column from a [[PartitionedFile]].
+ *
+ * If an extractor is available, use it. Otherwise, attempt to fetch the value directly from the
+ * file's metadata map, returning null if not found.
+ *
+ * Raw values (including null) are automatically converted to literals as a courtesy.
+ */
+ def getFileConstantMetadataColumnValue(
+ name: String,
+ file: PartitionedFile,
+ metadataExtractors: Map[String, PartitionedFile => Any]): Literal = {
+ val extractor = metadataExtractors.get(name).getOrElse {
+ (_: PartitionedFile).otherConstantMetadataColumnValues.get(name).orNull
Review Comment:
Yup. I already came to the same conclusion and fixed it on the OSS side. This side will get fixed when I rebase on the OSS sync (if not sooner).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1173758526
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * A scanner must derive each file-constant metadata field's value from each [[PartitionedFile]]
+ * it processes. By default, the value is obtained by a direct lookup of the column's name on
+ * [[PartitionedFile.otherConstantMetadataColumnValues]] (see
+ * [[FileFormat.getFileConstantMetadataColumnValue]]). However, implementations can override this
+ * method in order to provide more sophisticated lazy extractors (e.g. in case the column value is
+ * complicated or expensive to compute).
Review Comment:
I thought I _did_ describe it explicitly:
1. If you provide an extractor, the extractor has access to all state in the `PartitionedFile` (including the column value map) and can do any computations it needs to.
2. Otherwise, the column's value is fetched from the column value map.
Was it not clear [enough] in the comment that the extractor has access to the entire `PartitionedFile` ?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] johanl-db commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "johanl-db (via GitHub)" <gi...@apache.org>.
johanl-db commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1173803115
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * A scanner must derive each file-constant metadata field's value from each [[PartitionedFile]]
+ * it processes. By default, the value is obtained by a direct lookup of the column's name on
+ * [[PartitionedFile.otherConstantMetadataColumnValues]] (see
+ * [[FileFormat.getFileConstantMetadataColumnValue]]). However, implementations can override this
+ * method in order to provide more sophisticated lazy extractors (e.g. in case the column value is
+ * complicated or expensive to compute).
Review Comment:
The way I read it is that using `otherConstantMetadataColumnValues` is the default and an extractor should only be used for more advanced use cases. I think we rather want to encourage using extractors whenever possible and only fallback to manually providing the values when necessary.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on PR #40885:
URL: https://github.com/apache/spark/pull/40885#issuecomment-1518150945
FYI the [tests that failed](https://github.com/ryan-johnson-databricks/spark/actions/runs/4765599580/jobs/8471553389) are broken upstream -- they also fail on the version of spark/master this PR is currently based on.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1175034679
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * By default, the value of a file-constant metadata column is obtained by looking up the column's
+ * name in the file's metadata column value map. However, implementations can override this method
+ * in order to provide an extractor that has access to the entire [[PartitionedFile]] when
+ * deriving the column's value.
+ *
+ * NOTE: Extractors are lazy, invoked only if the query actually selects their column at runtime.
+ *
+ * See also [[FileFormat.getFileConstantMetadataColumnValue]].
+ */
+ def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any] =
Review Comment:
API needs to be clean. Functionality/safety is easy to achieve here.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] johanl-db commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "johanl-db (via GitHub)" <gi...@apache.org>.
johanl-db commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1173430612
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -241,47 +256,74 @@ object FileFormat {
FileSourceConstantMetadataStructField(FILE_BLOCK_LENGTH, LongType, nullable = false),
FileSourceConstantMetadataStructField(FILE_MODIFICATION_TIME, TimestampType, nullable = false))
+ /**
+ * All [[BASE_METADATA_FIELDS]] require custom extractors because they are derived directly from
+ * fields of the [[PartitionedFile]], and do have entries in the file's metadata map.
+ */
+ val BASE_METADATA_EXTRACTORS: Map[String, PartitionedFile => Any] = Map(
+ FILE_PATH -> { pf: PartitionedFile => pf.toPath.toString },
+ FILE_NAME -> { pf: PartitionedFile => pf.toPath.getName },
+ FILE_SIZE -> { pf: PartitionedFile => pf.fileSize },
+ FILE_BLOCK_START -> { pf: PartitionedFile => pf.start },
+ FILE_BLOCK_LENGTH -> { pf: PartitionedFile => pf.length },
+ // The modificationTime from the file has millisecond granularity, but the TimestampType for
+ // `file_modification_time` has microsecond granularity.
+ FILE_MODIFICATION_TIME -> { pf: PartitionedFile => pf.modificationTime * 1000 }
+ )
+
+ /**
+ * Extracts the [[Literal]] value of a file-constant metadata column from a [[PartitionedFile]].
+ *
+ * If an extractor is available, use it. Otherwise, attempt to fetch the value directly from the
+ * file's metadata map, returning null if not found.
+ *
+ * Raw values (including null) are automatically converted to literals as a courtesy.
+ */
+ def getFileConstantMetadataColumnValue(
+ name: String,
+ file: PartitionedFile,
+ metadataExtractors: Map[String, PartitionedFile => Any]): Literal = {
+ val extractor = metadataExtractors.get(name).getOrElse {
+ (_: PartitionedFile).otherConstantMetadataColumnValues.get(name).orNull
Review Comment:
Note: the syntax
```
(_: PartitionedFile).otherConstantMetadataColumnValues.get(name).orNull
```
to define the default extractor threw me off at first, the following would be easier to understand:
```
pf: PartitionedFile => pf.otherConstantMetadataColumnValues.get(name).orNull
```
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * A scanner must derive each file-constant metadata field's value from each [[PartitionedFile]]
+ * it processes. By default, the value is obtained by a direct lookup of the column's name on
+ * [[PartitionedFile.otherConstantMetadataColumnValues]] (see
+ * [[FileFormat.getFileConstantMetadataColumnValue]]). However, implementations can override this
+ * method in order to provide more sophisticated lazy extractors (e.g. in case the column value is
+ * complicated or expensive to compute).
Review Comment:
Someone adding a new constant metadata column in the future will need to choose between the two alternatives to provide values, this choice could be described more explicitly here:
- extractor: anytime the value can be directly derived from `PartitionedFile` information.
- `PartitionedFile.otherConstantMetadataColumnValues`: all other cases, e.g. the value is arbitrary or is coming from some other place
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1176435868
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -241,47 +260,74 @@ object FileFormat {
FileSourceConstantMetadataStructField(FILE_BLOCK_LENGTH, LongType, nullable = false),
FileSourceConstantMetadataStructField(FILE_MODIFICATION_TIME, TimestampType, nullable = false))
+ /**
+ * All [[BASE_METADATA_FIELDS]] require custom extractors because they are derived directly from
+ * fields of the [[PartitionedFile]], and do have entries in the file's metadata map.
Review Comment:
Erm, yes... I think I was trying to type "do not" but didn't quite make it.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1173755343
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -241,47 +256,74 @@ object FileFormat {
FileSourceConstantMetadataStructField(FILE_BLOCK_LENGTH, LongType, nullable = false),
FileSourceConstantMetadataStructField(FILE_MODIFICATION_TIME, TimestampType, nullable = false))
+ /**
+ * All [[BASE_METADATA_FIELDS]] require custom extractors because they are derived directly from
+ * fields of the [[PartitionedFile]], and do have entries in the file's metadata map.
+ */
+ val BASE_METADATA_EXTRACTORS: Map[String, PartitionedFile => Any] = Map(
+ FILE_PATH -> { pf: PartitionedFile => pf.toPath.toString },
+ FILE_NAME -> { pf: PartitionedFile => pf.toPath.getName },
+ FILE_SIZE -> { pf: PartitionedFile => pf.fileSize },
+ FILE_BLOCK_START -> { pf: PartitionedFile => pf.start },
+ FILE_BLOCK_LENGTH -> { pf: PartitionedFile => pf.length },
+ // The modificationTime from the file has millisecond granularity, but the TimestampType for
+ // `file_modification_time` has microsecond granularity.
+ FILE_MODIFICATION_TIME -> { pf: PartitionedFile => pf.modificationTime * 1000 }
+ )
+
+ /**
+ * Extracts the [[Literal]] value of a file-constant metadata column from a [[PartitionedFile]].
+ *
+ * If an extractor is available, use it. Otherwise, attempt to fetch the value directly from the
+ * file's metadata map, returning null if not found.
+ *
+ * Raw values (including null) are automatically converted to literals as a courtesy.
+ */
+ def getFileConstantMetadataColumnValue(
+ name: String,
+ file: PartitionedFile,
+ metadataExtractors: Map[String, PartitionedFile => Any]): Literal = {
+ val extractor = metadataExtractors.get(name).getOrElse {
+ (_: PartitionedFile).otherConstantMetadataColumnValues.get(name).orNull
Review Comment:
Yup. I already came to the same conclusion and fixed it on the OSS side. This side will get fixed when I rebase on the OSS sync (if not sooner).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1174017427
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * A scanner must derive each file-constant metadata field's value from each [[PartitionedFile]]
+ * it processes. By default, the value is obtained by a direct lookup of the column's name on
+ * [[PartitionedFile.otherConstantMetadataColumnValues]] (see
+ * [[FileFormat.getFileConstantMetadataColumnValue]]). However, implementations can override this
+ * method in order to provide more sophisticated lazy extractors (e.g. in case the column value is
+ * complicated or expensive to compute).
Review Comment:
That wording was intentional... the simple way is usually the right way and thus should be the default. If something special is going on, I'd prefer it to be highly visible.
Put another way -- if the column _can_ be represented simply by pulling the value from the map, why should we encourage the use of an extractor?
An alternative would be to require all constant metadata columns to provide extractors, and define a default extractor that just goes to the map... but this seems like extra boilerplate for no real benefit?
(I originally wanted to just embed the extractor in the attribute definition directly, but attribute/struct metadata cannot store code and IMO that's a wise design decision we shouldn't change)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1174017427
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * A scanner must derive each file-constant metadata field's value from each [[PartitionedFile]]
+ * it processes. By default, the value is obtained by a direct lookup of the column's name on
+ * [[PartitionedFile.otherConstantMetadataColumnValues]] (see
+ * [[FileFormat.getFileConstantMetadataColumnValue]]). However, implementations can override this
+ * method in order to provide more sophisticated lazy extractors (e.g. in case the column value is
+ * complicated or expensive to compute).
Review Comment:
That wording was intentional... the simple way is usually the right way and thus should be the default. If something special is going on, I'd prefer it to be highly visible.
Put another way -- if the column _can_ be populated by simply pulling its value from the map, why should we encourage the use of an extractor? Especially when the map is the only part of `PartitionedFile` whose content a `FileIndex` has any real control over?
An alternative would be to require all constant metadata columns to provide extractors, and define a default extractor that just goes to the map... but this seems like extra boilerplate for no real benefit?
(I originally wanted to just embed the extractor in the attribute definition directly, but attribute/struct metadata cannot store code and IMO that's a wise design decision we shouldn't change)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1175275233
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * By default, the value of a file-constant metadata column is obtained by looking up the column's
+ * name in the file's metadata column value map. However, implementations can override this method
+ * in order to provide an extractor that has access to the entire [[PartitionedFile]] when
+ * deriving the column's value.
+ *
+ * NOTE: Extractors are lazy, invoked only if the query actually selects their column at runtime.
+ *
+ * See also [[FileFormat.getFileConstantMetadataColumnValue]].
+ */
+ def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any] =
Review Comment:
Meanwhile, what "safety" issue are we even responding to here? Right now, the behavior of a file-constant metadata column is perfectly predictable (and consistent with its behavior before this PR) -- _unless_ the user opts in to using an extractor by overriding the method to provide an extractor for the column.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1175271997
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * By default, the value of a file-constant metadata column is obtained by looking up the column's
+ * name in the file's metadata column value map. However, implementations can override this method
+ * in order to provide an extractor that has access to the entire [[PartitionedFile]] when
+ * deriving the column's value.
+ *
+ * NOTE: Extractors are lazy, invoked only if the query actually selects their column at runtime.
+ *
+ * See also [[FileFormat.getFileConstantMetadataColumnValue]].
+ */
+ def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any] =
Review Comment:
My earlier response assumed you meant two APIs, and was pointing out that it would cause headaches for both user and implementation because most metadata columns don't need extractors, and the code that uses extractors is different from the code that uses the attributes (so both codes would have to manually manipulate the column list in some way).
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * By default, the value of a file-constant metadata column is obtained by looking up the column's
+ * name in the file's metadata column value map. However, implementations can override this method
+ * in order to provide an extractor that has access to the entire [[PartitionedFile]] when
+ * deriving the column's value.
+ *
+ * NOTE: Extractors are lazy, invoked only if the query actually selects their column at runtime.
+ *
+ * See also [[FileFormat.getFileConstantMetadataColumnValue]].
+ */
+ def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any] =
Review Comment:
Meanwhile, what "safety" issue are we even responding to here? Right now, the behavior of a file-constant metadata column is perfectly predictable (and consistent with its behavior before this PR) -- _unless_ the user opts in to using an extractor by overriding the method to provide an extractor for it.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1176152776
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -241,47 +260,74 @@ object FileFormat {
FileSourceConstantMetadataStructField(FILE_BLOCK_LENGTH, LongType, nullable = false),
FileSourceConstantMetadataStructField(FILE_MODIFICATION_TIME, TimestampType, nullable = false))
+ /**
+ * All [[BASE_METADATA_FIELDS]] require custom extractors because they are derived directly from
+ * fields of the [[PartitionedFile]], and do have entries in the file's metadata map.
Review Comment:
@ryan-johnson-databricks do I understand this correctly?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan closed pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
URL: https://github.com/apache/spark/pull/40885
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1175437133
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * By default, the value of a file-constant metadata column is obtained by looking up the column's
+ * name in the file's metadata column value map. However, implementations can override this method
+ * in order to provide an extractor that has access to the entire [[PartitionedFile]] when
+ * deriving the column's value.
+ *
+ * NOTE: Extractors are lazy, invoked only if the query actually selects their column at runtime.
+ *
+ * See also [[FileFormat.getFileConstantMetadataColumnValue]].
+ */
+ def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any] =
Review Comment:
I made a dummy PR https://github.com/apache/spark/pull/40930 to show what the other approach actually looks like. I still think it is uglier code for everyone involved, with no benefit to safety, but at least this way we have something concrete to compare.
The change for the other PR vs master is:
```
sql/core/src/main/scala/org/apache/spark/sql/execution/DataSourceScanExec.scala | 33 +++++++++++++---------------
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala | 145 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileScanRDD.scala | 70 +++++++++++++++--------------------------------------------
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/PartitioningAwareFileIndex.scala | 10 ++++-----
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetFileFormat.scala | 4 ++--
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/FileSourceCustomMetadataStructSuite.scala | 71 ++++++++++++++++++++++++++++++++++++++++++++++++------------
6 files changed, 196 insertions(+), 137 deletions(-)
```
The change of this PR vs. master touches fewer files and is ~50LoC smaller:
```
sql/core/src/main/scala/org/apache/spark/sql/execution/DataSourceScanExec.scala | 33 +++++++++++++++------------------
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileScanRDD.scala | 70 +++++++++++++++++-----------------------------------------------------
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/PartitioningAwareFileIndex.scala | 10 +++++-----
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/FileSourceCustomMetadataStructSuite.scala | 35 ++++++++++++++++++++++++++++++++++-
5 files changed, 143 insertions(+), 107 deletions(-)
```
The other PR vs. this one is changes 3 files and ~100LoC:
```
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetFileFormat.scala | 4 ++--
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/FileSourceCustomMetadataStructSuite.scala | 70 ++++++++++++++++++++++++++++++++++++++++------------------------------
3 files changed, 98 insertions(+), 75 deletions(-)
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1174017427
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * A scanner must derive each file-constant metadata field's value from each [[PartitionedFile]]
+ * it processes. By default, the value is obtained by a direct lookup of the column's name on
+ * [[PartitionedFile.otherConstantMetadataColumnValues]] (see
+ * [[FileFormat.getFileConstantMetadataColumnValue]]). However, implementations can override this
+ * method in order to provide more sophisticated lazy extractors (e.g. in case the column value is
+ * complicated or expensive to compute).
Review Comment:
That wording was intentional... the simple way is usually the right way and thus should be the default. If something special is going on, I'd prefer it to be highly visible.
Put another way -- if the column _can_ be populated by simply pulling its value from the map, why should we encourage the use of an extractor?
An alternative would be to require all constant metadata columns to provide extractors, and define a default extractor that just goes to the map... but this seems like extra boilerplate for no real benefit?
(I originally wanted to just embed the extractor in the attribute definition directly, but attribute/struct metadata cannot store code and IMO that's a wise design decision we shouldn't change)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1175271997
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * By default, the value of a file-constant metadata column is obtained by looking up the column's
+ * name in the file's metadata column value map. However, implementations can override this method
+ * in order to provide an extractor that has access to the entire [[PartitionedFile]] when
+ * deriving the column's value.
+ *
+ * NOTE: Extractors are lazy, invoked only if the query actually selects their column at runtime.
+ *
+ * See also [[FileFormat.getFileConstantMetadataColumnValue]].
+ */
+ def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any] =
Review Comment:
My earlier response assumed you meant two APIs, and was pointing out that it would cause headaches for both user and implementation because most metadata columns don't need extractors, and the code that uses extractors is different from the code that uses the attributes.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1174397988
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * By default, the value of a file-constant metadata column is obtained by looking up the column's
+ * name in the file's metadata column value map. However, implementations can override this method
+ * in order to provide an extractor that has access to the entire [[PartitionedFile]] when
+ * deriving the column's value.
+ *
+ * NOTE: Extractors are lazy, invoked only if the query actually selects their column at runtime.
+ *
+ * See also [[FileFormat.getFileConstantMetadataColumnValue]].
+ */
+ def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any] =
Review Comment:
> Shall we have a single API to define constant metadata columns?
Maybe? I couldn't figure out a good way to do it:
> I originally wanted to just embed the extractor in the attribute definition directly, but attribute/struct metadata cannot store code and IMO that's a wise design decision we shouldn't change
I guess we could store pairs or a triple, but (a) the extractors are optional (we'd be forcing every column to either use one or opt out); and (b) the uses for the attribute/struct definitions are rather separated from the uses of the extractors (result: a lot of `x.map` and `x.filter` type calls clutter up the code)
> Should the extractor return a `Literal` instead of `Any`?
I thought about it? But:
* it complicates the definitions of all extractors with boilerplate, which IMO indicates a flawed design
* we anyway have to wrap it in a literal again to handle the `null` vs. `lit(null)` case (or require every extractor to think carefully about nulls).
* Currently, `getFileConstantMetadataColumnValue` takes care of converting to literal, and that's what everyone uses (nobody calls the extractor directly).
* It would be non-symmetric with the default case, that pulls `Any` from the column value map.
Note that nothing prevents an extractor from returning a literal, because `Literal.apply` will detect and return it unchanged.
Overall, I worry the above changes would only give the code a bigger surface area with little/no gain in safety or functionality?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1175031042
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * By default, the value of a file-constant metadata column is obtained by looking up the column's
+ * name in the file's metadata column value map. However, implementations can override this method
+ * in order to provide an extractor that has access to the entire [[PartitionedFile]] when
+ * deriving the column's value.
+ *
+ * NOTE: Extractors are lazy, invoked only if the query actually selects their column at runtime.
+ *
+ * See also [[FileFormat.getFileConstantMetadataColumnValue]].
+ */
+ def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any] =
Review Comment:
Can we have 2 APIs?
```
def constantMetadataCols: Seq[(StructField, PartitionedFile => Any)] = ...
def generatedMetadataCols: Seq[StructField]
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1175286323
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * By default, the value of a file-constant metadata column is obtained by looking up the column's
+ * name in the file's metadata column value map. However, implementations can override this method
+ * in order to provide an extractor that has access to the entire [[PartitionedFile]] when
+ * deriving the column's value.
+ *
+ * NOTE: Extractors are lazy, invoked only if the query actually selects their column at runtime.
+ *
+ * See also [[FileFormat.getFileConstantMetadataColumnValue]].
+ */
+ def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any] =
Review Comment:
Today's API is two pieces:
```scala
/** All fields of the _metadata struct (in declared order) */
def metadataSchemaFields: Seq[StructField]
/** Custom extractors for the subset of file-constant metadata columns that actually need them */
def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any]
```
Splitting the API would produce instead (at a minimum):
```scala
/** Extractor for file-constant metadata columns whose value comes from the metadata map */
val DEFAULT_EXTRACTOR: PartitionedFile => Any
/** File-constant fields of the _metadata struct */
def constantMetadataCols: Seq[(StructField, PartitionedFile => Any)]
/** File-generated fields of the _metadata struct */
def generatedMetadataCols: Seq[StructField]
```
The only safety issue I thought of so far, is that it might be possible to get the name mapping wrong between columns and their extractors. But if that happened, even the most basic unit test for the new column would fail. Meanwhile, name mapping is anyway needed internally because the list of metadata columns we have to work with is named attributes or even just column names extracted from the query plan itself. Thus, the pairing proposed above would anyway have to be reconstructed.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1175286323
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * By default, the value of a file-constant metadata column is obtained by looking up the column's
+ * name in the file's metadata column value map. However, implementations can override this method
+ * in order to provide an extractor that has access to the entire [[PartitionedFile]] when
+ * deriving the column's value.
+ *
+ * NOTE: Extractors are lazy, invoked only if the query actually selects their column at runtime.
+ *
+ * See also [[FileFormat.getFileConstantMetadataColumnValue]].
+ */
+ def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any] =
Review Comment:
Today's API is two pieces:
```scala
/** All fields of the _metadata struct */
def metadataSchemaFields: Seq[StructField]
/** Custom extractors for the subset of file-constant metadata columns that actually need them */
def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any]
```
Splitting the API would produce instead (at a minimum):
```scala
/** Extractor for file-constant metadata columns that just pull the value from the metadata map */
val DEFAULT_EXTRACTOR: PartitionedFile => Any
/** Defines the file-constant fields of the _metadata struct */
def constantMetadataCols: Seq[(StructField, PartitionedFile => Any)]
/** Defines the file-generated fields of the _metadata struct */
def generatedMetadataCols: Seq[StructField]
```
The only safety issue I thought of so far, is that it might be possible to get the name mapping wrong between columns and their extractors. But if that happened even the most basic unit test for the new column would fail. Meanwhile, that mapping is anyway needed internally because the list of metadata columns is named attributes or even column names, so the pairing proposed above would anyway have to be reconstructed.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ryan-johnson-databricks commented on a diff in pull request #40885: [SPARK-43226] Define extractors for file-constant metadata
Posted by "ryan-johnson-databricks (via GitHub)" <gi...@apache.org>.
ryan-johnson-databricks commented on code in PR #40885:
URL: https://github.com/apache/spark/pull/40885#discussion_r1174397988
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -203,6 +203,21 @@ trait FileFormat {
* method. Technically, a file format could choose suppress them, but that is not recommended.
*/
def metadataSchemaFields: Seq[StructField] = FileFormat.BASE_METADATA_FIELDS
+
+ /**
+ * The extractors to use when deriving file-constant metadata columns for this file format.
+ *
+ * By default, the value of a file-constant metadata column is obtained by looking up the column's
+ * name in the file's metadata column value map. However, implementations can override this method
+ * in order to provide an extractor that has access to the entire [[PartitionedFile]] when
+ * deriving the column's value.
+ *
+ * NOTE: Extractors are lazy, invoked only if the query actually selects their column at runtime.
+ *
+ * See also [[FileFormat.getFileConstantMetadataColumnValue]].
+ */
+ def fileConstantMetadataExtractors: Map[String, PartitionedFile => Any] =
Review Comment:
> Shall we have a single API to define constant metadata columns?
Maybe? I couldn't figure out a good way to do it:
> I originally wanted to just embed the extractor in the attribute definition directly, but attribute/struct metadata cannot store code and IMO that's a wise design decision we shouldn't change
I guess we could store pairs or a triple, but (a) the extractors are optional (we'd be forcing every column to either use one or opt out); and (b) the uses for the attribute/struct definitions are rather separated from the uses of the extractors (result: a lot of `x.map` and `x.filter` type calls clutter up the code)
> Should the extractor return a `Literal` instead of `Any`?
I thought about it? But:
* it complicates the definitions of all extractors with boilerplate, which IMO indicates a flawed design
* we anyway have to wrap it in a literal again to handle the null case (or require every extractor to think about nulls).
* Currently, `getFileConstantMetadataColumnValue` takes care of converting to literal, and that's what everyone uses (nobody calls the extractor directly).
* It would be non-symmetric with the default case, that pulls `Any` from the column value map.
Note that nothing prevents an extractor from returning a literal, because `Literal.apply` will detect and return it unchanged.
Overall, I worry the above changes would only give the code a bigger surface area with little/no gain in safety or functionality?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org