feat(catalog-managed): UCCommitter #1418

zachschuermann · 2025-10-22T19:56:58Z

What changes are proposed in this pull request?

Adds a new UCCommitter for writing to tables in UC (in uc-catalog crate for now, final resting place TBD).

Additionally, adds the following RW table features in supported writer features (did we never enable V2Checkpoint write?)

VacuumProtocolCheck
V2Checkpoint
CatalogManaged (behind catalog-managed feature flag)
CatalogOwnedPreview (behind catalog-managed feature 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

codecov · 2025-10-22T21:25:43Z

Codecov Report

❌ Patch coverage is 29.14286% with 124 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.53%. Comparing base (ddeef7f) to head (820b8c1).

Files with missing lines	Patch %	Lines
uc-catalog/src/lib.rs	0.00%	57 Missing ⚠️
uc-catalog/src/committer.rs	0.00%	53 Missing ⚠️
kernel/src/path.rs	44.44%	5 Missing and 5 partials ⚠️
kernel/src/committer.rs	87.50%	3 Missing ⚠️
kernel/src/log_path.rs	90.90%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1418      +/-   ##
==========================================
- Coverage   84.82%   84.53%   -0.29%     
==========================================
  Files         121      122       +1     
  Lines       32220    32361     +141     
  Branches    32220    32361     +141     
==========================================
+ Hits        27330    27356      +26     
- Misses       3561     3675     +114     
- Partials     1329     1330       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

zachschuermann · 2025-10-22T21:28:31Z

kernel/src/table_features/mod.rs

+        WriterFeature::VacuumProtocolCheck,
        WriterFeature::VariantType,
        WriterFeature::VariantTypePreview,
        WriterFeature::VariantShreddingPreview,
+        WriterFeature::V2Checkpoint,


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

Mmm, V2Checkpoint disallows multi-part checkpoints. I guess we don't support that in kernel so we're okay?

you mean for writes? yea and we have no intention of ever writing out multi-part checkpoints since they were broken from the start

zachschuermann · 2025-10-22T21:29:54Z

uc-client/src/client.rs

+        // 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(())


fixed latent bug

DrakeLin · 2025-10-23T22:29:15Z

uc-catalog/src/lib.rs

+        let mut table_url = Url::parse(&table_uri)?;
+        // add trailing slash
+        if !table_url.path().ends_with('/') {
+            table_url.path_segments_mut().unwrap().push("");


are we trying to push "/" ?

yea it's a little weird.. i think by adding a new empty path segment it adds a trailing slash

can we leave a comment to that effect?

uc-catalog/src/committer.rs

nicklan

cool, mostly good just a few comments

nicklan · 2025-10-24T00:24:28Z

kernel/src/table_features/mod.rs

+        WriterFeature::VacuumProtocolCheck,
        WriterFeature::VariantType,
        WriterFeature::VariantTypePreview,
        WriterFeature::VariantShreddingPreview,
+        WriterFeature::V2Checkpoint,


Mmm, V2Checkpoint disallows multi-part checkpoints. I guess we don't support that in kernel so we're okay?

uc-catalog/src/committer.rs

zachschuermann · 2025-11-14T19:38:01Z

uc-catalog/src/committer.rs

+            ),
+            last_backfilled_version,
+        );
+        let handle = tokio::runtime::Handle::current();


note that this currently only works on tokio - I'll make an issue to do better here.

nicklan

looks good, just a few small things

nicklan · 2025-11-14T21:00:46Z

uc-catalog/src/lib.rs

            start_version: Some(0),
            end_version: version.and_then(|v| v.try_into().ok()),
        };
-        // TODO: does it paginate?


Are we just assuming that the client internally handles any pagination? I think yes?

nicklan · 2025-11-14T21:02:06Z

uc-catalog/src/lib.rs

+        if let Some(commits) = &mut commits.commits {
+            commits.sort_by_key(|c| c.version);
+        }

        // if commits are present, we ensure they are sorted+contiguous
        if let Some(commits) = &commits.commits {


Suggested change

if let Some(commits) = &mut commits.commits {

commits.sort_by_key(|c| c.version);

}

// if commits are present, we ensure they are sorted+contiguous

if let Some(commits) = &commits.commits {

// if commits are present, we ensure they are sorted+contiguous

if let Some(commits) = &mut commits.commits {

commits.sort_by_key(|c| c.version);

That suggestion isn't very readable, just saying we can collapse the two checks

Ohh, maybe that doesn't work because it'll want to borrow as mutable and immutable. Then maybe:

commits.as_mut().map(|commits| commits.sort_by_key(|c| c.version));

Fewer lines, maybe not worth it

nicklan · 2025-11-14T21:02:48Z

uc-catalog/src/lib.rs

+        let mut table_url = Url::parse(&table_uri)?;
+        // add trailing slash
+        if !table_url.path().ends_with('/') {
+            table_url.path_segments_mut().unwrap().push("");


can we leave a comment to that effect?

nicklan · 2025-11-14T21:04:46Z

uc-catalog/src/lib.rs

            })
            .try_collect()?;

        info!("commits for kernel: {:?}\n", commits);


debug maybe?

nicklan · 2025-11-14T21:05:11Z

uc-catalog/src/lib.rs

+    // `ENDPOINT=".." TABLENAME=".." TOKEN=".." cargo t write_uc_table --nocapture -- --ignored`
+    #[ignore]
+    #[tokio::test(flavor = "multi_thread")]
+    async fn write_uc_table() -> Result<(), Box<dyn std::error::Error>> {


Should this just be an example program? Fine with that being a follow-up

nicklan · 2025-11-14T21:09:16Z

uc-catalog/src/committer.rs

+            last_backfilled_version,
+        );
+        let handle = tokio::runtime::Handle::current();
+        tokio::task::block_in_place(|| {


This only works in the multithreaded runtime. are we sure we'll always have that flavor?

github-actions bot assigned zachschuermann Oct 22, 2025

zachschuermann force-pushed the uc-committer branch from e0d9c59 to 2007eaf Compare October 22, 2025 21:23

github-actions bot added the breaking-change Change that require a major version bump label Oct 22, 2025

zachschuermann commented Oct 22, 2025

View reviewed changes

nicklan requested review from DrakeLin and nicklan October 23, 2025 18:44

DrakeLin reviewed Oct 23, 2025

View reviewed changes

nicklan reviewed Oct 24, 2025

View reviewed changes

zachschuermann force-pushed the uc-committer branch 5 times, most recently from 75a2201 to aee3668 Compare November 14, 2025 17:03

zachschuermann removed the breaking-change Change that require a major version bump label Nov 14, 2025

zachschuermann force-pushed the uc-committer branch from aee3668 to e2c3f6b Compare November 14, 2025 19:31

zachschuermann requested review from DrakeLin and nicklan November 14, 2025 19:31

github-actions bot added the breaking-change Change that require a major version bump label Nov 14, 2025

uc-committer

820b8c1

zachschuermann force-pushed the uc-committer branch from e2c3f6b to 820b8c1 Compare November 14, 2025 19:36

zachschuermann commented Nov 14, 2025

View reviewed changes

nicklan reviewed Nov 14, 2025

View reviewed changes

feat(catalog-managed): UCCommitter #1418

Are you sure you want to change the base?

feat(catalog-managed): UCCommitter #1418

Conversation

zachschuermann commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes are proposed in this pull request?

How was this change tested?

Uh oh!

codecov bot commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nicklan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicklan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zachschuermann commented Oct 22, 2025 •

edited

Loading

codecov bot commented Oct 22, 2025 •

edited

Loading