Skip to content

ffi: create data source from memory buffer#8426

Merged
myrrc merged 1 commit into
developfrom
myrrc/ffi-read-from-buffer
Jun 15, 2026
Merged

ffi: create data source from memory buffer#8426
myrrc merged 1 commit into
developfrom
myrrc/ffi-read-from-buffer

Conversation

@myrrc

@myrrc myrrc commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Match rust API: buffer is borrowed under the condition it won't be modified
until data source is freed.

We need this for ClickHouse integration as it hands us memory buffers.

@myrrc myrrc requested review from a team and AdamGS June 15, 2026 12:27
@myrrc myrrc added the changelog/feature A new feature label Jun 15, 2026
@myrrc myrrc enabled auto-merge (squash) June 15, 2026 12:27
@codspeed-hq

codspeed-hq Bot commented Jun 15, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by 12.48%

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚡ 1 improved benchmark
✅ 1544 untouched benchmarks
⏩ 10 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation encode_varbin[(1000, 2)] 176.7 µs 157.1 µs +12.48%

Tip

Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.


Comparing myrrc/ffi-read-from-buffer (8429479) with develop (2528dca)

Open in CodSpeed

Footnotes

  1. 10 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.

@AdamGS AdamGS left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me, but maybe needs someone with more C/FFI opinions.

@myrrc myrrc requested a review from 0ax1 June 15, 2026 13:11

@0ax1 0ax1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of nits and questions.

Comment thread vortex-ffi/cinclude/vortex.h
Comment thread vortex-ffi/cinclude/vortex.h
Comment thread vortex-ffi/cinclude/vortex.h
Comment thread vortex-ffi/src/data_source.rs Outdated
Comment thread vortex-ffi/src/data_source.rs
Comment thread vortex-ffi/src/data_source.rs Outdated
Comment thread vortex-ffi/test/scan.cpp
Comment thread vortex-ffi/test/scan.cpp
Comment thread vortex-ffi/src/data_source.rs Outdated
@myrrc myrrc force-pushed the myrrc/ffi-read-from-buffer branch from a219a48 to 268bb06 Compare June 15, 2026 13:52
Signed-off-by: Mikhail Kot <mikhail@spiraldb.com>
@myrrc myrrc force-pushed the myrrc/ffi-read-from-buffer branch from 268bb06 to 8429479 Compare June 15, 2026 13:53
@myrrc myrrc merged commit d737639 into develop Jun 15, 2026
66 of 67 checks passed
@myrrc myrrc deleted the myrrc/ffi-read-from-buffer branch June 15, 2026 14:00
vortex_ensure!(!buffer.is_null());

let session = vx_session::as_ref(session);
let bytes: &'static [u8] =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this approach implies that the data is already in ram, but in ch read buffer is a streaming abstraction, including positional read api over s3/ssd etc. wouldn't it be more reasonable to exposeVortexReadAt to ffi?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but this is a separate work which is non-trivial as opposed to this function. I have it on my TODO list. The issue with this is that we already have an inverse abstraction (duckdbfs) which we probably want to pull out.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/feature A new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants