feat: implement DuckDB filesystem integration for Vortex file handling #6198
Conversation
vortex-duckdb/src/scan.rs
Outdated
| "http" | "https" | "s3" => { | ||
| let reader = open_duckdb_reader(client_ctx, &url); | ||
|
|
||
| // Fallback to the legacy object_store path for s3 if DuckDB fs isn't configured. |
There was a problem hiding this comment.
when will that happen? I would really love to get rid of object_store here, it has some underlying dependencies on tokio that can cause some weird issues
There was a problem hiding this comment.
HTTP/HTTPS/S3 now route solely through DuckDB FS; legacy object_store fallback removed, and failures now bubble as errors if httpfs ext of DuckDB is not initialized.
joseph-isaacs
added
the
action/benchmark
CodSpeed Performance ReportMerging this PR will degrade performance by 45.74%Comparing
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| 🆕 | WallTime | 10M_50pct[5000000] |
N/A | 278.7 µs | N/A |
| 🆕 | WallTime | 1M_90pct[1000000] |
N/A | 35.2 µs | N/A |
| 🆕 | WallTime | 1M_50pct[500000] |
N/A | 50.9 µs | N/A |
| 🆕 | WallTime | 10M_90pct[10000000] |
N/A | 363 µs | N/A |
| 🆕 | WallTime | 10M_10pct[1000000] |
N/A | 132.4 µs | N/A |
| ❌ | WallTime | u16_FoR[10M] |
8.3 µs | 10.2 µs | -18.59% |
| 🆕 | WallTime | 10M_10pct[1000000] |
N/A | 219.9 µs | N/A |
| ❌ | WallTime | u8_FoR[10M] |
6.9 µs | 12.8 µs | -45.74% |
| 🆕 | WallTime | 1M_50pct[500000] |
N/A | 24.5 µs | N/A |
| 🆕 | WallTime | 1M_90pct[1000000] |
N/A | 57 µs | N/A |
| 🆕 | WallTime | 10M_50pct[5000000] |
N/A | 278.7 µs | N/A |
| 🆕 | WallTime | 1M_90pct[1000000] |
N/A | 57.6 µs | N/A |
| 🆕 | WallTime | 1M_50pct[500000] |
N/A | 50.6 µs | N/A |
| 🆕 | WallTime | 1M_10pct[100000] |
N/A | 23.9 µs | N/A |
| 🆕 | WallTime | 10M_90pct[10000000] |
N/A | 363.4 µs | N/A |
| 🆕 | WallTime | 10M_10pct[1000000] |
N/A | 219.8 µs | N/A |
| 🆕 | WallTime | 10M_50pct[5000000] |
N/A | 158.2 µs | N/A |
| 🆕 | WallTime | 1M_10pct[100000] |
N/A | 76.7 µs | N/A |
| 🆕 | WallTime | 10M_90pct[10000000] |
N/A | 195.8 µs | N/A |
| 🆕 | WallTime | 1M_10pct[100000] |
N/A | 45.6 µs | N/A |
| ... | ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
Footnotes
-
No successful run was found on
develop(db2e2af) during the generation of this report, so 68130ce was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩ -
1323 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
added
action/benchmark-sql
joseph-isaacs
added
action/benchmark-sql
0ax1
left a comment
0ax1
left a comment
There was a problem hiding this comment.
Thanks for taking the time to look into this. There's a couple of issues around thread safety / locking, error handling and memory leaks.
If you used AI to assist in writing the code for your PR please mention this in your PR description (see: https://github.com/vortex-data/vortex/blob/develop/CONTRIBUTING.md).
vortex-duckdb/src/copy.rs
Outdated
| SESSION.write_options().write(&mut file, array_stream).await | ||
| // Prefer DuckDB FS (httpfs/s3/etc.), fallback to local async fs if unavailable. | ||
| let ctx_raw = ctx_ptr.0 as cpp::duckdb_vx_client_context; | ||
| if let Ok(writer) = unsafe { duckdb_fs_create_writer(ctx_raw, &file_path) } { |
There was a problem hiding this comment.
We should probably only use the DuckDB FS and not fallback to our prev logic implicitly but return an error if the new solution fails to init. We can handle this though in a follow up on our end.
There was a problem hiding this comment.
Thanks for all your comments, I will address them ASAP.
vortex-duckdb/cpp/file_system.cpp
Outdated
| *error_out = duckdb_vx_error_create(message.data(), message.size()); | ||
| } | ||
|
|
||
| duckdb_state HandleException(duckdb_vx_error *error_out) { |
There was a problem hiding this comment.
This snippet does not handle an exception really, right? We take the error, call throw unconditionally ourselves and then return a DuckDBError.
There was a problem hiding this comment.
added HandleException that rethrows the captured std::exception_ptr, assigns a duckdb_vx_error, and returns DuckDBError; all catch-all blocks now pass std::current_exception() to it.
| if (!error_out) { | ||
| return; | ||
| } | ||
| *error_out = duckdb_vx_error_create(message.data(), message.size()); |
There was a problem hiding this comment.
Where does this error get freed?
There was a problem hiding this comment.
C++ allocates errors via duckdb_vx_error_create; Rust side consistently frees with duckdb_vx_error_free in fs_error.
vortex-duckdb/cpp/file_system.cpp
Outdated
|
|
||
| using namespace duckdb; | ||
|
|
||
| namespace { |
There was a problem hiding this comment.
removed the anonymous namespace
vortex-duckdb/cpp/file_system.cpp
Outdated
| handle->Truncate(0); | ||
| return reinterpret_cast<duckdb_vx_file_handle>(new FileHandleWrapper(std::move(handle))); | ||
| } catch (...) { | ||
| HandleException(error_out); |
There was a problem hiding this comment.
Did you mean to forward the exception being passed to catch to HandleException?
There was a problem hiding this comment.
retained SetError for central null-guarded assignment; it now directly uses duckdb_vx_error_create, and all call sites route through it.
|
|
||
| /// A VortexReadAt implementation backed by DuckDB's filesystem (e.g., httpfs/s3). | ||
| pub struct DuckDbFsReadAt { | ||
| handle: Arc<Mutex<FsFileHandle>>, |
There was a problem hiding this comment.
Why does this need a Mutex given that FsFileHandle is Send and Sync? If it is send and sync Arc<FsFileHandle> should be sufficient.
There was a problem hiding this comment.
handle is now Arc<FsFileHandle> without additional locking
| max_size: 8 * 1024 * 1024, // 8 MB | ||
| }; | ||
|
|
||
| const DEFAULT_CONCURRENCY: usize = 64; |
There was a problem hiding this comment.
Can you give more context, add a comment how you came up with this number?
There was a problem hiding this comment.
set DEFAULT_CONCURRENCY = 64 as a conservative ceiling that keeps range reads parallel while staying under common per-host limits.
| let mut size_out: cpp::idx_t = 0; | ||
| let status = unsafe { | ||
| cpp::duckdb_vx_fs_get_size( | ||
| handle.lock().as_ptr(), |
There was a problem hiding this comment.
We only lock the handle for the duration of retrieving the pointer but not for the duration of duckdb_vx_fs_get_size. But as questioned before, isn't the file handle thread safe (send + sync) anyway?
There was a problem hiding this comment.
handle is now Arc<FsFileHandle> without additional locking; Send/Sync markers retained.
| let mut out_len: cpp::idx_t = 0; | ||
| let status = unsafe { | ||
| cpp::duckdb_vx_fs_read( | ||
| handle.lock().as_ptr(), |
There was a problem hiding this comment.
Same here, if we really need a lock, it would need to held for duration of duckdb_vx_fs_read.
vortex-duckdb/src/copy.rs
Outdated
| pub struct VortexCopyFunction; | ||
|
|
||
| #[derive(Clone, Copy)] | ||
| struct SendableClientCtx(usize); |
There was a problem hiding this comment.
Why do we encode the pointer as a usize?
There was a problem hiding this comment.
replaced usize casting with NonNull<duckdb_vx_client_context_> (SendableClientCtx) and pass the raw pointer via as_ptr(). This avoids truncation/aliasing issues from integer casts and preserves provenance; all FFI touchpoints now use the typed wrapper.
5 participants
I tested it locally