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.80%. Comparing base (e05af8c) to head (a7aefc0).
⚠️ 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.78%   84.80%   +0.01%     
==========================================
  Files         118      118              
  Lines       30291    30325      +34     
  Branches    30291    30325      +34     
==========================================
+ Hits        25683    25716      +33     
  Misses       3382     3382              
- Partials     1226     1227       +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`.

kernel/src/actions/mod.rs

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.

kernel/src/actions/mod.rs

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

OussamaSaoudi

Nice tests! Just some code reshuffling then we're good to go 👍

kernel/src/transaction/mod.rs

OussamaSaoudi · 2025-10-02T23:27:38Z

kernel/src/transaction/mod.rs

+            if dm.is_internal() {
                return Err(Error::Generic(
                    "Cannot modify domains that start with 'delta.' as those are system controlled"
                        .to_string(),
                ));
            }
-            if !domains.insert(domain_metadata.domain()) {
+
+            if !seen_domains.insert(dm.domain()) {
                return Err(Error::Generic(format!(
                    "Metadata for domain {} already specified in this transaction",
-                    domain_metadata.domain()
+                    dm.domain()
                )));
            }


These feel like checks that should be done in with_domain_metadata:

if dm.is_internal()

if !seen_domains.insert(dm.domain())

Generally I prefer to fail early. Feel free to make this a followup issue.

I generally agree with the fail fast principle, but this case warrants an exception imo.

We're treating these methods as builder methods (return Self, even though there is no explicit build call) so we should present a fluent builder API. If we do the checking of the error conditions at this point, then we need to return DeltaResult<Self>, which is an awkward interface for both Rust (chaining requires try operator), but particularly for engines across the FFI boundary.

wdyt?

which is an awkward interface for both Rust (chaining requires try operator)

I was willing to deal with an awkward interface

but particularly for engines across the FFI boundary.

But this is a really good point! GJ

kernel/src/transaction/mod.rs

MannDP

tysm for the review! wanted to align on 2 high-level things so added thoughts on.

preserve pre-image of removed configuration
error checking, fail-fast or in commit

kernel/src/transaction/mod.rs

MannDP · 2025-10-03T23:03:18Z

kernel/src/transaction/mod.rs

+            if dm.is_internal() {
                return Err(Error::Generic(
                    "Cannot modify domains that start with 'delta.' as those are system controlled"
                        .to_string(),
                ));
            }
-            if !domains.insert(domain_metadata.domain()) {
+
+            if !seen_domains.insert(dm.domain()) {
                return Err(Error::Generic(format!(
                    "Metadata for domain {} already specified in this transaction",
-                    domain_metadata.domain()
+                    dm.domain()
                )));
            }


I generally agree with the fail fast principle, but this case warrants an exception imo.

We're treating these methods as builder methods (return Self, even though there is no explicit build call) so we should present a fluent builder API. If we do the checking of the error conditions at this point, then we need to return DeltaResult<Self>, which is an awkward interface for both Rust (chaining requires try operator), but particularly for engines across the FFI boundary.

wdyt?

kernel/src/transaction/mod.rs

OussamaSaoudi · 2025-10-07T22:30:01Z

I think I'm okay with deferred error handling for now. I think the flow/error handling could be improved a bit as we move to splitting the Transaction type into different states.

Regarding the pre-image of removed configuration, I think we should compute it ourselves since we're gonna have to do it anyway.

kernel/src/transaction/mod.rs

zachschuermann

getting close - one architectural question then will rereview after we close on that

zachschuermann · 2025-10-07T22:49:45Z

kernel/src/transaction/mod.rs

+        self.domain_metadatas
+            .push(DomainMetadata::remove(domain, String::new()));


I think we seem to be conflating the API communicating domain metadata additions/removals with the underlying DomainMetadata itself.

Can we simplify and make our lives easier below if we just take a set of domains to remove? Then we (1) don't have the 'fill-in-later' behavior here, and (2) immediately know if we have removals

I suppose we might need to still have something that knows our global set of domains since it's a logic error to add/remove in the same transaciton..

hmm this is interesting. if we have a separate state variable for removals, we can make it something like HashSet<String> instead. then we immediately know whether there are removals or not (point 2).

regarding point 1, the "fill-in" behavior, we can do log replay and store that value in a OnceCell or similar on the first removal. Subsequent removals can then immediately look up the configuration value to preserve. But, I think now we are adding class level state for something that could have been totally local in commit.

wdyt about adding a separate state variable as HashSet<String> for removals @zachschuermann?

resolved offline; will merge pr as-is. (can cut a follow-ups as this is internal state only - no external API)

zachschuermann

LGTM i think @DrakeLin is gonna take a stab at some quick follow up?

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 OussamaSaoudi, nicklan and zachschuermann 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

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

kernel/src/transaction/mod.rs Outdated Show resolved Hide resolved

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

OussamaSaoudi requested changes Oct 2, 2025

View reviewed changes

add cmt

535d242

MannDP commented Oct 3, 2025

View reviewed changes

MannDP requested a review from OussamaSaoudi October 3, 2025 23:14

OussamaSaoudi approved these changes Oct 7, 2025

View reviewed changes

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

zachschuermann reviewed Oct 7, 2025

View reviewed changes

nicklan removed their request for review October 14, 2025 23:13

Merge branch 'main' into manndp/remove_domain_metadata

9ade4c7

MannDP requested a review from zachschuermann October 22, 2025 18:32

Merge branch 'main' into manndp/remove_domain_metadata

a7aefc0

zachschuermann approved these changes Oct 22, 2025

View reviewed changes

DrakeLin merged commit 09a82a3 into delta-io:main Oct 22, 2025
22 checks passed

		self.domain_metadatas
		.push(DomainMetadata::remove(domain, String::new()));

Uh oh!

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

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

Uh oh!

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!

Uh oh!

Uh oh!

lbhm left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

OussamaSaoudi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

MannDP left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

OussamaSaoudi commented Oct 7, 2025

Uh oh!

Uh oh!

zachschuermann 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!

zachschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MannDP commented Sep 9, 2025 •

edited by zachschuermann

Loading

codecov bot commented Sep 9, 2025 •

edited

Loading