GH-41365: [Python] Fix S3 URI with whitespace silently falling back to LocalFileSystem by ernestprovo23 · Pull Request #49372 · apache/arrow

ernestprovo23 · 2026-02-22T19:10:23Z

Rationale

When _resolve_filesystem_and_path() receives an S3 URI containing spaces (e.g. s3://bucket/path with space/file), the C++ URI parser raises ValueError("Cannot parse URI"). The Python code previously caught all "Cannot parse URI" errors and silently fell back to LocalFileSystem, producing a confusing downstream error instead of the real parsing failure.

What changed

Added _is_likely_uri() helper — a Python port of the C++ IsLikelyUri() heuristic from cpp/src/arrow/filesystem/path_util.cc. It checks whether a string has a valid URI scheme prefix (2-36 chars, RFC 3986 compliant).
Modified error handling in _resolve_filesystem_and_path() — the except ValueError block now distinguishes:
- "empty scheme" → still falls back to LocalFileSystem (no scheme at all)
- "Cannot parse URI" + _is_likely_uri() returns False → still falls back (not a URI, just a local path)
- "Cannot parse URI" + _is_likely_uri() returns True → re-raises the error (it IS a URI, just malformed)
Changed raise e to raise — preserves the original traceback (minor style improvement).

Tests added

test_is_likely_uri() — unit tests for the heuristic covering valid schemes (s3://, gs://, hdfs://, abfss://, grpc+https://), local paths, Windows drives, and invalid prefixes.
test_resolve_filesystem_and_path_uri_with_spaces() — verifies S3/GCS/ABFSS URIs with spaces now raise ValueError.
test_resolve_filesystem_and_path_local_with_spaces() — verifies local paths with spaces still return LocalFileSystem.

Test plan

All new tests pass (14/14)
Manual verification: _resolve_filesystem_and_path("s3://bucket/path with space") raises ValueError
Manual verification: _resolve_filesystem_and_path("/local/path with space") returns LocalFileSystem
Existing test_fs.py tests pass (requires full C++ build environment)

GitHub Issue: [Python] Pyarrow fs incorrectly resolves S3 URIs with white space as a local path #41365

github-actions · 2026-02-22T19:10:46Z

⚠️ GitHub issue #41365 has been automatically assigned in GitHub to PR creator.

rok · 2026-02-22T20:15:30Z

Hey @ernestprovo23, thanks for working on this and opening the PR. I'll kick off the CI run and hopefully I or someone else wil find time early next week to do a review.

rok · 2026-02-22T20:17:41Z

    )


+def _is_likely_uri(path):


You mention on the issue:

Working on a fix that ports the C++ IsLikelyUri() heuristic to Python and uses it in _resolve_filesystem_and_path()

Could you check if it's possible to expose IsLikelyUri through Python so ultimately the logic is only implemented once and is always doing the same?

So I think calling the C++ function would be preferable. The change to expose the functionallity could look like:

diff --git i/cpp/src/arrow/filesystem/filesystem.h w/cpp/src/arrow/filesystem/filesystem.h index 3a47eb62f5..85ee3ff114 100644 --- i/cpp/src/arrow/filesystem/filesystem.h +++ w/cpp/src/arrow/filesystem/filesystem.h @@ -23,6 +23,7 @@ #include <iosfwd> #include <memory> #include <string> +#include <string_view> #include <utility> #include <vector> @@ -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 i/cpp/src/arrow/filesystem/path_util.cc w/cpp/src/arrow/filesystem/path_util.cc index dc82afd07e..cdc1807781 100644 --- i/cpp/src/arrow/filesystem/path_util.cc +++ w/cpp/src/arrow/filesystem/path_util.cc @@ -28,8 +28,28 @@ #include "arrow/util/uri.h" namespace arrow { - namespace fs { + +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)); +} + namespace internal { // XXX How does this encode Windows UNC paths? @@ -337,26 +357,6 @@ bool IsEmptyPath(std::string_view v) { return true; } diff --git i/cpp/src/arrow/filesystem/path_util.h w/cpp/src/arrow/filesystem/path_util.h index d49d9d2efa..2f767fc7dc 100644 --- i/cpp/src/arrow/filesystem/path_util.h +++ w/cpp/src/arrow/filesystem/path_util.h @@ -27,6 +27,10 @@ namespace arrow { namespace fs { + +ARROW_EXPORT +bool IsLikelyUri(std::string_view s); + 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 i/python/pyarrow/_fs.pyx w/python/pyarrow/_fs.pyx index 0739b6acba..0b8ab7480f 100644 --- i/python/pyarrow/_fs.pyx +++ w/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 i/python/pyarrow/fs.py w/python/pyarrow/fs.py index 41308cd60c..b0acb97d94 100644 --- i/python/pyarrow/fs.py +++ w/python/pyarrow/fs.py @@ -31,6 +31,7 @@ from pyarrow._fs import ( # noqa _MockFileSystem, FileSystemHandler, PyFileSystem, + _is_likely_uri, _copy_files, _copy_files_selector, ) @@ -82,32 +83,6 @@ def __getattr__(name): ) diff --git i/python/pyarrow/includes/libarrow_fs.pxd w/python/pyarrow/includes/libarrow_fs.pxd index af01c47c8c..48604972d2 100644 --- i/python/pyarrow/includes/libarrow_fs.pxd +++ w/python/pyarrow/includes/libarrow_fs.pxd @@ -92,6 +92,7 @@ cdef extern from "arrow/filesystem/api.h" namespace "arrow::fs" nogil: CResult[shared_ptr[CFileSystem]] CFileSystemFromUriOrPath \ "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": diff --git i/python/pyarrow/tests/test_fs.py w/python/pyarrow/tests/test_fs.py index 537ceb9999..31f3f8a8ca 100644 --- i/python/pyarrow/tests/test_fs.py +++ w/python/pyarrow/tests/test_fs.py @@ -1715,6 +1715,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

Does that make sense?

ernestprovo23 · 2026-03-01T22:29:34Z

@rok friendly ping on this when you get a chance. the ARM64 macOS 14 failure looks like a CI infra issue (missing Paginator.h header from the AWS SDK on the runner), not related to this change.

rok · 2026-03-03T17:31:22Z

Hey @ernestprovo23 ! The Paginator.h issue is unrelated.

conor-f · 2026-03-10T17:27:57Z

I see from the test cases that non-ASCII characters in the path are not currently accounted for. Would it be worthwhile to add this case to the added unit test cases? I have recently noticed S3 URLs with umlauts in the path being parsed as filesystem URLs and it looks like this could be the same cause!

e.g. s3://asdf/äöü

ernestprovo23 · 2026-03-19T13:19:55Z

thanks for the detailed walkthrough on the C++/Cython approach — makes a lot more sense than reimplementing the logic in Python. working on reworking the PR to expose IsLikelyUri through the binding. will push an update soon.

ernestprovo23 · 2026-05-19T10:16:55Z

@rok Pushed an update for the Cython binding issue causing the build failures.

The cpp_string_view cppclass in python/pyarrow/includes/common.pxd has the known-broken constructor declarations referenced by the cython/cython#6651 comment right above it — the inner declarations are named string_view(...) rather than cpp_string_view(...) so Cython parses them as methods, not constructors. Result: cpp_string_view(c_path) failed with no constructor found for C++ type 'cpp_string_view' and then crashed the Cython compiler downstream.

Switched the CIsLikelyUri binding to take const c_string&; C++17's implicit std::string -> std::string_view conversion handles the call at the C++ level, so the actual C++ signature is unchanged. Verified locally that Cython now compiles _fs.pyx cleanly. Also rebased on main.

Two remaining CI failures look pre-existing and unrelated to this PR — flagging in case it saves you a click:

AMD64 Ubuntu Meson — cpp/src/arrow/extension/meson.build:27 references a missing fixed_shape_tensor_test.cc
ARM64 macOS 14 * — cpp/src/arrow/flight/transport/grpc/serialization_internal.cc:356 nodiscard warning treated as error

Happy to add an inline comment in _fs.pyx referencing cython/cython#6651 if you'd prefer the workaround documented at the call site.

Reviewer feedback on apache#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)

pitrou · 2026-07-21T14:08:00Z

+// Declared in arrow/filesystem/filesystem.h, defined in path_util.cc.
+ARROW_EXPORT bool IsLikelyUri(std::string_view path);


Why declare it a second time? Let's choose one location and only one.

pitrou · 2026-07-21T14:08:19Z

-ARROW_EXPORT
-bool IsLikelyUri(std::string_view s);
+inline bool IsLikelyUri(std::string_view s) { return ::arrow::fs::IsLikelyUri(s); }


pitrou · 2026-07-21T14:10:54Z

+    assert isinstance(fs, LocalFileSystem)
+
+
+def test_resolve_filesystem_and_path_uri_with_non_ascii():


Can you fold this and test_resolve_filesystem_and_path_uri_with_spaces together?

pitrou · 2026-07-21T14:11:10Z

+        _resolve_filesystem_and_path("abfss://container@account/äöü")
+
+
+def test_resolve_filesystem_and_path_local_with_non_ascii():


Can you fold this and test_resolve_filesystem_and_path_local_with_spaces together?

…g 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.

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

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.

Reviewer feedback on apache#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)

…aration, fold URI tests Declare IsLikelyUri only in filesystem.h and drop both the duplicate declaration and the internal forwarding shim from path_util.h. In-tree callers now use the unqualified arrow::fs::IsLikelyUri; every one of them already reaches filesystem.h through its own filesystem header. Fold the space and non-ASCII cases of the resolve tests into two parametrized tests instead of four near-identical ones.

ernestprovo23 · 2026-08-23T18:05:40Z

thanks @pitrou, all four done.

dropped the second declaration, arrow::fs::IsLikelyUri now lives only in filesystem.h. also killed the internal:: inline forwarder you flagged... that was only there so i wouldn't have to touch callers, which isn't a good reason. the 7 in-tree call sites (mockfs, localfs, filesystem, s3fs x2, azurefs, gcsfs) call it unqualified now and they each already pull filesystem.h in through their own fs header, so no new includes.

folded both test pairs, the spaces and non-ascii cases are two parametrized tests now instead of four near-identical ones.

rebased on main. heads up the workflow runs are sitting at action_required so nothing's actually built on this sha yet.

pitrou · 2026-08-25T14:03:27Z


-ARROW_EXPORT
-bool IsLikelyUri(std::string_view s);
-


Ok, I would rather the keep the declaration here, as "path utilities" is really where this function belongs IMHO.

Also, it can stay in the internal namespace, as that will still be visible from PyArrow AFAIK.

Works for me. i can handle later today

ernestprovo23 requested review from AlenkaF, raulcd and rok as code owners February 22, 2026 19:10

github-actions Bot added Component: Python awaiting review Awaiting review labels Feb 22, 2026

rok reviewed Feb 22, 2026

View reviewed changes

github-actions Bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Feb 23, 2026

github-actions Bot added Component: C++ awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 7, 2026

ernestprovo23 force-pushed the fix-s3-uri-whitespace-resolution branch from ab5292d to 9e400f6 Compare May 19, 2026 10:16

ernestprovo23 requested a review from pitrou as a code owner July 19, 2026 16:03

pitrou reviewed Jul 21, 2026

View reviewed changes

ernestprovo23 added 5 commits August 23, 2026 11:28

ernestprovo23 force-pushed the fix-s3-uri-whitespace-resolution branch from 4947778 to aaf321b Compare August 23, 2026 15:29

pitrou reviewed Aug 25, 2026

View reviewed changes

		// Declared in arrow/filesystem/filesystem.h, defined in path_util.cc.
		ARROW_EXPORT bool IsLikelyUri(std::string_view path);

		assert isinstance(fs, LocalFileSystem)


		def test_resolve_filesystem_and_path_uri_with_non_ascii():

		_resolve_filesystem_and_path("abfss://container@account/äöü")


		def test_resolve_filesystem_and_path_local_with_non_ascii():

Conversation

ernestprovo23 commented Feb 22, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale

What changed

Tests added

Test plan

Uh oh!

github-actions Bot commented Feb 22, 2026

Uh oh!

rok commented Feb 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ernestprovo23 commented Mar 1, 2026

Uh oh!

rok commented Mar 3, 2026

Uh oh!

conor-f commented Mar 10, 2026

Uh oh!

ernestprovo23 commented Mar 19, 2026

Uh oh!

ernestprovo23 commented May 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ernestprovo23 commented Aug 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ernestprovo23 commented Feb 22, 2026 •

edited by github-actions Bot

Loading