feat: support writing domain metadata (2/2) #1275

MannDP · 2025-09-09T23:05:15Z

What changes are proposed in this pull request?

This PR is (2/2) to support writing domain metadata. It adds support for removing metadata for user-specified domains. As per the Delta specification, for a removed domain metadata "writers should preserve an accurate pre-image of the configuration". Thus for any removals, we need to perform a log replay and restore the original configuration of the domain with the removed flag set to true. Furthermore, for any removal where the domain does not already exist in the log, we treat this as a no-op and do not write any record to the Delta Log.

resolves Support domain metadata writes #1270

How was this change tested?

Added new integration tests in write.rs.

codecov · 2025-09-09T23:09:41Z

Codecov Report

❌ Patch coverage is 97.72727% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 84.67%. Comparing base (05e6698) to head (eae4394).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/transaction/mod.rs	96.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1275      +/-   ##
==========================================
+ Coverage   84.55%   84.67%   +0.11%     
==========================================
  Files         113      113              
  Lines       28009    28337     +328     
  Branches    28009    28337     +328     
==========================================
+ Hits        23683    23993     +310     
- Misses       3189     3206      +17     
- Partials     1137     1138       +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.

kernel/src/actions/mod.rs

## What changes are proposed in this pull request?  This PR is (1/2) to support writing domain metadata. 1. (this PR) support adding or updating a domain metadata configuration 2. (follow up) support _removing_ a domain metadata, #1275. - resolves #1270 Writing domain metadata, as per the [Delta protocol](https://arc.net/l/quote/fwjwtumy) requires: - The table must be on writer version 7, and a feature named `domainMetadata` must exist in the table's `writerFeatures`. We satisfy this via an explicit check in `Transaction::commit`. - Two overlapping transactions conflict if they both contain a domain metadata action for the same metadata domain. We satisfy this with Kernel's coarse grained conflict detection based on the fact that the `delta_log` dir already has a log file with the name this txn was going to write. No new logic needed in this PR. ## How was this change tested?  Added new integration tests in `write.rs`.

zachschuermann · 2025-09-24T20:42:57Z

kernel/src/actions/mod.rs

