Treat prefix ending with "/" as directory #219
Conversation
|
/test |
|
Successfully created a job for commit 402486c: |
pfio/v2/s3.py
Outdated
| @@ -414,8 +426,28 @@ def isdir(self, file_path: str): | |||
| returning boolean or ``None`` would be nicer. | |||
There was a problem hiding this comment.
Please update this note as it contradicts with the new implementaiton.
| @@ -398,12 +408,14 @@ def stat(self, path): | |||
| return S3ObjectStat(key, res) | |||
| except ClientError as e: | |||
| if e.response['Error']['Code'] == '404': | |||
| if self.isdir(path): | |||
| return S3PrefixStat(key) | |||
There was a problem hiding this comment.
I don't think this is a good design - in cases like kind of a "non-existent" directory (somewhat hard to define), objects in a bucket like
foo/bar.txtandsomefile.txt
In this case, s3.isdir('foo') would be true and also false for s3.isdir('somefile.txt'), which is nice, but this code will also return true for s3.isdir('whatever-nothing'), which is just nothing.
There was a problem hiding this comment.
but this code will also return true for s3.isdir('whatever-nothing'), which is just nothing.
I think (and I confirmed locally) that s3.isdir('whatever-nothing') returns false because this PR handles "common-prefix" as directories.
on the other hand, I think it's not the best choice in terms of performance. It always calls list-object API when the server returns 404 status code.
There was a problem hiding this comment.
Sorry, my bad! I misread the code.
|
/test |
|
Successfully created a job for commit 39f1b04: |
Currently, S3 does not imitate directory. Thus, we have to branch code based on the filesystem when handling directories with
pfio.This PR handles common prefixes that end with "/" as directories in
S3to increase compatibility betweenFileSystems.