Skip to content

Conversation

@zachschuermann
Copy link
Member

@zachschuermann zachschuermann commented Nov 27, 2024

What changes are proposed in this pull request?

  1. all new FFI for Transaction including creation, adding commit info, adding write metadata, and committing
  2. new default engine FFI for writing parquet
  3. write_table C example

This PR affects the following public APIs

TODO

How was this change tested?

  1. new C example write_table
  2. TODO more rust tests with miri enabled

@codecov
Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 0% with 90 lines in your changes missing coverage. Please review.

Project coverage is 83.05%. Comparing base (c3a868f) to head (bbf0a86).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
ffi/src/lib.rs 0.00% 72 Missing ⚠️
ffi/src/engine_data.rs 0.00% 17 Missing ⚠️
ffi/src/error.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #550      +/-   ##
==========================================
- Coverage   83.45%   83.05%   -0.40%     
==========================================
  Files          74       74              
  Lines       16877    17009     +132     
  Branches    16877    17009     +132     
==========================================
+ Hits        14084    14127      +43     
- Misses       2135     2225      +90     
+ Partials      658      657       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot added the breaking-change Change that require a major version bump label Nov 27, 2024

#[cfg(feature = "default-engine")]
#[no_mangle]
pub unsafe extern "C" fn get_engine_data(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Seems we're passing in ownership. Should we call this into_engine_data?

@zachschuermann
Copy link
Member Author

closing in favor of #962

OussamaSaoudi pushed a commit that referenced this pull request Jul 29, 2025
This PR is a retake of
#550

## What changes are proposed in this pull request?
This PR implements the ffi functions required to do appends. The main
difference over #550 is
how it's tested: instead of using a C based example we now opt for a
rust-based test that is able to re-used various testing-related utility
functions that are used in the kernel tests. To do this cleanly, i've
moved several functions over to the test-utils crate.

## This PR affects the following public APIs
- Adds new ffi functions related to appends / transactions

## How was this change tested?
Using the `test_basic_append` in `ffi/src/transaction/mod.rs`. 


## Main points to comment on for reviewers of Draft PR:
I would like to draw the reviewer's attention to:
- the concept of testing new ffi functionality through rust tests
instead of a C example program
- usage of `tempfile::tempdir` to create a tmp directory for test data
during the test
- some outstanding `TODO`s in the `test_basic_append` test

Once the current approach is approved by reviewers, I can follow up with
more testing:
- the write schema
- appending to a partitioned table
joonyoo181 pushed a commit to joonyoo181/delta-kernel-rs that referenced this pull request Aug 5, 2025
This PR is a retake of
delta-io#550

## What changes are proposed in this pull request?
This PR implements the ffi functions required to do appends. The main
difference over delta-io#550 is
how it's tested: instead of using a C based example we now opt for a
rust-based test that is able to re-used various testing-related utility
functions that are used in the kernel tests. To do this cleanly, i've
moved several functions over to the test-utils crate.

## This PR affects the following public APIs
- Adds new ffi functions related to appends / transactions

## How was this change tested?
Using the `test_basic_append` in `ffi/src/transaction/mod.rs`. 


## Main points to comment on for reviewers of Draft PR:
I would like to draw the reviewer's attention to:
- the concept of testing new ffi functionality through rust tests
instead of a C example program
- usage of `tempfile::tempdir` to create a tmp directory for test data
during the test
- some outstanding `TODO`s in the `test_basic_append` test

Once the current approach is approved by reviewers, I can follow up with
more testing:
- the write schema
- appending to a partitioned table
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change Change that require a major version bump

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants