Skip to content

fix(python): treat a single-letter URL scheme as a filesystem path - #9957

Open
jackylee-ch wants to merge 1 commit into
vortex-data:developfrom
jackylee-ch:fix/py-windows-drive-path
Open

jackylee-ch wants to merge 1 commit into
vortex-data:developfrom
jackylee-ch:fix/py-windows-drive-path

Conversation

@jackylee-ch

Copy link
Copy Markdown
Contributor

Url::parse accepts a Windows absolute path like C:\data\file.vortex as a URL whose scheme is the
drive letter (c), so resolve_store (vortex-python/src/object_store/resolve.rs:46) handed every
absolute Windows path to the cloud registry and failed with an unrecognised scheme. vx.open,
vx.io.write and vx.dataset all resolve through this one function, so absolute-path I/O was broken
on Windows.

The repository already made this decision, twice, with the reasoning written out:

  • vortex-file/src/multi/uri.rs:52 — "Url::parse accepts Windows absolute paths like C:\foo as a
    URL with a single-letter scheme (c). No real URL scheme is one character, so treat any
    single-letter scheme as a filesystem path instead", guarded by url.scheme().len() > 1 and pinned
    by test_single_letter_scheme_is_path
  • vortex-ffi/src/data_source.rs:85 — the same guard

vortex-python was never moved onto it. This applies the same rule, and folds the old Err(_) arm
into it since both mean "this is a local path".

Windows wheels are published (package.yml:33, x86_64-pc-windows-msvc) and there is a
python-windows CI job, but its functional check round-trips the relative path "smoke.vortex"
(ci.yml:200), which Url::parse rejects outright — so the drive-letter arm was never exercised.

Tests

cargo test --release -p vortex-python --lib: 35 passed, 31 before. The four new cases cover
backslash and forward-slash drive paths, a lowercase drive letter, and a bare drive root; the test
carries the same name as the vortex-file one. Removing the guard fails all four and leaves the other
sixteen resolve cases green.

pytest vortex-python/test: 340 passed, 2 skipped, 1 xfailed.

AI assistance

Written with agentic AI assistance; I found the two existing implementations of this guard and the
CI gap before changing anything.

`Url::parse` accepts a Windows absolute path like `C:\data\file.vortex` as a
URL whose scheme is the drive letter, so `resolve_store` handed every absolute
Windows path to the object-store registry and failed with an unrecognised
scheme. `vx.open`, `vx.io.write` and `vx.dataset` all resolve through this one
function.

The repository already made this decision twice — `parse_uri_or_path` in
vortex-file and `data_source.rs` in vortex-ffi both guard on
`url.scheme().len() > 1`, with the rationale written out. vortex-python was
never moved onto it.

The Windows Python CI job round-trips the relative path "smoke.vortex", which
`Url::parse` rejects outright, so the drive-letter arm was never exercised.

Signed-off-by: jackylee-ch <qcsd2011@gmail.com>
@codspeed

codspeed Bot commented Sep 21, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by 11.98%

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚡ 2 improved benchmarks
✅ 217 untouched benchmarks
⏩ 2252 skipped benchmarks1

Performance Changes

Benchmark BASE HEAD Efficiency
words_gather_scalar_avx2[65536] 9.4 µs 8.3 µs +13.2%
mul_u32_nonnull_avx512 6.2 µs 5.6 µs +10.78%

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.


Comparing jackylee-ch:fix/py-windows-drive-path (a0a6b75) with develop (895935e)

Open in CodSpeed

Footnotes

  1. 2252 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant