fix(python): treat a single-letter URL scheme as a filesystem path - #9957
Open
jackylee-ch wants to merge 1 commit into
Open
jackylee-ch wants to merge 1 commit into
jackylee-ch wants to merge 1 commit into
Conversation
`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>
Merging this PR will improve performance by 11.98%
|
| 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)
Footnotes
-
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Url::parseaccepts a Windows absolute path likeC:\data\file.vortexas a URL whose scheme is thedrive letter (
c), soresolve_store(vortex-python/src/object_store/resolve.rs:46) handed everyabsolute Windows path to the cloud registry and failed with an unrecognised scheme.
vx.open,vx.io.writeandvx.datasetall resolve through this one function, so absolute-path I/O was brokenon Windows.
The repository already made this decision, twice, with the reasoning written out:
vortex-file/src/multi/uri.rs:52— "Url::parseaccepts Windows absolute paths likeC:\fooas aURL with a single-letter scheme (
c). No real URL scheme is one character, so treat anysingle-letter scheme as a filesystem path instead", guarded by
url.scheme().len() > 1and pinnedby
test_single_letter_scheme_is_pathvortex-ffi/src/data_source.rs:85— the same guardvortex-python was never moved onto it. This applies the same rule, and folds the old
Err(_)arminto it since both mean "this is a local path".
Windows wheels are published (
package.yml:33,x86_64-pc-windows-msvc) and there is apython-windowsCI job, but its functional check round-trips the relative path"smoke.vortex"(
ci.yml:200), whichUrl::parserejects 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 coverbackslash 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
resolvecases 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.