Fix S3 globbing for duckdbfs with httplib

[myrrc]

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

[codspeed-hq]

Merging this PR will improve performance by 17.64%

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

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)}

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

[0ax1]

@claude review

[claude]

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.