8000 Fix S3 globbing for duckdbfs with httplib by myrrc · Pull Request #6709 · vortex-data/vortex · GitHub
[go: up one dir, main page]
Skip to content

Fix S3 globbing for duckdbfs with httplib#6709

Merged
myrrc merged 1 commit intodevelopfrom
myrrc/duckdbfs-glob-fix
Feb 27, 2026
Merged

Fix S3 globbing for duckdbfs with httplib#6709
myrrc merged 1 commit intodevelopfrom
myrrc/duckdbfs-glob-fix

Conversation

@myrrc
Copy link
Contributor
@myrrc myrrc commented Feb 27, 2026 •
edited
Loading

Using vortex_filesystem=duckdb, both of httpfs backends are broken
when using globs on S3 files:

  1. httplib was broken because duckdb folder list API returns full paths for
    S3 files, and we assumed relative paths. This PR fixes this behavior
  2. curl was broken because of Fix incorrect AWS list request in 1.4 duckdb/duckdb-httpfs#265
    in 1.4. Up until upstream fixes this, globbing won't work.
    This issue is not present in 1.5 anymore

Because of this bug, our benchmarks fail to run with curl backend, see #6685

@myrrc myrrc requested a review from joseph-isaacs February 27, 2026 15:50
@myrrc myrrc added the changelog/fix A bug fix label Feb 27, 2026
@myrrc myrrc force-pushed the myrrc/duckdbfs-glob-fix branch 2 times, most recently from bbba2b6 to 8fea279 Compare February 27, 2026 15:53
@myrrc myrrc mentioned this pull request Feb 27, 2026
Open
@codspeed-hq
Copy link
codspeed-hq bot commented Feb 27, 2026 •
edited
Loading

Merging this PR will improve performance by 17.64%

⚡ 1 improved benchmark
✅ 953 untouched benchmarks
⏩ 1466 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
⚡ Simulation chunked_opt_bool_canonical_into[(10, 1000)] 1.6 ms 1.4 ms +17.64%

Comparing myrrc/duckdbfs-glob-fix (ccbe879) with develop (fc3af37)

Open in CodSpeed

Footnotes

  1. 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
fix globbing for duckdbfs+httplib
ccbe879 
Signed-off-by: Mikhail Kot <mikhail@spiraldb.com>
@myrrc myrrc force-pushed the myrrc/duckdbfs-glob-fix branch from 8fea279 to ccbe879 Compare February 27, 2026 16:04
@myrrc myrrc requested review from 0ax1 and removed request for joseph-isaacs February 27, 2026 16:31
@myrrc myrrc enabled auto-merge (squash) February 27, 2026 16:46
@0ax1
Copy link
Contributor
0ax1 commented Feb 27, 2026

@claude review

@claude
Copy link
Contributor
claude bot commented Feb 27, 2026 •
edited
Loading

Claude finished @0ax1's task in 1m 20s —— View job

Code Review

  • Gather context on the PR
  • Read changed files
  • Analyze changes and provide review feedback

The fix is straightforward and correct. The root cause is well-identified: DuckDB's ListFiles returns full paths (e.g., s3://bucket/prefix/file.parquet) for remote filesystems but only entry names (relative to the directory being listed) for local files. The old code always joined dir + "/" + entry.name, which double-prefixed remote entries.

vortex-duckdb/src/filesystem.rs

Bug: silent fallback on strip_prefix failure (line 197–199)

let relative_path = full_path
    .strip_prefix(&base_path)
    .unwrap_or_else(|| &full_path)  // ← silently returns a full S3 URL
    .to_string();

If strip_prefix fails (e.g., base_url has a trailing slash that doesn't match, or the bucket/path doesn't align), relative_path ends up being the raw full S3 URL. Downstream callers then attempt to open e.g. s3://bucket/prefix/file.parquet as a relative path, which silently produces a wrong URL. This should at least log a warning, or return an error:

let relative_path = full_path
    .strip_prefix(&base_path)
    .ok_or_else(|| vortex_err!(
        "Listed path {full_path:?} does not share prefix {base_path:?}"
    ))?
    .to_string();

Fix this →

Minor: "really ugly" comment (lines 172–175)

The comment is honest but could be clearer. What's actually happening is that Url::to_string() for s3://bucket/path doesn't include a trailing slash, but listed entries will be s3://bucket/path/file, so we need s3://bucket/path/ to strip cleanly. Consider replacing the comment with something like:

// Url::to_string() for s3://bucket/path has no trailing slash,
// but listed entries are s3://bucket/path/file — add "/" so strip_prefix works.
(format!("{base_url}/"), true)

Minor: removed tracing

The tracing::debug! call in list() was removed and not re-added inside list_recursive. It's useful for diagnosing issues in production — consider adding a tracing::debug!("Listing {directory_url} with base {base_url}") at the top of list_recursive.

vortex-duckdb/src/duckdb/file_system.rs

Good: The updated doc comment on DirEntry::name and duckdb_fs_list_dir accurately describes what DuckDB actually returns. This is an important correction since the old docs were misleading and directly led to the bug.

vortex-duckdb/cpp/include/duckdb_vx/file_system.h

The updated @param name doc comment is correct. Minor nit: the new comment is missing a period at the end, unlike the @param is_dir and @param user_data lines.

Missing tests

There are no new tests for the is_remote_path branching logic in list_recursive. While testing against real S3 is hard in CI, the path-handling logic itself (constructing full_path, stripping base_path) could be unit-tested with a fake list of entries and a mock. That said, this is an integration-level fix and it's understandable to skip for now.

Summary

The fix is correct and addresses the root cause cleanly. The one issue worth fixing before merge is the silent unwrap_or_else fallback — it could produce confusing downstream errors rather than a clear "bad prefix" error here. Everything else is minor polish.

@myrrc myrrc merged commit 0abfb33 into develop Feb 27, 2026
51 checks passed
@myrrc myrrc deleted the myrrc/duckdbfs-glob-fix branch February 27, 2026 17:30
fastio pushed a commit to fastio/vortex that referenced this pull request Mar 10, 2026
@myrrc
Fix S3 globbing for duckdbfs with httplib (vortex-data#6709)
97d843c 
Using vortex_filesystem=duckdb, both of httpfs backends are broken
when using globs on S3 files:

1. httplib was broken because duckdb folder list API returns full paths
for
   S3 files, and we assumed relative paths. This PR fixes this behavior
2. curl was broken because of
duckdb/duckdb-httpfs#265
   in 1.4. Up until upstream fixes this, globbing won't work.
   This issue is not present in 1.5 anymore

Because of this bug, our benchmarks fail to run with curl backend, see
vortex-data#6685

Signed-off-by: Mikhail Kot <mikhail@spiraldb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@0ax1 0ax1 0ax1 approved these changes

Assignees

No one assigned

Labels

changelog/fix A bug fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@myrrc @0ax1
0