feat(catalog-managed): UCCommitter #1418
Conversation
zachschuermann
force-pushed
the
uc-committer
branch
from
e0d9c59 to
2007eaf
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1418 +/- ##
==========================================
- Coverage 84.61% 84.38% -0.23%
==========================================
Files 117 119 +2
Lines 29936 30439 +503
Branches 29936 30439 +503
==========================================
+ Hits 25330 25687 +357
- Misses 3382 3524 +142
- Partials 1224 1228 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actions
bot
added
the
breaking-change
| WriterFeature::VacuumProtocolCheck, | ||
| WriterFeature::VariantType, | ||
| WriterFeature::VariantTypePreview, | ||
| WriterFeature::VariantShreddingPreview, | ||
| WriterFeature::V2Checkpoint, |
There was a problem hiding this comment.
since the test tables i was playing with have V2Checkpoint and VacuumProtocolCheck I went ahead and listed them here. I think this is safe (and even desirable) since (1) we don't yet write v2 checkpoint and (2) we don't vacuum and this will unblock writes to such tables
There was a problem hiding this comment.
Mmm, V2Checkpoint disallows multi-part checkpoints. I guess we don't support that in kernel so we're okay?
| // Note: can't just deserialize to () directly so we make an empty struct to deserialize | ||
| // the `{}` into. Externally we still return unit type for ease of use/understanding. | ||
| #[derive(Deserialize)] | ||
| struct EmptyResponse {} | ||
| let _: EmptyResponse = self.handle_response(response).await?; | ||
| Ok(()) |
| let mut table_url = Url::parse(&table_uri)?; | ||
| // add trailing slash | ||
| if !table_url.path().ends_with('/') { | ||
| table_url.path_segments_mut().unwrap().push(""); |
There was a problem hiding this comment.
are we trying to push "/" ?
| "{version:020}.{uuid}.json", | ||
| version = commit_metadata.version() | ||
| ); | ||
| // FIXME use table path from commit_metadata? |
There was a problem hiding this comment.
Should this be a FIXME or a TODO?
| WriterFeature::VacuumProtocolCheck, | ||
| WriterFeature::VariantType, | ||
| WriterFeature::VariantTypePreview, | ||
| WriterFeature::VariantShreddingPreview, | ||
| WriterFeature::V2Checkpoint, |
There was a problem hiding this comment.
Mmm, V2Checkpoint disallows multi-part checkpoints. I guess we don't support that in kernel so we're okay?
| ); | ||
| // FIXME use table path from commit_metadata? | ||
| let mut commit_path = Url::parse(&self.table_uri)?; | ||
| commit_path.path_segments_mut().unwrap().extend(&[ |
There was a problem hiding this comment.
all the unwraps should be ok_ors
| .next() | ||
| .unwrap() | ||
| .unwrap(); | ||
| println!("wrote commit file: {:?}", committed); |
There was a problem hiding this comment.
this is part of a lib right? we should info!
3 participants
What changes are proposed in this pull request?
Adds a new
UCCommitterfor writing to tables in UC (inuc-catalogcrate for now, final resting place TBD).Additionally, adds the following RW table features in supported writer features (to unblock our write tests but also since these are required t
VacuumProtocolCheckV2CheckpointCatalogManaged(behindcatalog-managedfeature flag)CatalogOwnedPreview(behindcatalog-managedfeature flag)How was this change tested?
added an ignored test to write to cc tables; run with
ENDPOINT=".." TABLENAME=".." TOKEN=".." cargo t write_uc_table --nocapture -- --ignored