feat(c): add write/commit C FFI bindings#522
Conversation
Add write-side C FFI bindings following the existing read-side patterns: API surface (14 functions): - WriteBuilder: new_write_builder, free, with_overwrite, new_write, new_commit - TableWrite: free, write_arrow_batch (via Arrow C Data Interface), prepare_commit - TableCommit: free, commit, overwrite, truncate_table, abort - CommitMessages: free Tests (15 cases): - Read path: empty table read, data read, column projection - Predicate: equal, IS NOT NULL, IS IN, less_than filter, AND combinator - Write path: builder lifecycle, full roundtrip, multiple batches, empty commit, overwrite mode, truncate, abort - Error handling: null pointer checks - Integration: two commits with same builder Design follows Python bindings: in-process commit message transfer, shared commit_user between writer and committer, Arrow C Data Interface for zero-copy batch import. Closes apache#520
|
|
||
| let table_write = &mut *((*tw).inner as *mut TableWrite); | ||
|
|
||
| let ffi_array = ptr::read(array as *const FFI_ArrowArray); |
There was a problem hiding this comment.
ptr::read does not clear the caller's release callback. The imported copy owns the same resources while the original struct still appears valid, which can result in a double-free.
| let ffi_array = ptr::read(array as *const FFI_ArrowArray); | ||
| let ffi_schema = ptr::read(schema as *const FFI_ArrowSchema); | ||
|
|
||
| let batch = match unsafe { from_ffi(ffi_array, &ffi_schema) } { |
There was a problem hiding this comment.
Non-Struct or nullable root arrays can panic here. The imported schema is also not checked against the table schema, so an incompatible schema may be written directly into data files.
| } | ||
| let table_write = &mut *((*tw).inner as *mut TableWrite); | ||
|
|
||
| match runtime().block_on(table_write.prepare_commit()) { |
There was a problem hiding this comment.
The messages do not retain their source table or commit_user. Messages produced for one table can therefore be committed to another table, creating a snapshot that references files under the wrong location.
| let table_commit = &*((*tc).inner as *const TableCommit); | ||
| let messages = Box::from_raw((*msgs).inner as *mut Vec<CommitMessage>); | ||
| // Release the outer wrapper to avoid double-free when caller frees msgs | ||
| drop(Box::from_raw(msgs)); |
There was a problem hiding this comment.
This destroys the messages handle even though the API documentation requires callers to free it explicitly. Freeing it after commit becomes a use-after-free, and a failed commit cannot be retried or aborted.
| return ptr::null_mut(); | ||
| } | ||
|
|
||
| match runtime().block_on(table_commit.abort(&messages)) { |
There was a problem hiding this comment.
prepare_commit can produce new_index_files, but the current abort implementation only deletes data and changelog files. Dynamic-bucket aborts leave index files behind.
| }; | ||
| } | ||
| let table_ref = &*((*table).inner as *const Table); | ||
| let wb = table_ref.new_write_builder(); |
There was a problem hiding this comment.
The generated commit_user cannot be supplied by the caller, and the API does not accept a commit_identifier. Separate writers cannot share commit identity, and uncertain commit results cannot be retried idempotently.
2 participants
Summary
Add write-side C FFI bindings (
bindings/c/src/write.rs) following the same opaque handle /extern "C"patterns as the existing read-side bindings. Closes #520.API surface (14 functions)
Design decisions
FFI_ArrowArray/FFI_ArrowSchemaraw pointersTests (15 cases, all passing)
Run:
cargo +1.91.0 test -p paimon-c