+    pub(crate) fn remove(domain: String) -> Self {
+        Self {
+            domain,
+            configuration: String::new(),


is this how existing implementations do removes? I kinda figured it would retain the old configuration but haven't dug into this yet

great call out! i went back and looked at the protocol again. "Writers should preserve an accurate pre-image of the configuration" -> so we should keep it around actually.

Retaining the old configuration would require log replay though, right? We could accept configuration: Option<String> as an additional parameter to let the caller provide the old value if already known, but TBH I'm not sure if that improves the current solution.

You're right that it requires log replay. But as long as we do that once it should be okay. I checked the Java version of the delta kernel for inspiration and they follow this pattern where remove takes only the domain and they internally determine the pre-image configuration value to preserve. Code ptr: https://github.com/delta-io/delta/blob/master/kernel/kernel-api/src/main/java/io/delta/kernel/internal/TransactionImpl.java#L230C15-L230C35.

The other thing is if we let users pass it in, we likely should validate that the pre-image they provide is correct. Which would require log replay anyways.

kernel/src/transaction/mod.rs

Co-authored-by: Zach Schuermann <[email protected]>

lbhm

Overall LGTM. I left a few nits and one suggestion for improving test coverage.

lbhm · 2025-09-25T14:18:30Z

kernel/src/actions/mod.rs

+    pub(crate) fn remove(domain: String) -> Self {
+        Self {
+            domain,
+            configuration: String::new(),


Retaining the old configuration would require log replay though, right? We could accept configuration: Option<String> as an additional parameter to let the caller provide the old value if already known, but TBH I'm not sure if that improves the current solution.

lbhm · 2025-09-25T14:20:25Z

kernel/src/transaction/mod.rs

    }

+    /// Remove domain metadata from the Delta log.
+    /// This creates a tombstone to logically delete the specified domain. We don't check


nit: IMO it is good style to have a paragraph break (i.e., empty line) after the one-line summary.

hmm, I'm not following what you mean. Do you mean the "We don't check" should start on the next line perhaps?

I meant

/// Remove domain metadata from the Delta log. /// /// This creates a tombstone to logically delete the specified domain. We don't check

See this clippy lint for reference.

lbhm · 2025-09-25T14:23:34Z

kernel/src/actions/mod.rs

    }

+    // Create a new DomainMetadata action to remove a domain.
+    pub(crate) fn remove(domain: String) -> Self {


I feel like remove is a somewhat misleading name for a method that is actually a constructor. I don't have a strong opinion here, but maybe something like new_tombstone communicates the function's intent more clearly?

kernel/tests/write.rs

lbhm · 2025-09-25T14:29:40Z

kernel/tests/write.rs

Should we maybe also test that domain_metadata::domain_metadata_configuration correctly returns None when called with a domain that has a tombstone?

Good call out. The last test verifies this case. It sets the domain metadata in txn1 then removes it in txn2. Then we assert that:

The log contains the tombstone record

Calling get domain metadata for that domain returns None

MannDP self-assigned this Sep 9, 2025

MannDP mentioned this pull request Sep 9, 2025

feat: support writing domain metadata (1/2) #1274

Merged

MannDP force-pushed the manndp/remove_domain_metadata branch from fd368f5 to dece4cd Compare September 11, 2025 00:23

zachschuermann mentioned this pull request Sep 11, 2025

feat!: Add row tracking writer feature #1239

Merged

MannDP force-pushed the manndp/remove_domain_metadata branch 2 times, most recently from a568124 to 2c50fd0 Compare September 11, 2025 19:03

MannDP changed the title ~~<WIP> feat: support writing domain metadata (2/2)~~ feat: support writing domain metadata (2/2) Sep 11, 2025

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

MannDP commented Sep 11, 2025

View reviewed changes

kernel/src/actions/mod.rs Show resolved Hide resolved

MannDP marked this pull request as ready for review September 11, 2025 19:05

MannDP force-pushed the manndp/remove_domain_metadata branch from 2c50fd0 to f7319a6 Compare September 17, 2025 16:13

MannDP requested review from zachschuermann, OussamaSaoudi and nicklan September 17, 2025 16:14

MannDP force-pushed the manndp/remove_domain_metadata branch from f7319a6 to 05ae907 Compare September 17, 2025 16:18

MannDP force-pushed the manndp/remove_domain_metadata branch 2 times, most recently from f4bfcd5 to ef7ca8a Compare September 24, 2025 20:35

feat: support removing domain metadata

7897691

MannDP force-pushed the manndp/remove_domain_metadata branch from ef7ca8a to 7897691 Compare September 24, 2025 20:37

zachschuermann reviewed Sep 24, 2025

View reviewed changes

Apply suggestion from @zachschuermann

27f59c6

Co-authored-by: Zach Schuermann <[email protected]>

lbhm approved these changes Sep 25, 2025

View reviewed changes

impl pr review comments

eae4394

MannDP requested a review from zachschuermann September 25, 2025 16:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: support writing domain metadata (2/2) #1275

feat: support writing domain metadata (2/2) #1275

MannDP commented Sep 9, 2025 •

edited by zachschuermann

Loading

Uh oh!

codecov bot commented Sep 9, 2025 •

edited

Loading

Uh oh!

Uh oh!

zachschuermann Sep 24, 2025

Uh oh!

MannDP Sep 24, 2025

Uh oh!

lbhm Sep 25, 2025 •

edited

Loading

Uh oh!

MannDP Sep 25, 2025

Uh oh!

Uh oh!

lbhm left a comment

Uh oh!

lbhm Sep 25, 2025 •

edited

Loading

Uh oh!

lbhm Sep 25, 2025

Uh oh!

MannDP Sep 25, 2025

Uh oh!

lbhm Sep 25, 2025

Uh oh!

lbhm Sep 25, 2025

Uh oh!

Uh oh!

lbhm Sep 25, 2025

Uh oh!

MannDP Sep 25, 2025

Uh oh!

Uh oh!

feat: support writing domain metadata (2/2) #1275

Are you sure you want to change the base?

feat: support writing domain metadata (2/2) #1275

Conversation

MannDP commented Sep 9, 2025 • edited by zachschuermann 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 Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lbhm Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lbhm left a comment

Choose a reason for hiding this comment

Uh oh!

lbhm Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MannDP commented Sep 9, 2025 •

edited by zachschuermann

Loading

codecov bot commented Sep 9, 2025 •

edited

Loading

lbhm Sep 25, 2025 •

edited

Loading

lbhm Sep 25, 2025 •

edited

Loading