Improve URL parsing in VortexMultiFileScan to handle file paths more … by iceboundrock · Pull Request #6842 · vortex-data/vortex

iceboundrock · 2026-03-08T10:10:17Z

Summary

This PR fixes the issue that relative paths cause Neither URL nor path error

Closes: #6841

This PR utilizes AI (codex from OpenAI) to generate code.

API Changes

None

Testing

cargo test -p vortex-duckdb --lib

…robustly Fixes vortex-data#6841 where relative file paths caused a "Neither URL nor path" error. Relative paths are now resolved to absolute paths via `std::fs::canonicalize` before being converted to a `file://` URL. Glob patterns containing `*`, `?`, or `[` skip canonicalization because the pattern itself does not correspond to an existing path. The URL parsing logic is extracted into a `parse_glob_url` helper with unit tests covering s3 URLs, `file://` URLs, absolute glob patterns, absolute file paths, relative file paths, and the error case for non-existent paths. Signed-off-by: Ruoshi Li <iceboundrock@gmail.com>

- Extract URL parsing into parse_glob_url and path resolution into canonicalize_path_prefix for testability. - Fix relative glob paths (e.g. ./data/*.vortex): canonicalize only the directory prefix before the first glob character, then append the glob suffix, so relative patterns resolve to absolute file URLs. - Replace repeated generic "Neither URL nor path" errors with specific messages that include the underlying IO error (e.g. "No such file or directory"), making diagnosis easier. - Add unit tests covering s3/file URLs, absolute/relative paths, absolute/relative glob patterns, and the not-found error case. Signed-off-by: Ruoshi Li <iceboundrock@gmail.com>

Signed-off-by: Ruoshi Li <iceboundrock@gmail.com>

AdamGS · 2026-03-09T11:26:28Z

vortex-duckdb/src/multi_file.rs

+/// is canonicalized and the remaining glob suffix is appended unchanged. This allows relative glob
+/// patterns such as `./data/*.vortex` to be resolved correctly.
+fn canonicalize_path_prefix(path_str: &str) -> VortexResult<PathBuf> {
+    let first_glob_char_pos = path_str.find(['*', '?', '[']);


can we use the glob crate to detect these pattern? this feel brittle

I will use it in the future. For this PR, I will follow @myrrc 's comment to remove the implementation and just keep tests.

codspeed-hq · 2026-03-09T11:28:23Z

Merging this PR will degrade performance by 11%

⚡ 4 improved benchmarks
❌ 2 regressed benchmarks
✅ 994 untouched benchmarks
⏩ 1466 skipped benchmarks¹

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Mode	Benchmark	`BASE`	`HEAD`	Efficiency
⚡	Simulation	`patched_take_200k_first_chunk_only`	5.4 ms	4.8 ms	+11.97%
⚡	Simulation	`patched_take_200k_dispersed`	5.6 ms	4.7 ms	+19.86%
⚡	Simulation	`take_200k_dispersed`	4.5 ms	3.6 ms	+24.35%
⚡	Simulation	`take_200k_first_chunk_only`	4.2 ms	3.3 ms	+26.42%
❌	Simulation	`map_each[BufferMut<i32>, 128]`	770.6 ns	858.1 ns	-10.2%
❌	Simulation	`bitwise_not_vortex_buffer_mut[128]`	471.9 ns	530.3 ns	-11%

_{Comparing Lychee-Technology:develop (1b252df) with develop (0309e57)}

1466 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. ↩

myrrc

Duckdb resolves symlinks in its filesystem module, so we do a double job here with canonicalization. Same for manual glob checks. The error was that we needed to pass Url an absolute path which I've fixed in #6851. We do want to merge tests from this PR though, increased code coverage is always a good thing.

Can you please keep the tests but replace canonicalization/globbing checks with the solution present upstream?

iceboundrock · 2026-03-09T16:50:05Z

Duckdb resolves symlinks in its filesystem module, so we do a double job here with canonicalization. Same for manual glob checks. The error was that we needed to pass Url an absolute path which I've fixed in #6851. We do want to merge tests from this PR though, increased code coverage is always a good thing.

Can you please keep the tests but replace canonicalization/globbing checks with the solution present upstream?

Done

Merges upstream changes including PR vortex-data#6851 (duckdb: read from local path, correct err on invalid path). Resolves conflict in multi_file.rs by keeping the parse_glob_url wrapper function (with its tests) from this branch, but replacing the canonicalize_path_prefix implementation with the simpler std::path::absolute approach from upstream. Signed-off-by: iceboundrock <iceboundrock@outlook.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

myrrc · 2026-03-10T10:56:31Z

vortex-duckdb/src/multi_file.rs

+    }
+
+    #[test]
+    fn test_parse_glob_url_relative_glob_path() -> VortexResult<()> {


Thanks, I believe the PR is nearly ready.

Can you also add a test for relative paths like "../test.vortex", please?
I've tested and it fails, likely a flaw in my implementation.

$ duckdb -c 'SELECT * FROM "../vortex/vortex-bench/data/tpch/1.0/vortex-file-compressed/customer_0.vortex"' Binder Error: Object store error: Object at location /home/myrrc/duckdb-vortex/%2E%2E/vortex/vortex-bench/data/tpch/1.0/vortex-file-compressed/customer_0.vortex not found: No such file or directory (os error 2)

Root cause

std::path::absolute() makes a path absolute but does not normalize .. and/or . components. So ../test.vortex from /home/user/vortex/ becomes /home/user/vortex/../test.vortex, and Url::from_file_path then percent-encodes the dots as %2E%2E, causing DuckDB to look up a wrong/encoded path.

Fix

Added a normalize_path helper function that walks the path components and resolves . and .. without touching the filesystem. This is called after absolute() before creating the Url.

Tests

added: test_parse_glob_url_parent_relative_path — creates a temp file in .., constructs a ../filename relative path, and asserts the resulting URL is absolute, has no %2E encoding, and ends with the correct filename.

added test_parse_glob_url_dot_normalization, just test the normalize_path algorithm without creating any files.

Signed-off-by: Ruoshi Li <iceboundrock@gmail.com>

AdamGS added the changelog/fix A bug fix label Mar 8, 2026

AdamGS self-assigned this Mar 8, 2026

iceboundrock force-pushed the develop branch from 83c0d31 to aff7c79 Compare March 8, 2026 19:47

iceboundrock and others added 3 commits March 8, 2026 13:14

Make parse_glob_url module-private

854453b

Signed-off-by: Ruoshi Li <iceboundrock@gmail.com>

Merge branch 'develop' into develop

1b252df

AdamGS reviewed Mar 9, 2026

View reviewed changes

AdamGS requested a review from myrrc March 9, 2026 13:57

myrrc requested changes Mar 9, 2026

View reviewed changes

myrrc assigned myrrc and unassigned AdamGS Mar 9, 2026

myrrc added the ext/duckdb Relates to the DuckDB integration label Mar 9, 2026

iceboundrock and others added 2 commits March 9, 2026 10:03

Merge remote-tracking branch 'origin/develop' into develop

17d99b9

iceboundrock requested a review from myrrc March 9, 2026 17:19

iceboundrock added 2 commits March 9, 2026 10:23

Merge branch 'vortex-data:develop' into develop

05fabeb

Merge branch 'vortex-data:develop' into develop

2b514fa

myrrc reviewed Mar 10, 2026

View reviewed changes

iceboundrock and others added 2 commits March 10, 2026 12:38

Merge branch 'vortex-data:develop' into develop

20aed55

feat: enhance glob URL parsing and add normalization for relative paths

ab65460

Signed-off-by: Ruoshi Li <iceboundrock@gmail.com>

iceboundrock force-pushed the develop branch from 6d986c4 to ab65460 Compare March 10, 2026 20:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve URL parsing in VortexMultiFileScan to handle file paths more …#6842

Improve URL parsing in VortexMultiFileScan to handle file paths more …#6842
iceboundrock wants to merge 10 commits intovortex-data:developfrom
Lychee-Technology:develop

iceboundrock commented Mar 8, 2026 •

edited

Loading

Uh oh!

AdamGS Mar 9, 2026

Uh oh!

iceboundrock Mar 9, 2026

Uh oh!

codspeed-hq bot commented Mar 9, 2026 •

edited

Loading

Uh oh!

myrrc left a comment

Uh oh!

iceboundrock commented Mar 9, 2026 •

edited

Loading

Uh oh!

myrrc Mar 10, 2026

Uh oh!

iceboundrock Mar 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

iceboundrock commented Mar 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

API Changes

Testing

Uh oh!

AdamGS Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

iceboundrock Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

codspeed-hq bot commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will degrade performance by 11%

Performance Changes

Footnotes

Uh oh!

myrrc left a comment

Choose a reason for hiding this comment

Uh oh!

iceboundrock commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

myrrc Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

iceboundrock Mar 10, 2026

Choose a reason for hiding this comment

Root cause

Fix

Tests

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

iceboundrock commented Mar 8, 2026 •

edited

Loading

codspeed-hq bot commented Mar 9, 2026 •

edited

Loading

iceboundrock commented Mar 9, 2026 •

edited

Loading