CDF API for FFI #1335

kssenii · 2025-09-22T14:37:40Z

What changes are proposed in this pull request?

Change Data Feed FFI APIs #1196

How was this change tested?

Tests in ffi/src/table_changes.rs

OussamaSaoudi · 2025-09-23T15:43:36Z

kernel/src/table_changes/scan.rs

        &self,
        engine: Arc<dyn Engine>,
-    ) -> DeltaResult<impl Iterator<Item = DeltaResult<ScanResult>> + use<'_>> {
+    ) -> DeltaResult<impl Iterator<Item = DeltaResult<ScanResult>>> {


thank you 👍

OussamaSaoudi · 2025-09-23T15:43:46Z

kernel/src/log_segment.rs

                .is_some_and(|first_commit| first_commit.version == start_version),
            Error::generic(format!(
-                "Expected the first commit to have version {start_version}"
+                "Expected the first commit to have version {start_version}, got {:?}",


OussamaSaoudi · 2025-09-23T15:45:15Z

ffi/src/table_changes.rs

+use delta_kernel::arrow::array::{
+    ffi::{FFI_ArrowArray, FFI_ArrowSchema},
+    ArrayData, RecordBatch, StructArray,
+};


Suggested change

use delta_kernel::arrow::array::{

ffi::{FFI_ArrowArray, FFI_ArrowSchema},

ArrayData, RecordBatch, StructArray,

};

use delta_kernel::arrow::array::ffi::{FFI_ArrowArray, FFI_ArrowSchema};

use delta_kernel::arrow::array::ArrayData, RecordBatch, StructArray;

OussamaSaoudi · 2025-09-23T15:51:33Z

ffi/src/table_changes.rs

+    let mut data = data
+        .data
+        .lock()
+        .map_err(|_| Error::generic("poisoned mutex"))?;


Suggested change

.map_err(|_| Error::generic("poisoned mutex"))?;

.map_err(|_| Error::generic("poisoned scan table changes iterator mutex"))?;

OussamaSaoudi · 2025-09-23T16:03:38Z

ffi/src/table_changes.rs

+    }
+
+    #[tokio::test]
+    async fn test_table_changes() -> Result<(), Box<dyn std::error::Error>> {


Suggested change

async fn test_table_changes() -> Result<(), Box<dyn std::error::Error>> {

async fn test_table_changes_getters() -> Result<(), Box<dyn std::error::Error>> {

OussamaSaoudi · 2025-09-23T16:20:26Z

ffi/src/table_changes.rs

+                let data_ref = &mut *data;
+                let array = std::mem::replace(&mut data_ref.array, FFI_ArrowArray::empty());
+                get_engine_data(array, &(*data).schema, allocate_err)


Once you make scan_table_changes_next return ArrowFFIData by value, you can do this:

Suggested change

let data_ref = &mut *data;

let array = std::mem::replace(&mut data_ref.array, FFI_ArrowArray::empty());

get_engine_data(array, &(*data).schema, allocate_err)

get_engine_data(data.array, &data.schema, allocate_err)

ffi/src/table_changes.rs

OussamaSaoudi · 2025-09-23T18:17:29Z

ffi/src/table_changes.rs

+        let mut visitor_state = KernelExpressionVisitorState::default();
+        let pred_id = (predicate.visitor)(predicate.predicate, &mut visitor_state);
+        let predicate = unwrap_kernel_predicate(&mut visitor_state, pred_id);
+        debug!("Got predicate: {:#?}", predicate);


Suggested change

debug!("Got predicate: {:#?}", predicate);

debug!("Table changes got predicate: {:#?}", predicate);

OussamaSaoudi

I think some details can be iterated on, but I think this is super valuable. Thanks @kssenii!!

OussamaSaoudi · 2025-09-26T22:46:50Z

ffi/src/table_changes.rs

+
+        let batch_struct_array: StructArray = record_batch.into();
+        let array_data: ArrayData = batch_struct_array.into_data();
+        let (out_array, out_schema) = to_ffi(&array_data)?;


This line will produce a new schema for each call. I think we need to eventually just return ArrowData instead of ArrowFFIData. We can do that as a followup though :)

I think we need to eventually just return ArrowData

e.g. FFI_ArrowArray?

OussamaSaoudi · 2025-09-26T22:56:15Z

ffi/src/table_changes.rs

+        .data
+        .lock()
+        .map_err(|_| Error::generic("poisoned scan table changes iterator mutex"))?;
+    if let Some(scan_result) = data.next().transpose()? {


nit: We can avoid nesting by simply returning early :)

Suggested change

if let Some(scan_result) = data.next().transpose()? {

let Some(scan_result) = data.next().transpose()? else {

return Ok(ArrowFFIData::empty());

}

OussamaSaoudi · 2025-09-26T22:58:04Z

ffi/src/table_changes.rs

+#[no_mangle]
+pub unsafe extern "C" fn scan_table_changes_next(
+    data: Handle<SharedScanTableChangesIterator>,
+) -> ExternResult<ArrowFFIData> {


We're checking that the iteration has ended by checking that the ArrowFFIData is empty. I think we'll eventually want to change this too

What would be a better way to do this?

likely the FFI OptionalValue once that's merged. @samansmink is working on that, so we can do this as a followup.

OussamaSaoudi · 2025-09-26T23:02:27Z

One more thing @kssenii Can you track the CDF FFI followups in the tracking issue in #1335? Main item is removing the duplicate work when constructing the schema for ArrowFFIData.

codecov · 2025-09-26T23:03:35Z

Codecov Report

❌ Patch coverage is 87.62475% with 62 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.85%. Comparing base (733a117) to head (c4f5303).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
ffi/src/table_changes.rs	87.19%	14 Missing and 48 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1335      +/-   ##
==========================================
+ Coverage   84.79%   84.85%   +0.06%     
==========================================
  Files         118      119       +1     
  Lines       30366    30862     +496     
  Branches    30366    30862     +496     
==========================================
+ Hits        25748    26188     +440     
- Misses       3387     3395       +8     
- Partials     1231     1279      +48

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…i-for-ffi

OussamaSaoudi · 2025-09-29T16:32:13Z

Hey @kssenii could you patch up the CI issues? You can click on the jobs to see what's failing (example: cargo clippy --benches --tests --all-features -- -D warnings)

kssenii · 2025-10-06T16:22:17Z

@OussamaSaoudi, could you please reenable ci workflow?
All checks should be fixed now, though for Miri I made a blind fix as I cannot run it locally (getting test table_changes::tests::test_table_changes_between_commits ... error: unsupported operation: can't call foreign function 'ioctl' on OS 'linux' when running on linux and test table_changes::tests::test_table_changes_getters ... error: unsupported operation: can't call foreign function 'kqueue' on OS 'macos' when running on macos...)

OussamaSaoudi · 2025-10-06T16:28:18Z

@kssenii

Reenabled CI 👍

test table_changes::tests::test_table_changes_between_commits ... error: unsupported operation: can't call foreign function 'ioctl' on OS 'linux'

Hmm this might be a known issue with CI on FFI. Let me check.

OussamaSaoudi · 2025-10-06T16:31:26Z

@kssenii for now, please disable miri on the test.

    #[cfg_attr(miri, ignore)] // FIXME: re-enable miri (can't call foreign function `ioctl` on OS `linux`)

kssenii · 2025-10-06T16:33:15Z

for now, please disable miri on the test.

In CI it did not get the error I have locally, but this:

test result: ok. 16 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out; finished in 239.42s

error: the main thread terminated without waiting for all remaining threads

note: set `MIRIFLAGS=-Zmiri-ignore-leaks` to disable this check

error: aborting due to 1 previous error

error: test failed, to rerun pass `--lib`

(https://github.com/delta-io/delta-kernel-rs/actions/runs/18136387522/job/51844035758)
So my blind fix is: 52d4015

Though on latest CI run it fails completely differently, checking...

OussamaSaoudi · 2025-10-08T00:03:39Z

ffi/src/table_changes.rs

+        unsafe {
+            free_table_changes_scan(table_changes_scan);
+            free_engine(engine);
+        }


@kssenii the latest build indicates that we're leaking memory for the schema:

507 | #[derive(Debug, PartialEq, Clone, Eq)] | ^^^^^ note: inside `table_changes::table_changes_schema` --> ffi/src/table_changes.rs:118:14 | 118 | Arc::new(table_changes.schema().clone()).into() | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ note: inside closure --> ffi/src/table_changes.rs:584:31 | 584 | let schema = unsafe { table_changes_schema(table_changes.shallow_copy()).shallow_copy() };

I think we may need to call free_schema in the test for the newly allocated copy of the schema.

Suggested change

unsafe {

free_table_changes_scan(table_changes_scan);

free_engine(engine);

}

unsafe {

free_table_changes_scan(table_changes_scan);

free_engine(engine);

free_schema(schema);

}

OussamaSaoudi · 2025-10-08T23:26:34Z

ffi/src/lib.rs

 pub mod engine_data;
 pub mod engine_funcs;
 pub mod error;
+pub mod table_changes;


Let's gate the entire feature since it depends on Arrow.

Suggested change

pub mod table_changes;

#[cfg(feature = "default-engine-base")]

pub mod table_changes;

This lets us remove the repeated #[cfg(feature = "default-engine-base")] below

…i-for-ffi

kssenii · 2025-10-15T16:43:27Z

Hi @OussamaSaoudi, is there anything needed to be done to have this PR merged?

nicklan

lgtm! thanks for the contribution!

nicklan · 2025-10-17T21:43:18Z

ffi/src/engine_data.rs

    pub schema: FFI_ArrowSchema,
 }

+#[cfg(feature = "default-engine-base")]


too bad the arrow folks didn't implement Default for FFI_ArrowArray and FFI_ArrowSchema

OussamaSaoudi · 2025-10-20T22:39:58Z

@kssenii pls update the branch and ping me so we can merge :D

Thank you for your contribution!

…i-for-ffi

kssenii · 2025-10-21T17:56:58Z

@OussamaSaoudi-db, ping :)

…i-for-ffi

CDF API for FFI

37aee77

github-actions bot assigned kssenii Sep 22, 2025

zachschuermann requested review from OussamaSaoudi, nicklan, scovich and zachschuermann September 22, 2025 17:39

OussamaSaoudi reviewed Sep 23, 2025

View reviewed changes

Review fixes

87a370e

kssenii requested a review from OussamaSaoudi September 25, 2025 09:46

OussamaSaoudi approved these changes Sep 26, 2025

View reviewed changes

kssenii added 2 commits September 29, 2025 14:42

Merge remote-tracking branch 'delta-io/main' into delta-kernel-cdf-ap…

84ab135

…i-for-ffi

Review fix

5da6338

OussamaSaoudi removed request for scovich and zachschuermann September 29, 2025 16:20

OussamaSaoudi added Area: FFI Related to FFI Area: change-data-feed labels Sep 29, 2025

kssenii and others added 5 commits September 29, 2025 19:55

Apply clippy

5c968c0

Apply cargo fmt

e39c356

Fix Doc check

f409977

Fix build with --no-default-features

1f6054b

Try fix miri check

52d4015

OussamaSaoudi reviewed Oct 8, 2025

View reviewed changes

Apply cargo fmt; Try fix memory leak

b9193da

OussamaSaoudi reviewed Oct 8, 2025

View reviewed changes

kssenii and others added 2 commits October 9, 2025 18:35

Remove repeated dependency

8bcf6da

Merge remote-tracking branch 'delta-io/main' into delta-kernel-cdf-ap…

fbaf128

…i-for-ffi

kssenii requested a review from OussamaSaoudi October 10, 2025 19:13

OussamaSaoudi approved these changes Oct 16, 2025

View reviewed changes

nicklan approved these changes Oct 17, 2025

View reviewed changes

Merge remote-tracking branch 'delta-io/main' into delta-kernel-cdf-ap…

1d73cf7

…i-for-ffi

Merge remote-tracking branch 'delta-io/main' into delta-kernel-cdf-ap…

c4f5303

…i-for-ffi

zachschuermann merged commit 2b49385 into delta-io:main Oct 23, 2025
22 checks passed

	.map_err(\|_\| Error::generic("poisoned mutex"))?;
	.map_err(\|_\| Error::generic("poisoned scan table changes iterator mutex"))?;

	async fn test_table_changes() -> Result<(), Box<dyn std::error::Error>> {
	async fn test_table_changes_getters() -> Result<(), Box<dyn std::error::Error>> {

	debug!("Got predicate: {:#?}", predicate);
	debug!("Table changes got predicate: {:#?}", predicate);

-    if let Some(scan_result) = data.next().transpose()? {
+    let Some(scan_result) = data.next().transpose()?  else {
+        return Ok(ArrowFFIData::empty());
+    }

	pub mod table_changes;
	#[cfg(feature = "default-engine-base")]
	pub mod table_changes;

Uh oh!

CDF API for FFI #1335

CDF API for FFI #1335

Uh oh!

Conversation

kssenii commented Sep 22, 2025

What changes are proposed in this pull request?

How was this change tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OussamaSaoudi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OussamaSaoudi commented Sep 26, 2025

Uh oh!

codecov bot commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

OussamaSaoudi commented Sep 29, 2025

Uh oh!

kssenii commented Oct 6, 2025

Uh oh!

OussamaSaoudi commented Oct 6, 2025

Uh oh!

OussamaSaoudi commented Oct 6, 2025

Uh oh!

kssenii commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kssenii commented Oct 15, 2025

Uh oh!

nicklan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OussamaSaoudi commented Oct 20, 2025

Uh oh!

kssenii commented Oct 21, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

codecov bot commented Sep 26, 2025 •

edited

Loading

kssenii commented Oct 6, 2025 •

edited

Loading