[SPARK-19748][SQL]refresh function has a wrong order to do cache invalidate and regenerate the inmemory var for InMemoryFileIndex with FileStatusCache #17079
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…alidate and regenerate the inmemory var for InMemoryFileIndex with FileStatusCache
windpiger
changed the title
|
Test build #73502 has finished for PR 17079 at commit
|
Contributor
Author
cloud-fan
reviewed
Feb 28, 2017
| val fileStatusCache = FileStatusCache.getOrCreate(spark) | ||
| val dirPath = new Path(dir.getAbsolutePath) | ||
| val catalog = new InMemoryFileIndex(spark, Seq(dirPath), Map.empty, | ||
| None, fileStatusCache) { |
Contributor
There was a problem hiding this comment.
nit:
val catalog =
new XXX(...) {
def xxx
}
cloud-fan
reviewed
Feb 28, 2017
|
|
||
| assert(catalog.leafFilePaths.size == 1) | ||
| assert(catalog.leafFilePaths.head.toString.stripSuffix("/") == | ||
| s"file:${file.getAbsolutePath.stripSuffix("/")}") |
Contributor
There was a problem hiding this comment.
this looks hacky, can you turn them into Path and compare?
Contributor
Author
There was a problem hiding this comment.
ok, let me modify~ thanks~
Contributor
|
good catch! Can you show a real example that fails because of this bug? I'm wondering why the existing unit tests didn't expose this bug... |
Contributor
Author
|
there is no related test case for InMemoryFileIndex with FileStatusCache. |
|
Test build #73546 has finished for PR 17079 at commit
|
gatorsmile
reviewed
Feb 28, 2017
| } | ||
|
|
||
| assert(catalog.leafDirPaths.isEmpty) | ||
| assert(catalog.leafFilePaths.isEmpty) |
Member
There was a problem hiding this comment.
Move these two asserts after stringToFile
gatorsmile
reviewed
Feb 28, 2017
| new InMemoryFileIndex(spark, Seq(dirPath), Map.empty, None, fileStatusCache) { | ||
| def leafFilePaths: Seq[Path] = leafFiles.keys.toSeq | ||
| def leafDirPaths: Seq[Path] = leafDirToChildrenFiles.keys.toSeq | ||
| } |
Member
There was a problem hiding this comment.
Nit: Indents issues for the above three lines.
Member
|
LGTM except two minor comments. |
|
Test build #73560 has finished for PR 17079 at commit
|
Contributor
|
thanks, merging to master/2.1! |
asfgit
pushed a commit
that referenced
this pull request
Feb 28, 2017
…alidate and regenerate the inmemory var for InMemoryFileIndex with FileStatusCache
## What changes were proposed in this pull request?
If we refresh a InMemoryFileIndex with a FileStatusCache, it will first use the FileStatusCache to re-generate the cachedLeafFiles etc, then call FileStatusCache.invalidateAll.
While the order to do these two actions is wrong, this lead to the refresh action does not take effect.
```
override def refresh(): Unit = {
refresh0()
fileStatusCache.invalidateAll()
}
private def refresh0(): Unit = {
val files = listLeafFiles(rootPaths)
cachedLeafFiles =
new mutable.LinkedHashMap[Path, FileStatus]() ++= files.map(f => f.getPath -> f)
cachedLeafDirToChildrenFiles = files.toArray.groupBy(_.getPath.getParent)
cachedPartitionSpec = null
}
```
## How was this patch tested?
unit test added
Author: windpiger <[email protected]>
Closes #17079 from windpiger/fixInMemoryFileIndexRefresh.
(cherry picked from commit a350bc1)
Signed-off-by: Wenchen Fan <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Mar 3, 2017
…ed to listFiles twice ## What changes were proposed in this pull request? Currently when we resolveRelation for a `FileFormat DataSource` without providing user schema, it will execute `listFiles` twice in `InMemoryFileIndex` during `resolveRelation`. This PR add a `FileStatusCache` for DataSource, this can avoid listFiles twice. But there is a bug in `InMemoryFileIndex` see: [SPARK-19748](#17079) [SPARK-19761](#17093), so this pr should be after SPARK-19748/ SPARK-19761. ## How was this patch tested? unit test added Author: windpiger <[email protected]> Closes #17081 from windpiger/resolveDataSourceScanFilesTwice.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
If we refresh a InMemoryFileIndex with a FileStatusCache, it will first use the FileStatusCache to re-generate the cachedLeafFiles etc, then call FileStatusCache.invalidateAll.
While the order to do these two actions is wrong, this lead to the refresh action does not take effect.
How was this patch tested?
unit test added