From a5191ab084625ccd7423ebbc219656899a332ff8 Mon Sep 17 00:00:00 2001 From: Ernest Provo Date: Sun, 22 Feb 2026 14:10:02 -0500 Subject: [PATCH 1/4] GH-41365: [Python] Fix S3 URI resolution with whitespace falling back to LocalFileSystem Port the C++ `IsLikelyUri()` heuristic to Python and use it in `_resolve_filesystem_and_path()` to distinguish between local paths and malformed URIs before suppressing `ValueError`. Previously, URIs like `s3://bucket/path with space/file` would silently fall back to `LocalFileSystem` because the "Cannot parse URI" error was unconditionally swallowed. Now, paths that look like URIs (valid scheme prefix) propagate the parsing error, while local paths continue to fall back to `LocalFileSystem` as expected. --- python/pyarrow/fs.py | 40 ++++++++++++++++++--- python/pyarrow/tests/test_fs.py | 61 +++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 5 deletions(-) diff --git a/python/pyarrow/fs.py b/python/pyarrow/fs.py index 670ccaaf2455..41308cd60c1d 100644 --- a/python/pyarrow/fs.py +++ b/python/pyarrow/fs.py @@ -82,6 +82,32 @@ def __getattr__(name): ) +def _is_likely_uri(path): + """ + Check if a string looks like a URI (has a valid scheme followed by ':'). + + This is a Python port of the C++ ``IsLikelyUri()`` heuristic in + ``cpp/src/arrow/filesystem/path_util.cc``. It is intentionally + conservative: single-letter schemes are excluded (could be a Windows + drive letter) and the scheme must conform to RFC 3986. + """ + if not path or path[0] == '/': + return False + colon_pos = path.find(':') + if colon_pos < 0: + return False + # One-letter URI schemes don't officially exist; may be a Windows drive + if colon_pos < 2: + return False + # The largest IANA-registered scheme is 36 characters + if colon_pos > 36: + return False + scheme = path[:colon_pos] + if not scheme[0].isalpha(): + return False + return all(c.isalnum() or c in ('+', '-', '.') for c in scheme[1:]) + + def _ensure_filesystem(filesystem, *, use_mmap=False): if isinstance(filesystem, FileSystem): return filesystem @@ -174,13 +200,17 @@ def _resolve_filesystem_and_path(path, filesystem=None, *, memory_map=False): filesystem, path = FileSystem.from_uri(path) except ValueError as e: msg = str(e) - if "empty scheme" in msg or "Cannot parse URI" in msg: - # neither an URI nor a locally existing path, so assume that - # local path was given and propagate a nicer file not found - # error instead of a more confusing scheme parsing error + if "empty scheme" in msg: + # No scheme at all — treat as a local path and propagate + # a nicer "file not found" error later. + pass + elif "Cannot parse URI" in msg and not _is_likely_uri(path): + # Path doesn't look like a URI (no valid scheme prefix), + # so treat it as a local path rather than surfacing a + # confusing URI-parsing error. pass else: - raise e + raise else: path = filesystem.normalize_path(path) diff --git a/python/pyarrow/tests/test_fs.py b/python/pyarrow/tests/test_fs.py index 5bf1950c0654..1f3c96b8252f 100644 --- a/python/pyarrow/tests/test_fs.py +++ b/python/pyarrow/tests/test_fs.py @@ -1725,6 +1725,67 @@ def test_filesystem_from_path_object(path): assert path == p.resolve().absolute().as_posix() +def test_is_likely_uri(): + """Unit tests for the _is_likely_uri() heuristic.""" + from pyarrow.fs import _is_likely_uri + + # Valid URI schemes + assert _is_likely_uri("s3://bucket/key") + assert _is_likely_uri("gs://bucket/key") + assert _is_likely_uri("hdfs://host/path") + assert _is_likely_uri("file:///local/path") + assert _is_likely_uri("abfss://container@account/path") + assert _is_likely_uri("grpc+https://host:443") + + # Not URIs — local paths, Windows drives, empty, etc. + assert not _is_likely_uri("") + assert not _is_likely_uri("/absolute/path") + assert not _is_likely_uri("relative/path") + assert not _is_likely_uri("C:\\Users\\foo") # single-letter → drive + assert not _is_likely_uri("C:/Users/foo") + assert not _is_likely_uri("3bucket://key") # scheme starts with digit + assert not _is_likely_uri("-scheme://key") # scheme starts with dash + + +def test_resolve_filesystem_and_path_uri_with_spaces(): + """ + URIs with a recognised scheme but un-encoded spaces must raise + ValueError — NOT silently fall back to LocalFileSystem. + (GH-41365) + """ + from pyarrow.fs import _resolve_filesystem_and_path + + # S3 URI with spaces should raise, not return LocalFileSystem + with pytest.raises(ValueError, match="Cannot parse URI"): + _resolve_filesystem_and_path("s3://bucket/path with space/file.parquet") + + # GCS URI with spaces should also raise + with pytest.raises(ValueError, match="Cannot parse URI"): + _resolve_filesystem_and_path("gs://bucket/path with space/file.csv") + + # abfss URI with spaces + with pytest.raises(ValueError, match="Cannot parse URI"): + _resolve_filesystem_and_path( + "abfss://container@account/dir with space/file" + ) + + +def test_resolve_filesystem_and_path_local_with_spaces(): + """ + Local paths (no scheme) with spaces should still resolve to + LocalFileSystem — they must NOT be confused with malformed URIs. + """ + from pyarrow.fs import _resolve_filesystem_and_path + + # Absolute local path with spaces → LocalFileSystem + fs, path = _resolve_filesystem_and_path("/tmp/path with spaces/data") + assert isinstance(fs, LocalFileSystem) + + # Non-existent absolute path → still LocalFileSystem + fs, path = _resolve_filesystem_and_path("/nonexistent/path") + assert isinstance(fs, LocalFileSystem) + + @pytest.mark.s3 def test_filesystem_from_uri_s3(s3_server): from pyarrow.fs import S3FileSystem From 670c937faabed86a91ed1c2dbab7b7e3a1b90f38 Mon Sep 17 00:00:00 2001 From: Ernest Provo Date: Fri, 6 Mar 2026 22:52:49 -0500 Subject: [PATCH 2/4] GH-41365: [Python] Refactor _is_likely_uri to use C++ via Cython Replace the pure-Python _is_likely_uri() with a Cython wrapper that calls the C++ arrow::fs::IsLikelyUri(), as requested in PR review. - Move IsLikelyUri from arrow::fs::internal to arrow::fs namespace - Add public declaration in filesystem.h - Keep inline backward-compat wrapper in path_util.h - Add Cython binding in libarrow_fs.pxd and wrapper in _fs.pyx - Import from _fs module in fs.py, delete Python implementation - Add non-ASCII scheme test cases --- cpp/src/arrow/filesystem/filesystem.h | 7 +++++++ cpp/src/arrow/filesystem/path_util.cc | 4 ++++ cpp/src/arrow/filesystem/path_util.h | 7 +++++-- python/pyarrow/_fs.pyx | 8 ++++++++ python/pyarrow/fs.py | 27 +------------------------ python/pyarrow/includes/libarrow_fs.pxd | 2 ++ python/pyarrow/tests/test_fs.py | 2 ++ 7 files changed, 29 insertions(+), 28 deletions(-) diff --git a/cpp/src/arrow/filesystem/filesystem.h b/cpp/src/arrow/filesystem/filesystem.h index 3a47eb62f524..85ee3ff11468 100644 --- a/cpp/src/arrow/filesystem/filesystem.h +++ b/cpp/src/arrow/filesystem/filesystem.h @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -529,6 +530,12 @@ class ARROW_EXPORT SlowFileSystem : public FileSystem { /// The user is responsible for synchronization of calls to this function. void EnsureFinalized(); +/// \brief Return whether a path string is likely a URI. +/// +/// This heuristic is conservative and may return false for malformed URIs. +ARROW_EXPORT +bool IsLikelyUri(std::string_view path); + /// \defgroup filesystem-factories Functions for creating FileSystem instances /// /// @{ diff --git a/cpp/src/arrow/filesystem/path_util.cc b/cpp/src/arrow/filesystem/path_util.cc index dc82afd07e7a..e56788137319 100644 --- a/cpp/src/arrow/filesystem/path_util.cc +++ b/cpp/src/arrow/filesystem/path_util.cc @@ -337,6 +337,8 @@ bool IsEmptyPath(std::string_view v) { return true; } +} // namespace internal + bool IsLikelyUri(std::string_view v) { if (v.empty() || v[0] == '/') { return false; @@ -357,6 +359,8 @@ bool IsLikelyUri(std::string_view v) { return ::arrow::util::IsValidUriScheme(v.substr(0, pos)); } +namespace internal { + struct Globber::Impl { std::regex pattern_; diff --git a/cpp/src/arrow/filesystem/path_util.h b/cpp/src/arrow/filesystem/path_util.h index d49d9d2efa7f..d146a57fbe10 100644 --- a/cpp/src/arrow/filesystem/path_util.h +++ b/cpp/src/arrow/filesystem/path_util.h @@ -27,6 +27,10 @@ namespace arrow { namespace fs { + +// Declared in arrow/filesystem/filesystem.h, defined in path_util.cc. +ARROW_EXPORT bool IsLikelyUri(std::string_view path); + namespace internal { constexpr char kSep = '/'; @@ -159,8 +163,7 @@ std::string ToSlashes(std::string_view s); ARROW_EXPORT bool IsEmptyPath(std::string_view s); -ARROW_EXPORT -bool IsLikelyUri(std::string_view s); +inline bool IsLikelyUri(std::string_view s) { return ::arrow::fs::IsLikelyUri(s); } class ARROW_EXPORT Globber { public: diff --git a/python/pyarrow/_fs.pyx b/python/pyarrow/_fs.pyx index 0739b6acba32..0b8ab7480f83 100644 --- a/python/pyarrow/_fs.pyx +++ b/python/pyarrow/_fs.pyx @@ -84,6 +84,14 @@ def _file_type_to_string(ty): return f"{ty.__class__.__name__}.{ty._name_}" +def _is_likely_uri(path): + cdef c_string c_path + if not isinstance(path, str): + raise TypeError("Path must be a string") + c_path = tobytes(path) + return CIsLikelyUri(cpp_string_view(c_path)) + + cdef class FileInfo(_Weakrefable): """ FileSystem entry info. diff --git a/python/pyarrow/fs.py b/python/pyarrow/fs.py index 41308cd60c1d..bc68d3f2d7cd 100644 --- a/python/pyarrow/fs.py +++ b/python/pyarrow/fs.py @@ -33,6 +33,7 @@ PyFileSystem, _copy_files, _copy_files_selector, + _is_likely_uri, ) # For backward compatibility. @@ -82,32 +83,6 @@ def __getattr__(name): ) -def _is_likely_uri(path): - """ - Check if a string looks like a URI (has a valid scheme followed by ':'). - - This is a Python port of the C++ ``IsLikelyUri()`` heuristic in - ``cpp/src/arrow/filesystem/path_util.cc``. It is intentionally - conservative: single-letter schemes are excluded (could be a Windows - drive letter) and the scheme must conform to RFC 3986. - """ - if not path or path[0] == '/': - return False - colon_pos = path.find(':') - if colon_pos < 0: - return False - # One-letter URI schemes don't officially exist; may be a Windows drive - if colon_pos < 2: - return False - # The largest IANA-registered scheme is 36 characters - if colon_pos > 36: - return False - scheme = path[:colon_pos] - if not scheme[0].isalpha(): - return False - return all(c.isalnum() or c in ('+', '-', '.') for c in scheme[1:]) - - def _ensure_filesystem(filesystem, *, use_mmap=False): if isinstance(filesystem, FileSystem): return filesystem diff --git a/python/pyarrow/includes/libarrow_fs.pxd b/python/pyarrow/includes/libarrow_fs.pxd index d18dc2d2bde1..9e504625d67c 100644 --- a/python/pyarrow/includes/libarrow_fs.pxd +++ b/python/pyarrow/includes/libarrow_fs.pxd @@ -93,6 +93,8 @@ cdef extern from "arrow/filesystem/api.h" namespace "arrow::fs" nogil: "arrow::fs::FileSystemFromUriOrPath"(const c_string& uri, c_string* out_path) + c_bool CIsLikelyUri "arrow::fs::IsLikelyUri"(cpp_string_view path) + cdef cppclass CFileSystemGlobalOptions \ "arrow::fs::FileSystemGlobalOptions": c_string tls_ca_file_path diff --git a/python/pyarrow/tests/test_fs.py b/python/pyarrow/tests/test_fs.py index 1f3c96b8252f..a696f61c942d 100644 --- a/python/pyarrow/tests/test_fs.py +++ b/python/pyarrow/tests/test_fs.py @@ -1745,6 +1745,8 @@ def test_is_likely_uri(): assert not _is_likely_uri("C:/Users/foo") assert not _is_likely_uri("3bucket://key") # scheme starts with digit assert not _is_likely_uri("-scheme://key") # scheme starts with dash + assert not _is_likely_uri("schéme://bucket/key") # non-ASCII in scheme + assert not _is_likely_uri("漢字://bucket/key") # non-ASCII in scheme def test_resolve_filesystem_and_path_uri_with_spaces(): From 9e400f61de7b943fa134919939e481da6a2eb957 Mon Sep 17 00:00:00 2001 From: Ernest Provo Date: Tue, 19 May 2026 06:15:47 -0400 Subject: [PATCH 3/4] GH-41365: [Python] Fix Cython binding for IsLikelyUri MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cpp_string_view cppclass in common.pxd has known-broken constructor declarations (see cython/cython#6651 reference in common.pxd) — they parse as methods rather than constructors. Switch CIsLikelyUri to take const c_string&; C++17's implicit std::string -> std::string_view conversion handles the call site. --- python/pyarrow/_fs.pyx | 2 +- python/pyarrow/includes/libarrow_fs.pxd | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/python/pyarrow/_fs.pyx b/python/pyarrow/_fs.pyx index 0b8ab7480f83..cc40bf7271f6 100644 --- a/python/pyarrow/_fs.pyx +++ b/python/pyarrow/_fs.pyx @@ -89,7 +89,7 @@ def _is_likely_uri(path): if not isinstance(path, str): raise TypeError("Path must be a string") c_path = tobytes(path) - return CIsLikelyUri(cpp_string_view(c_path)) + return CIsLikelyUri(c_path) cdef class FileInfo(_Weakrefable): diff --git a/python/pyarrow/includes/libarrow_fs.pxd b/python/pyarrow/includes/libarrow_fs.pxd index 9e504625d67c..e119c6f0043f 100644 --- a/python/pyarrow/includes/libarrow_fs.pxd +++ b/python/pyarrow/includes/libarrow_fs.pxd @@ -93,7 +93,7 @@ cdef extern from "arrow/filesystem/api.h" namespace "arrow::fs" nogil: "arrow::fs::FileSystemFromUriOrPath"(const c_string& uri, c_string* out_path) - c_bool CIsLikelyUri "arrow::fs::IsLikelyUri"(cpp_string_view path) + c_bool CIsLikelyUri "arrow::fs::IsLikelyUri"(const c_string& path) cdef cppclass CFileSystemGlobalOptions \ "arrow::fs::FileSystemGlobalOptions": From 4947778448e5d41d6df0e9c5422372eca16a588d Mon Sep 17 00:00:00 2001 From: Ernest Provo Date: Sun, 19 Jul 2026 11:59:14 -0400 Subject: [PATCH 4/4] GH-41365: [Python] Add regression tests for non-ASCII URI paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewer feedback on apache/arrow#49372 asked whether non-ASCII characters in the path (e.g. "s3://asdf/äöü") are affected by the same silent LocalFileSystem fallback as un-encoded whitespace. They are. The URI parser rejects any byte outside the RFC 3986 character set, so a UTF-8 path raises "Cannot parse URI" exactly like a space does, and the pre-fix `_resolve_filesystem_and_path` swallowed it. Verified against the released resolver: the new non-ASCII test fails with the same "DID NOT RAISE ValueError" as the whitespace test, and both pass with the fix in this branch. No additional code change is needed - `IsLikelyUri` only inspects the scheme (everything before the first ':'), so non-ASCII bytes in the path never reach it and the scheme still validates. These tests are therefore regression coverage for a case the fix already handles. Added: - test_is_likely_uri: non-ASCII in the path is still a URI; non-ASCII local/relative paths are still not URIs - test_resolve_filesystem_and_path_uri_with_non_ascii: umlauts and CJK under s3/gs/abfss schemes must raise, not fall back - test_resolve_filesystem_and_path_local_with_non_ascii: absolute and relative non-ASCII local paths must still resolve to LocalFileSystem (the relative cases are the first coverage of the "Cannot parse URI and not a likely URI" branch) --- python/pyarrow/tests/test_fs.py | 65 +++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/python/pyarrow/tests/test_fs.py b/python/pyarrow/tests/test_fs.py index a696f61c942d..08c7dddca9a2 100644 --- a/python/pyarrow/tests/test_fs.py +++ b/python/pyarrow/tests/test_fs.py @@ -1737,6 +1737,12 @@ def test_is_likely_uri(): assert _is_likely_uri("abfss://container@account/path") assert _is_likely_uri("grpc+https://host:443") + # Only the scheme (everything before the first ':') is inspected, so + # non-ASCII characters in the *path* don't change the verdict. + assert _is_likely_uri("s3://asdf/äöü") + assert _is_likely_uri("s3://bucket/über/daten.parquet") + assert _is_likely_uri("s3://bucket/数据/file.parquet") + # Not URIs — local paths, Windows drives, empty, etc. assert not _is_likely_uri("") assert not _is_likely_uri("/absolute/path") @@ -1747,6 +1753,8 @@ def test_is_likely_uri(): assert not _is_likely_uri("-scheme://key") # scheme starts with dash assert not _is_likely_uri("schéme://bucket/key") # non-ASCII in scheme assert not _is_likely_uri("漢字://bucket/key") # non-ASCII in scheme + assert not _is_likely_uri("/tmp/äöü/data") # non-ASCII local path + assert not _is_likely_uri("dätä/file.parquet") # non-ASCII, no scheme def test_resolve_filesystem_and_path_uri_with_spaces(): @@ -1788,6 +1796,63 @@ def test_resolve_filesystem_and_path_local_with_spaces(): assert isinstance(fs, LocalFileSystem) +def test_resolve_filesystem_and_path_uri_with_non_ascii(): + """ + URIs with a recognised scheme but un-encoded non-ASCII characters must + raise ValueError — NOT silently fall back to LocalFileSystem. This is + the same failure mode as un-encoded spaces: the URI parser rejects any + byte outside the RFC 3986 character set, so such paths have to be + percent-encoded (e.g. "s3://asdf/%C3%A4%C3%B6%C3%BC"). + (GH-41365) + """ + from pyarrow.fs import _resolve_filesystem_and_path + + # S3 URI with umlauts should raise, not return LocalFileSystem + with pytest.raises(ValueError, match="Cannot parse URI"): + _resolve_filesystem_and_path("s3://asdf/äöü") + + with pytest.raises(ValueError, match="Cannot parse URI"): + _resolve_filesystem_and_path("s3://bucket/über/daten.parquet") + + # Non-Latin scripts fail the same way + with pytest.raises(ValueError, match="Cannot parse URI"): + _resolve_filesystem_and_path("s3://bucket/数据/file.parquet") + + # GCS URI with umlauts should also raise + with pytest.raises(ValueError, match="Cannot parse URI"): + _resolve_filesystem_and_path("gs://bucket/äöü/x.csv") + + # abfss URI with umlauts + with pytest.raises(ValueError, match="Cannot parse URI"): + _resolve_filesystem_and_path("abfss://container@account/äöü") + + +def test_resolve_filesystem_and_path_local_with_non_ascii(): + """ + Local paths (no scheme) containing non-ASCII characters should still + resolve to LocalFileSystem — they must NOT be confused with malformed + URIs. + """ + from pyarrow.fs import _resolve_filesystem_and_path + + # Absolute local path with umlauts → LocalFileSystem + fs, path = _resolve_filesystem_and_path("/tmp/äöü/data") + assert isinstance(fs, LocalFileSystem) + + # Absolute local path with a non-Latin script → LocalFileSystem + fs, path = _resolve_filesystem_and_path("/tmp/数据/data") + assert isinstance(fs, LocalFileSystem) + + # Relative non-ASCII paths do reach the URI parser and fail with + # "Cannot parse URI", but _is_likely_uri() rejects them as URIs, so + # they must still fall back to LocalFileSystem. + fs, path = _resolve_filesystem_and_path("dätä/file.parquet") + assert isinstance(fs, LocalFileSystem) + + fs, path = _resolve_filesystem_and_path("dätä/fi:le.parquet") + assert isinstance(fs, LocalFileSystem) + + @pytest.mark.s3 def test_filesystem_from_uri_s3(s3_server): from pyarrow.fs import S3FileSystem