feat(FFI): snapshot log tail FFI #1379
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1379 +/- ##
==========================================
+ Coverage 84.73% 84.88% +0.14%
==========================================
Files 113 114 +1
Lines 28396 29040 +644
Branches 28396 29040 +644
==========================================
+ Hits 24062 24651 +589
- Misses 3198 3215 +17
- Partials 1136 1174 +38 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actions
bot
added
the
breaking-change
zachschuermann
force-pushed
the
snapshot-builder-ffi
branch
from
b275a62 to
f9e24c6
Compare
| // Convert LogPathArray to Vec<LogPath> | ||
| let log_tail = match unsafe { log_tail.log_paths() } { | ||
| Ok(paths) => paths, | ||
| Err(e) => return DeltaResult::Err(e).into_extern_result(&engine_ref), |
There was a problem hiding this comment.
Suggested change
| Err(e) => return DeltaResult::Err(e).into_extern_result(&engine_ref), | |
| err => return err.into_extern_result(&engine_ref), |
Comment on lines
+1023
to
+1029
| let log_path = | ||
| log_path::FfiLogPath::new(kernel_string_slice!(commit1_path), 123456789, 100); | ||
| let log_tail = vec![log_path]; | ||
| let log_tail = log_path::LogPathArray { | ||
| ptr: log_tail.as_ptr(), | ||
| len: log_tail.len(), | ||
| }; |
There was a problem hiding this comment.
Might be worth just making a LogPathArray::fromVec.
Also, while it's handled right here, I think we need to be very careful with Vec since drop could still be called and we'd have a use-after-free.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
Adds two new APIs for our FFI to support catalog-managed tables:
snapshot_with_log_tailto get the latest snapshot of a catalog-managed tablesnapshot_at_version_with_log_tailto get the snapshot of a catalog-managed table at a specific versionAnd in order to implement the above, a new
LogPathArraytype is introduced to communicateVec<LogPath>across FFI (basically just uses ptr + len for the array type and theFfiLogPathdoes KernelStringSlice + size + mod time)The above APIs are behind a new
catalog-managed(FFI) feature flag similar to kernel's owncatalog-managedflag.How was this change tested?
new UT