New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HIVE-25960: Fix S3a recursive listing logic. #3031
Conversation
Got introduced as part of HIVE-22411. |
@@ -361,7 +361,7 @@ private static void listS3FilesRecursive(FileStatus base, FileSystem fs, List<Fi | |||
RemoteIterator<LocatedFileStatus> remoteIterator = fs.listFiles(base.getPath(), true); | |||
while (remoteIterator.hasNext()) { | |||
LocatedFileStatus each = remoteIterator.next(); | |||
Path relativePath = new Path(each.getPath().toString().replace(base.toString(), "")); | |||
Path relativePath = new Path(each.getPath().toString().replaceFirst(base.getPath().toString(), "")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change makes sense
some ideas:
isn't Path.relativize for the same purpose?
a) is so could you please check its source if it does this optimized version or not worse (from performance POV)
b) if it isn't, could you please refactor this to utility method here? in this case, please include unit test into TestFileUtils
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanx @abstractdog for the review.
isn't Path.relativize for the same purpose?
This Path & the path used here is different, the first one is from the Java package. The latter one is from Hadoop-Common
https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/Path.java
b) if it isn't, could you please refactor this to utility method here? in this case, please include unit test into TestFileUtils
Done
Test failure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM +1
…xena reviewed by Laszlo Bodor)
…xena reviewed by Laszlo Bodor)
Changes replace to replaceFirst and rather than comparing with FileStatus, changes to compare with the Actual Path