cchighman commented on a change in pull request #28841:
URL: https://github.com/apache/spark/pull/28841#discussion_r449241653
##########
File path:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/PartitioningAwareFileIndex.scala
##########
@@ -83,7 +85,7 @@ abstract class PartitioningAwareFileIndex(
val files: Seq[FileStatus] = leafDirToChildrenFiles.get(path) match {
case Some(existingDir) =>
// Directory has children files in it, return them
- existingDir.filter(f => matchGlobPattern(f) && isNonEmptyFile(f))
+ existingDir.filter(f => matchPathPattern(f) && isNonEmptyFile(f))
Review comment:
> `f` here is `FileStatus`. It looks inefficient to get the `FileStatus`
again in
https://github.com/apache/spark/pull/28841/files#diff-3c816e65d0bbdf16d46c7ea4e0b8cbaeR55
I agree. I considered this in one of my earlier commits actually. There
are two places where we handle options in the *FileIndex classes, here in
_PartitioningAwareFileIndex_ and _InMemoryFileIndex_. In both places, we
derive from the _PathFilter_ class which exposes an overridable method called
_accept_ which takes _Path_ as its only parameter.
I could break with the abstraction and overload the method so that it would
take a FileStatus directly. I'll look into this unless there are any
objections.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
