feat: StagedCommitter for easier Committer integration
#1514
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1514 +/- ##
==========================================
- Coverage 84.67% 84.61% -0.06%
==========================================
Files 122 122
Lines 32741 32762 +21
Branches 32741 32762 +21
==========================================
+ Hits 27722 27723 +1
- Misses 3674 3695 +21
+ Partials 1345 1344 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| tracing::debug!("wrote staged commit file: {:?}", committed); | ||
|
|
||
| self.catalog_committer | ||
| .commit_request(engine, &staged_commit_path, self.context.as_ref()) |
There was a problem hiding this comment.
Can you help me understand: is the commit_metadata passed into the catalog_committer::commit_request method?
| let staged_commit_path = commit_metadata.staged_commit_path()?; | ||
| engine | ||
| .json_handler() | ||
| .write_json_file(&staged_commit_path, Box::new(actions), false)?; | ||
|
|
||
| let committed = engine.storage_handler().head(&staged_commit_path)?; |
There was a problem hiding this comment.
So is the point of StagedCommitter just to allow reuse of these lines of code here that write the staged commit file?
I wonder if this is worth the overhead / potential complexity?
We can always add this later -- and I do think that just copying these few lines of code, or exposing, if you'd like, a public utility that makes this 1 line (I still prefer just these 4, since it's so simple) could be a better alternative in the short term?
There was a problem hiding this comment.
Note that a c/c++ engine cannot just paste these lines of rust code. This utility is almost entirely targeted at ffi engines that don't have an easy way to "just write the data" because they don't have access to all the rust default engine machinery.
That said, @zachschuermann , would it be simpler from an FFI perspective to just provide a write_actions utility that duckdb could call to write the staged file? I think in the end the amount of code would be similar, but it might circumvent the context problem?
There was a problem hiding this comment.
Thanks Nick. Can we schedule some time (or point me to some resources) to help me learn more about FFI and non-rust-engine complexity ?
For example: If a C++ Connector already has a C++ Engine implementation (not saying that is a trivial task) -- is it then easy for them to use the UC Committer?
Actually, to back up further, would a C++ Connector us the rust-UC-Committer (via FFI bindings) or would it have to build its own UC-Committer?
3 participants
What changes are proposed in this pull request?
Adds a new
StagedCommitterstruct which effectively does the staged commit write for the engine and only requires the catalog RPC to be implemented.How was this change tested?
todo