delta-incubator / delta-dotnet

DeltaLake bindings for dotnet based on delta-rs
Apache License 2.0
25 stars 6 forks source link

feat: Add Delta Kernel FFI support for read path and expose Apache.Arrow.Table and Microsoft.Data.Analysis.DataFrame read interface #89

Closed mdrakiburrahman closed 3 weeks ago

mdrakiburrahman commented 1 month ago

Why this change is needed

This PR adds Delta Kernel FFI based read support.

Closes issue: https://github.com/delta-incubator/delta-dotnet/issues/82

How

Delta Kernel integration

pr

Misc

Test

mightyshazam commented 1 month ago

The methods don't have to be synchronous. Technically, C is always synchronous. However, any method that takes a callback function can be wrapped with a TaskCompletionSource to make it asynchronous in C#. tokio only matters if the underlying rust method is async. If that were the case, then, like the Bridge code, it would require a runtime or create it upon the method call.

mdrakiburrahman commented 1 month ago

Makes sense @mightyshazam - I had the incorrect understanding that this works through and through due to the Tokio wrapper. But I'm guessing the idea is the Cancellation Token would be respected by that TaskCompletionSource and abandon whatever C FFI call was being made that's taking too long.

Also, do you prefer I get this TSC wrapper done before merging this PR, so we change the ITable interface to be completely async?

Or should we have the synchronous methods as the PR currently has, and expose additional async methods?

mightyshazam commented 1 month ago

Makes sense @mightyshazam - I had the incorrect understanding that this works through and through due to the Tokio wrapper. But I'm guessing the idea is the Cancellation Token would be respected by that TaskCompletionSource and abandon whatever C FFI call was being made that's taking too long.

Also, do you prefer I get this TSC wrapper done before merging this PR, so we change the ITable interface to be completely async?

Or should we have the synchronous methods as the PR currently has, and expose additional async methods?

We can change the methods to async later. As long as we don't increment the version of the package, it won't push a new one.

mdrakiburrahman commented 1 month ago

@mightyshazam - yeap good point, sounds good, I added this issue to track this:

https://github.com/delta-incubator/delta-dotnet/issues/91

Will get it done in a separate smaller PR