Skip to content

ARROW-13090: [Python] Fix create_dir() implementation in FSSpecHandler #10540

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.

Sign up for GitHub

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

Closed
pitrou wants to merge 1 commit into from
Closed

ARROW-13090: [Python] Fix create_dir() implementation in FSSpecHandler #10540

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion python/pyarrow/fs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,10 @@ def get_file_info_selector(self, selector):

def create_dir(self, path, recursive):
# mkdir also raises FileNotFoundError when base directory is not found
self.fs.mkdir(path, create_parents=recursive)
try:
self.fs.mkdir(path, create_parents=recursive)
except FileExistsError:
Copy link
Member

@jorisvandenbossche jorisvandenbossche Jun 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the "FileExistsError" a bit strange, because I first thought this was about a directory that already existed (but apparently, fsspec's mkdir is fine with that as well and will pass silently if the target directory already exists, at least I checked this for their local filesystem). But so it is actually about a file already existing with that name.
And the current behaviour of our filesystem is then to silently not create a directory. But is that our desired behaviour? I would say that create_dir should guarantee that the target directory exists after calling that method (or otherwise error)

Copy link
Member Author

@pitrou pitrou Jun 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But so it is actually about a file already existing with that name.

Are you sure that's the case?

Copy link
Member

@jorisvandenbossche jorisvandenbossche Jun 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

from fsspec.implementations.local import LocalFileSystem as FSSpecLocalFileSystem
fs = FSSpecLocalFileSystem()
fs.mkdir("test_dir")
fs.touch("test_file")

# creating an existing dir again is fine
In [9]: fs.mkdir("test_dir")

# creating a dir that already exists as a file errors
In [10]: fs.mkdir("test_file")
...
FileExistsError: [Errno 17] File exists: '/home/joris/scipy/test_file'

(now, this was a manual test of the expected behaviour, will check what actually happens in the failing test)

Copy link
Member

@jorisvandenbossche jorisvandenbossche Jun 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, but it's the MemoryFileSystem that fails with the exact same example from above .. So they are again not very consistent, that seems a bug in the MemoryFileSystem. Will open an issue about it.

pass

def delete_dir(self, path):
self.fs.rm(path, recursive=True)
Expand Down