[Python] Pyarrow fs incorrectly resolves S3 URIs with white space as a local path

[jaidisido]

Describe the bug, including details regarding any error messages, version, and platform.

Pyarrow fs incorrectly resolves valid S3 URIs with a whitespace as a local path:

from pyarrow.fs import _resolve_filesystem_and_path, FileSystem

uri = "s3://bucket/prefix with space/a=a"

resolved_filesystem, resolved_path = _resolve_filesystem_and_path(uri, None)

resolved_filesystem
<pyarrow._fs.LocalFileSystem at 0x10316ff30>

This causes subsequent calls such as getting the file info to fail:

path_info = resolved_filesystem.get_file_info(resolved_path)

pyarrow.lib.ArrowInvalid: Expected a local filesystem path, got a URI...

A quick look into the method indicates that a LocalFilesytem is chosen by default and returned if alternative filesystems are not detected which seems like a dubious strategy...

I assume this is where the S3 filesystem should be detected but a URI containing a whitespace seems to throw an exception although it's valid:

filesystem, path = FileSystem.from_uri(uri)

Cannot parse URI: 's3://bucket/prefix with space/a=a/'

Component(s)

Python

[AlenkaF]

I think you will need to have URI-encoded spaces in this case, see https://en.wikipedia.org/wiki/Percent-encoding#The_application/x-www-form-urlencoded_type. Could you try replacing whitespace with +?

[AlenkaF]

In any case, we should improve the error message. I will change the labels, contributions are very welcome!

[CrystalZhou0529]

Hi @AlenkaF, I just opened a PR with the implementation of the new error message. Please take a look at my changes if you have time, and let me know if you have any other concerns regarding the error message please! Thanks.

[pitrou]

A quick look into the method, indicates that a LocalFilesytem is chosen by default and returned if alternative filesystems are not detected which seems like a dubious strategy...

I'm not sure there is a better strategy. Can you suggest something?

[jaidisido]

A quick look into the method, indicates that a LocalFilesytem is chosen by default and returned if alternative filesystems are not detected which seems like a dubious strategy...

I'm not sure there is a better strategy. Can you suggest something?

I just think there shouldn't be a default and rather an exception raised if no filesystem is detected, including LocalFilesystem. But I appreciate that it might break too many things at this point

[jorisvandenbossche]

So going through the logic in the _resolve_filesystem_and_path helper function, I think the main reason we catch (and swallow) the error from FileSystem.from_uri is because we wanted to provide a better error message for local file paths that were not found.

However, in that case you typically get an error about an empty scheme, and not a "Cannot parse URI" error. For example:

In [1]: import pyarrow.fs

In [2]: pyarrow.fs.FileSystem.from_uri("local/file/path")
...
ArrowInvalid: URI has empty scheme: 'local/file/path'
/arrow/cpp/src/arrow/filesystem/filesystem.cc:937  ParseFileSystemUri(uri_string)

Essentially, if an error happens and if the user provides a URI we want to show the error that happens when parsing the URI, and it the user passes a local file path, we would like to show the error that happens when we assume it is a local file path (and further down the line this will then give a file not found error).

But couldn't we use some basic heuristic to determine if the user is actually passing a URI? (I don't know how robust it is to check if :// in the string?)

Based on the above example, it might also to only catch the "empty scheme" error and let the "Cannot parse URI" error always bubble up to the user.

[pitrou]

There are all kinds of failure modes:

>>> pq.read_table("/local file")
Traceback (most recent call last):
  ...
FileNotFoundError: /local file

>>> pq.read_table("local file")
Traceback (most recent call last):
  ...
FileNotFoundError: local file

>>> pq.read_table("s3://invalid bucket/bar")
Traceback (most recent call last):
  ...
ArrowInvalid: Expected a local filesystem path, got a URI: 's3://invalid bucket/bar'

>>> pq.read_table("s3://really-nonexistent-bucket/bar")
Traceback (most recent call last):
  ...
OSError: Bucket 'really-nonexistent-bucket' not found

[pitrou]

But couldn't we use some basic heuristic to determine if the user is actually passing a URI? (I don't know how robust it is to check if :// in the string?)

It's probably not robust, a local path could contain that string too (it's probably rare on Unix, but perhaps not on Windows).

Edit: we do have such a heuristic already in C++, it's where the "Expected a local filesystem path, got a URI" message comes from. It can probably reused.

arrow/cpp/src/arrow/filesystem/path_util.cc

Lines 342 to 360 in 032e6a4

	bool IsLikelyUri(std::string_view v) {
	if (v.empty() || v[0] == '/') {
	return false;
	}
	const auto pos = v.find_first_of(':');
	if (pos == v.npos) {
	return false;
	}
	if (pos < 2) {
	// One-letter URI schemes don't officially exist, perhaps a Windows drive letter?
	return false;
	}
	if (pos > 36) {
	// The largest IANA-registered URI scheme is "microsoft.windows.camera.multipicker"
	// with 36 characters.
	return false;
	}
	return ::arrow::util::IsValidUriScheme(v.substr(0, pos));
	}

[jorisvandenbossche]

There are all kinds of failure modes:

Yes, but so those errors mostly comes from after _resolve_filesystem_and_path, when we are actually trying to read based on the inferred filesystem+path combo:

In [1]: from pyarrow.fs import _resolve_filesystem_and_path

In [2]: _resolve_filesystem_and_path("/local file")
Out[2]: (<pyarrow._fs.LocalFileSystem at 0x7fa597bc38b0>, '/local file')

In [3]: _resolve_filesystem_and_path("local file")
Out[3]: (<pyarrow._fs.LocalFileSystem at 0x7fa591904bb0>, 'local file')

In [4]: _resolve_filesystem_and_path("s3://invalid bucket/bar")
Out[4]: (<pyarrow._fs.LocalFileSystem at 0x7fa591380eb0>, 's3://invalid bucket/bar')

In [5]: _resolve_filesystem_and_path("s3://really-nonexistent-bucket/bar")
...
OSError: Bucket 'really-nonexistent-bucket' not found

So it is for the third case where we give a wrong result in this step of the code path, which then later in pq.read_table, when actually trying to read the file, gives the confusing "ArrowInvalid: Expected a local filesystem path, got a URI: 's3://invalid bucket/bar'".
But so that only happens because we incorrectly return that as a local file system. While I think we should be able to detect that it is actually a URI, and therefore that we should return the error from trying to parse that URI with FileSystem.from_uri (as happens in the last case above)

Edit: we do have such a heuristic already in C++, it's where the "Expected a local filesystem path, got a URI" message comes from. It can probably reused.

One option could be to use that in FileSystemFromUri, for example when the parsing fails check if it actually is a likely URI, and if not raise a different error message?

Or bind it in Python, so we can use it to decide in _resolve_filesystem_and_path whether we want to bubble up the error or not.

[pitrou]

One option could be to use that in FileSystemFromUri, for example when the parsing fails check if it actually is a likely URI, and if not raise a different error message?

Or bind it in Python, so we can use it to decide in _resolve_filesystem_and_path whether we want to bubble up the error or not.

FileSystemFromUri expects a URI, so we should keep the original error message there.

Using it in _resolve_filesystem_and_path, though, seems reasonable.