feat!: add ffi for idempotent write primitives #1191
Conversation
github-actions
bot
added
the
breaking-change
samansmink
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1191 +/- ##
==========================================
+ Coverage 84.68% 84.73% +0.04%
==========================================
Files 116 117 +1
Lines 29762 29894 +132
Branches 29762 29894 +132
==========================================
+ Hits 25204 25330 +126
+ Misses 3342 3340 -2
- Partials 1216 1224 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| row: usize, | ||
| transforms: &CTransforms, | ||
| ) -> Option<Handle<SharedExpression>> { | ||
| ) -> OptionalValue<Handle<SharedExpression>> { |
There was a problem hiding this comment.
changed function signature but not test change points to missing coverage? perhaps let's track an issue to fix?
| version: i64, | ||
| ) -> DeltaResult<Handle<ExclusiveTransaction>> { | ||
| let app_id_string: DeltaResult<String> = unsafe { TryFromStringSlice::try_from_slice(&app_id) }; | ||
| Ok(Box::new(txn.with_transaction_id(app_id_string?, version)).into()) |
There was a problem hiding this comment.
one thing i'm noticing with these builder FFIs - we are un-boxing (above) and re-boxing (here) every builder method that consumes self. maybe not worth optimizing yet but seems non-ideal
There was a problem hiding this comment.
This sounds like it would be a follow up issue to this PR, right? Should we open an issue for it?
| fn get_app_id_version_impl( | ||
| snapshot: Arc<Snapshot>, | ||
| app_id: DeltaResult<String>, | ||
| extern_engine: &dyn ExternEngine, | ||
| ) -> DeltaResult<Option<i64>> { | ||
| snapshot.get_app_id_version(&app_id?, extern_engine.engine().as_ref()) | ||
| } |
There was a problem hiding this comment.
do we need this or could we just inline snapshot.get_app_id_version(...) above?
There was a problem hiding this comment.
well the app_id might be a DeltaResult::Err right? so then we'd need to handle that anyway so I thought this was simpler and more in line with the other ffi methods?
Perhaps my rust skills are lacking here, but how else would we cleanly return an ExternResult::Err from get_app_id_version if app_id is a DeltaResult::Err?
There was a problem hiding this comment.
makes sense that it's easier to return the error 👍
|
hey @samansmink just ping whenever you're ready for us to take another look! |
|
@zachschuermann I think CI should pass now, I still need to look at how to more cleanly fix the miri issue with the test. I've disabled miri for the whole test for now. There's only your comment about |
|
@samansmink looks good, just needs a rebase and some clearer naming :) |
|
Thanks for the review @OussamaSaoudi! Applied proposed fixes and updated the branch! |
samansmink
force-pushed
the
poc-idempotent-writes
branch
from
dfe0197 to
63ef6e6
Compare
Co-authored-by: Zach Schuermann <[email protected]>
|
@zachschuermann done! |
|
@samansmink does this means CREATE TABLE is supported too ? |
|
@djouallah no, not yet unfortunately! |
4 participants
What changes are proposed in this pull request?
This PR adds ffi functions for
Transaction::with_transaction_idandSnapshot::get_app_id_version. With these primitives, idempotent writes can be achieved.Because we were lacking an FFI safe Option implementation, I added a FFI safe
OptionalValuestruct. Note that this doesn't really result in the nicest looking C API, but it does work and is clean from the rust side.This PR affects the following public APIs
added to ffi:
get_app_id_version(snapshot, app_id, engine)with_transaction_id(txn, app_id, version, engine)change function call:
get_transform_for_rownow returns new FFI-safe OptionalValue instead of OptionHow was this change tested?
a new test was added that was based on the
test_write_txn_actionstest from kernel crateNotes to reviewer
PTAL at the
OptionalValue<T>struct added as an FFI safeOption<T>