You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/12/29 08:31:32 UTC
[GitHub] [spark] Yaohua628 opened a new pull request #35055: [SPARK-37769][SQL][FOLLOWUP] Filtering files if metadata columns are present in the data filter
Yaohua628 opened a new pull request #35055:
URL: https://github.com/apache/spark/pull/35055
### What changes were proposed in this pull request?
Follow-up PR of #34575. Filtering files if metadata columns are present in the data filter.
### Why are the changes needed?
Performance improvements.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing UTs and a new UT.
--
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] Yaohua628 commented on pull request #35055: [SPARK-37769][SQL][FOLLOWUP] Filtering files if metadata columns are present in the data filter
Posted by GitBox <gi...@apache.org>.
Yaohua628 commented on pull request #35055:
URL: https://github.com/apache/spark/pull/35055#issuecomment-1002460008
Hi @cloud-fan, here's metadata filtering PR, please take a look whenever you have a chance. Also, got a question: what about `df.inputFiles`? Do you have any idea how to make it work as well? Thanks!
--
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] Yaohua628 commented on a change in pull request #35055: [SPARK-37769][SQL][FOLLOWUP] Filtering files if metadata columns are present in the data filter
Posted by GitBox <gi...@apache.org>.
Yaohua628 commented on a change in pull request #35055:
URL: https://github.com/apache/spark/pull/35055#discussion_r776566204
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/PartitioningAwareFileIndex.scala
##########
@@ -71,8 +73,34 @@ abstract class PartitioningAwareFileIndex(
def isNonEmptyFile(f: FileStatus): Boolean = {
isDataPath(f.getPath) && f.getLen > 0
}
+
+ // retrieve the file metadata filters and reduce to a final filter expression
+ val fileMetadataFilterOpt = dataFilters.filter(_.references.forall {
+ case MetadataAttribute(_) => true
+ case _ => false
+ }).reduceOption(expressions.And)
+
+ // will create internal rows for each file, position 0: Metadata Struct
+ val boundedFilterOpt = fileMetadataFilterOpt.map { fileMetadataFilter =>
+ Predicate.createInterpreted(fileMetadataFilter.transform {
+ case _: AttributeReference => BoundReference(0, METADATA_STRUCT, nullable = true)
+ })
+ }
+
+ def matchFileMetadataPredicate(f: FileStatus): Boolean = {
+ // use option.forall, so if there is no filter, return true
+ boundedFilterOpt.forall(_.eval(
+ InternalRow.fromSeq(Seq(InternalRow.fromSeq(Seq(
Review comment:
could we have this util method and share the common code maybe along with the metadata schema pruning PR?
I am thinking after pruning, we don't need to always create `InternalRow` with all fields in both `PartitioningAwareFileIndex.listFiles(...)` and `FileScanRDD`. WDYT?
--
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 #35055: [SPARK-37769][SQL][FOLLOWUP] Filtering files if metadata columns are present in the data filter
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #35055:
URL: https://github.com/apache/spark/pull/35055#issuecomment-1015379618
LGTM if 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] cloud-fan commented on pull request #35055: [SPARK-37769][SQL][FOLLOWUP] Filtering files if metadata columns are present in the data filter
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #35055:
URL: https://github.com/apache/spark/pull/35055#issuecomment-1016096165
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] cloud-fan commented on a change in pull request #35055: [SPARK-37769][SQL][FOLLOWUP] Filtering files if metadata columns are present in the data filter
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35055:
URL: https://github.com/apache/spark/pull/35055#discussion_r776222941
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/FileMetadataStructSuite.scala
##########
@@ -288,6 +288,20 @@ class FileMetadataStructSuite extends QueryTest with SharedSparkSession {
)
}
+ metadataColumnsTest("filter on metadata and user data", schema) { (df, _, f1) =>
Review comment:
I think we should also check the physical plan and make sure the final file list is pruned.
--
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] Yaohua628 commented on pull request #35055: [SPARK-37769][SQL][FOLLOWUP] Filtering files if metadata columns are present in the data filter
Posted by GitBox <gi...@apache.org>.
Yaohua628 commented on pull request #35055:
URL: https://github.com/apache/spark/pull/35055#issuecomment-1016097405
thanks, Wenchen!
--
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] Yaohua628 commented on a change in pull request #35055: [SPARK-37769][SQL][FOLLOWUP] Filtering files if metadata columns are present in the data filter
Posted by GitBox <gi...@apache.org>.
Yaohua628 commented on a change in pull request #35055:
URL: https://github.com/apache/spark/pull/35055#discussion_r776531617
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/FileMetadataStructSuite.scala
##########
@@ -288,6 +288,20 @@ class FileMetadataStructSuite extends QueryTest with SharedSparkSession {
)
}
+ metadataColumnsTest("filter on metadata and user data", schema) { (df, _, f1) =>
Review comment:
no, it won't fail before. I added this test just wanna make sure we only get metadata filters in `listFiles(...)` and do the correct filtering [here](https://github.com/apache/spark/pull/35055/files#diff-0d5b92aea54d993bd94249fdc9ba7d2616a3f4f04289ffaa2deb55a06585d4f9R78)
thanks for the suggestion! updated tests to check files!
--
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 #35055: [SPARK-37769][SQL][FOLLOWUP] Filtering files if metadata columns are present in the data filter
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #35055:
URL: https://github.com/apache/spark/pull/35055
--
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] Yaohua628 commented on pull request #35055: [SPARK-37769][SQL][FOLLOWUP] Filtering files if metadata columns are present in the data filter
Posted by GitBox <gi...@apache.org>.
Yaohua628 commented on pull request #35055:
URL: https://github.com/apache/spark/pull/35055#issuecomment-1015930171
> LGTM if tests pass
Thanks, passed!
--
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] AmplabJenkins commented on pull request #35055: [SPARK-37769][SQL][FOLLOWUP] Filtering files if metadata columns are present in the data filter
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #35055:
URL: https://github.com/apache/spark/pull/35055#issuecomment-1002806733
Can one of the admins verify this patch?
--
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] Yaohua628 commented on pull request #35055: [SPARK-37769][SQL][FOLLOWUP] Filtering files if metadata columns are present in the data filter
Posted by GitBox <gi...@apache.org>.
Yaohua628 commented on pull request #35055:
URL: https://github.com/apache/spark/pull/35055#issuecomment-1015191844
@cloud-fan Hi Wenchen, updated this PR on top of the schema pruning.
Also, add 2 utility methods and tried to share more code between `FileScanRDD` and `PartitioningAwareFileIndex` to create/update metadata internal row. Please let me know what do you think, thanks!!
--
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 change in pull request #35055: [SPARK-37769][SQL][FOLLOWUP] Filtering files if metadata columns are present in the data filter
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35055:
URL: https://github.com/apache/spark/pull/35055#discussion_r776557948
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/PartitioningAwareFileIndex.scala
##########
@@ -71,8 +73,34 @@ abstract class PartitioningAwareFileIndex(
def isNonEmptyFile(f: FileStatus): Boolean = {
isDataPath(f.getPath) && f.getLen > 0
}
+
+ // retrieve the file metadata filters and reduce to a final filter expression
+ val fileMetadataFilterOpt = dataFilters.filter(_.references.forall {
+ case MetadataAttribute(_) => true
+ case _ => false
+ }).reduceOption(expressions.And)
+
+ // will create internal rows for each file, position 0: Metadata Struct
+ val boundedFilterOpt = fileMetadataFilterOpt.map { fileMetadataFilter =>
+ Predicate.createInterpreted(fileMetadataFilter.transform {
+ case _: AttributeReference => BoundReference(0, METADATA_STRUCT, nullable = true)
+ })
+ }
+
+ def matchFileMetadataPredicate(f: FileStatus): Boolean = {
+ // use option.forall, so if there is no filter, return true
+ boundedFilterOpt.forall(_.eval(
+ InternalRow.fromSeq(Seq(InternalRow.fromSeq(Seq(
Review comment:
Can we have a util method to create InternalRow from `Path`, `length: Long` and `modificationTime: Long`? To share code with `FileScanRDD`
--
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 change in pull request #35055: [SPARK-37769][SQL][FOLLOWUP] Filtering files if metadata columns are present in the data filter
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35055:
URL: https://github.com/apache/spark/pull/35055#discussion_r776569081
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/PartitioningAwareFileIndex.scala
##########
@@ -71,8 +73,34 @@ abstract class PartitioningAwareFileIndex(
def isNonEmptyFile(f: FileStatus): Boolean = {
isDataPath(f.getPath) && f.getLen > 0
}
+
+ // retrieve the file metadata filters and reduce to a final filter expression
+ val fileMetadataFilterOpt = dataFilters.filter(_.references.forall {
+ case MetadataAttribute(_) => true
+ case _ => false
+ }).reduceOption(expressions.And)
+
+ // will create internal rows for each file, position 0: Metadata Struct
+ val boundedFilterOpt = fileMetadataFilterOpt.map { fileMetadataFilter =>
+ Predicate.createInterpreted(fileMetadataFilter.transform {
+ case _: AttributeReference => BoundReference(0, METADATA_STRUCT, nullable = true)
+ })
+ }
+
+ def matchFileMetadataPredicate(f: FileStatus): Boolean = {
+ // use option.forall, so if there is no filter, return true
+ boundedFilterOpt.forall(_.eval(
+ InternalRow.fromSeq(Seq(InternalRow.fromSeq(Seq(
Review comment:
SGTM
--
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 change in pull request #35055: [SPARK-37769][SQL][FOLLOWUP] Filtering files if metadata columns are present in the data filter
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35055:
URL: https://github.com/apache/spark/pull/35055#discussion_r776222732
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/FileMetadataStructSuite.scala
##########
@@ -288,6 +288,20 @@ class FileMetadataStructSuite extends QueryTest with SharedSparkSession {
)
}
+ metadataColumnsTest("filter on metadata and user data", schema) { (df, _, f1) =>
Review comment:
Does this test fail before?
--
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