feat!: Add with_data_change to transaction #1281

emkornfield · 2025-09-10T21:08:40Z

🥞 Stacked PR

Use this link to review incremental changes.

stack/with_data_change [Files changed]
- stack/add_stats_to_remove [Files changed]
  - stack/remove_files [Files changed]

This PR adds a with_data_change method to apply whether a transaction represents a data change to all the file additions (and in the future removals). This helps prevents clients from making accidentally mixing files marked with dataChange = true or false, and is in the long term the direction we want to go in.

In the future we will also like want to physically record dataChange at the commit level, and move away from per file denotation.

Some implications of this change:

The schema of add_files is now dependent on whether this flag is sent (this is to allow connectors to maintain backwards compatibility).
Change the default engine to no longer pass through dataChange at the file level but instead require using the new setter.

Testing:

Existing FFI tests verify backwards compatibility.
Write tests now call this method with the default engine and no change of output happens.
An additional test is added to verify setting the flag to false, writes the appropriate value on add actions.

BREAKING CHANGE: Engines must use with_data_change on the transaction level instead of passing it to the method. add_files_schema is moved to be scoped on a the transaction.

kernel/examples/write-table/src/main.rs

kernel/src/engine/default/parquet.rs

emkornfield · 2025-09-11T18:11:27Z

Need to look into CI failures.

codecov · 2025-09-12T04:26:30Z

Codecov Report

❌ Patch coverage is 95.37037% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.96%. Comparing base (1f21e4b) to head (a9159f0).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
ffi/src/transaction/mod.rs	91.30%	0 Missing and 2 partials ⚠️
kernel/src/engine/default/parquet.rs	87.50%	0 Missing and 1 partial ⚠️
kernel/src/row_tracking.rs	96.15%	0 Missing and 1 partial ⚠️
kernel/src/transaction/mod.rs	98.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1281      +/-   ##
==========================================
- Coverage   84.96%   84.96%   -0.01%     
==========================================
  Files         114      114              
  Lines       29439    29445       +6     
  Branches    29439    29445       +6     
==========================================
+ Hits        25012    25017       +5     
  Misses       3220     3220              
- Partials     1207     1208       +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.

nicklan

Generally good. I'm not sure we need to worry about maintaining backwards compatibility though. Let's just break things :)

nicklan · 2025-09-22T22:44:15Z

ffi/src/transaction/mod.rs

+        // Explicitly define the schema here which duplicates Transaction::add_files_schema, to demonstrate
+        // engines can still specify data change explicitly (i.e. preserve backwards compatibility for
+        // non-default engines).
+        let mut schema_fields = vec![


Why not just add a static method on Transaction that can return the right schema depending on if you want data change or not, and call that, since you have both correct schemas statically in txn

I wanted to decouple the test from add_schema code in general. I think if we are going to make more modifications to the code maintaining the static would potentially add complexity for test only code. If this is a common pattern in rust, happy to adapt it.

This is also only necessary because we want to test backwards compatibility, if we don't care about it it would be easier to cleanup.

after discussing offline, i've removed backwads compatibility and now pass this schema through from the transaction add_files_schema

kernel/src/row_tracking.rs

emkornfield · 2025-09-27T00:26:54Z

@nicklan thanks for the review, the code is now simplified to not be backwards compatible. Is the windows failure a known issue (on the surface seems like it might be unrelated to this PR but could be missing something)

kernel/src/transaction/mod.rs

nicklan

looks good, thanks for simplifying! one more suggestion on the ffi

nicklan · 2025-09-29T23:06:20Z

ffi/src/transaction/mod.rs

+///
+/// # Safety
+///
+/// Caller is responsible for passing a valid handle. CONSUMES TRANSACTION and commit info


These apis that consume the argument are inherently more dangerous because there's no compiler on the other side enforcing things like there in pure rust.

Could we just as_ref_mut() it instead, and set the bool on the txn? I think that might require a new method in handle.rs but it should be pretty trivial to write that (and if it's not stop and we can do it this way)

Will take a look at doing this. I think the current underlying function consumes the input, so I might need to adjust there as well.

I added a a new set_data_change method on Transaction and called it here (renaming this method as well) to avoid consuming the value.

Please let me know if this is what you had in mind. It seems at least at the surface as_mut on handle works here but I guess there might be subtleties

zachschuermann

looks good will finalize tomorrow - assuming we want data_change to be a txn-wide const (not per-file as it was before) cc @nicklan @scovich

zachschuermann · 2025-10-01T05:16:35Z

ffi/src/transaction/mod.rs

+                unsafe {
+                    txn_with_engine_info
+                        .shallow_copy()
+                        .as_ref()
+                        .add_files_schema()
+                }
+                .as_ref()
+                .try_into_arrow()?,


nit: preference for pulling this out to a separate binding instead of inline deep nesting

ffi/src/transaction/mod.rs

zachschuermann · 2025-10-01T05:18:14Z

kernel/src/engine/default/parquet.rs

+    /// Note that the schema does not contain the dataChange column. Users of the default engine
+    /// must add this column by calling [`crate::transaction::Transaction::with_data_change`].


Suggested change

/// Note that the schema does not contain the dataChange column. Users of the default engine

/// must add this column by calling [`crate::transaction::Transaction::with_data_change`].

/// Note that the schema does not contain the dataChange column. In order to set `data_change` flag, use [`crate::transaction::Transaction::with_data_change`].

kernel/src/transaction/mod.rs

zachschuermann · 2025-10-01T05:21:58Z

kernel/src/transaction/mod.rs

+    /// Same as [`Transaction::with_data_change`] but set the value directly instead of
+    /// using a fluent API.
+    pub fn set_data_change(&mut self, data_change: bool) {
+        self.data_change = data_change;
+    }


hm if this is mostly used for FFI how about just make this internal_api so we don't have many ways to do the same thing? (not a super strong preference, just hoping to simplify user API)

zachschuermann · 2025-10-01T05:23:32Z

kernel/src/transaction/mod.rs

+    /// Concretely, it is the expected schema for [`EngineData`] passed to [`add_files`], as it is the base
+    /// for constructing an add_file. Each row represents metadata about a
+    /// file to be added to the table. Kernel takes this information and extends it to the full add_file
+    /// action schema, adding additional fields (e.g., baseRowID) as necessary.


Suggested change

/// action schema, adding additional fields (e.g., baseRowID) as necessary.

/// action schema, adding internal fields (e.g., baseRowID) as necessary.

zachschuermann · 2025-10-01T05:23:53Z

kernel/src/transaction/mod.rs

+    ///
+    /// Note: While currently static, in the future the schema might change depending on
+    /// options set on the transaction or features enabled on the table.
+    ///


awesome, thank you so much for the docs improvements!!

oh haha realizing this is code movement :)

(but still appreciate the other docs above etc!)

nicklan

thanks, lgtm assuming you address zach's comments!

zachschuermann

looks good, sorry forgot to post some of these comments!

kernel/src/row_tracking.rs

zachschuermann · 2025-10-10T16:54:56Z

kernel/src/transaction/mod.rs

+    ///
+    /// Note: While currently static, in the future the schema might change depending on
+    /// options set on the transaction or features enabled on the table.
+    ///


oh haha realizing this is code movement :)

zachschuermann · 2025-10-10T16:55:16Z

kernel/src/transaction/mod.rs

+    ///
+    /// Note: While currently static, in the future the schema might change depending on
+    /// options set on the transaction or features enabled on the table.
+    ///


(but still appreciate the other docs above etc!)

kernel/tests/write.rs

## 🥞 Stacked PR Use this [link](https://github.com/delta-io/delta-kernel-rs/pull/1281/files) to review incremental changes. - [**stack/with_data_change**](delta-io#1281) [[Files changed](https://github.com/delta-io/delta-kernel-rs/pull/1281/files)] - [stack/add_stats_to_remove](delta-io#1390) [[Files changed](https://github.com/delta-io/delta-kernel-rs/pull/1390/files/a9159f03f6260388d7bbf9f4fd2ca06f8db21f8f..7637fb90686733d81815f4fae5f9e47880f55a4f)] - [stack/remove_files](delta-io#1353) [[Files changed](https://github.com/delta-io/delta-kernel-rs/pull/1353/files/7637fb90686733d81815f4fae5f9e47880f55a4f..94e2108cbea608b98284c634807b4f265cddfa29)] --------- This PR adds a with_data_change method to apply whether a transaction represents a data change to all the file additions (and in the future removals). This helps prevents clients from making accidentally mixing files marked with dataChange = true or false, and is in the long term the direction we want to go in. In the future we will also like want to physically record dataChange at the commit level, and move away from per file denotation. Some implications of this change: * The schema of add_files is now dependent on whether this flag is sent (this is to allow connectors to maintain backwards compatibility). * Change the default engine to no longer pass through dataChange at the file level but instead require using the new setter. Testing: 1. Existing FFI tests verify backwards compatibility. 2. Write tests now call this method with the default engine and no change of output happens. 3. An additional test is added to verify setting the flag to false, writes the appropriate value on add actions. BREAKING CHANGE: Engines must use with_data_change on the transaction level instead of passing it to the method. `add_files_schema` is moved to be scoped on a the transaction.

github-actions bot assigned emkornfield Sep 10, 2025

emkornfield changed the title ~~wip~~ feat!: Add with_data_change to transaction Sep 10, 2025

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

emkornfield commented Sep 10, 2025

View reviewed changes

kernel/examples/write-table/src/main.rs Show resolved Hide resolved

emkornfield requested review from Fokko, nicklan and zachschuermann September 10, 2025 21:19

emkornfield commented Sep 10, 2025

View reviewed changes

kernel/src/engine/default/parquet.rs Show resolved Hide resolved

emkornfield commented Sep 10, 2025

View reviewed changes

kernel/src/engine/default/parquet.rs Show resolved Hide resolved

emkornfield marked this pull request as draft September 11, 2025 18:11

emkornfield force-pushed the stack/with_data_change branch from 553b09b to 14e87e0 Compare September 13, 2025 01:13

emkornfield marked this pull request as ready for review September 13, 2025 05:14

zachschuermann requested a review from scovich September 22, 2025 17:15

nicklan reviewed Sep 22, 2025

View reviewed changes

emkornfield requested a review from nicklan September 25, 2025 18:07

emkornfield mentioned this pull request Sep 26, 2025

feat!: add remove_files API #1353

Open

emkornfield force-pushed the stack/with_data_change branch from 56720ad to 0b10e6f Compare September 27, 2025 00:09

emkornfield commented Sep 27, 2025

View reviewed changes

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

emkornfield commented Sep 27, 2025

View reviewed changes

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

OussamaSaoudi removed the request for review from scovich September 29, 2025 22:41

emkornfield force-pushed the stack/with_data_change branch from b8bbfa5 to 48d49e6 Compare September 29, 2025 22:46

nicklan reviewed Sep 29, 2025

View reviewed changes

emkornfield requested a review from nicklan September 30, 2025 05:14

Fokko approved these changes Sep 30, 2025

View reviewed changes

zachschuermann requested a review from scovich October 1, 2025 05:12

zachschuermann reviewed Oct 1, 2025

View reviewed changes

nicklan approved these changes Oct 9, 2025

View reviewed changes

emkornfield requested a review from zachschuermann October 9, 2025 17:24

emkornfield force-pushed the stack/with_data_change branch from 3ed0d98 to 67b8b31 Compare October 9, 2025 17:28

emkornfield mentioned this pull request Oct 9, 2025

feat!: Add optional stats field to remove action #1390

Open

emkornfield force-pushed the stack/with_data_change branch from 1b0849e to c20c377 Compare October 9, 2025 21:36

emkornfield added 13 commits October 15, 2025 18:38

wip

8f6315d

fix nits

f94e04a

test with_data_change=false

0b77071

don't consume

6153c54

update contract

eb0708d

update name

c84fd57

update contract again

058d568

fix fmt

76cf43c

fix docs

ba47619

address feedback

91c8061

format

66e58f0

marker for dead code

f08306f

remove arg

f431f59

emkornfield force-pushed the stack/with_data_change branch from 6da6011 to f431f59 Compare October 15, 2025 18:38

zachschuermann approved these changes Oct 15, 2025

View reviewed changes

address comments

a9159f0

emkornfield merged commit 87e3552 into delta-io:main Oct 15, 2025
22 checks passed

		/// Note that the schema does not contain the dataChange column. Users of the default engine
		/// must add this column by calling [`crate::transaction::Transaction::with_data_change`].

	/// action schema, adding additional fields (e.g., baseRowID) as necessary.
	/// action schema, adding internal fields (e.g., baseRowID) as necessary.

Uh oh!

feat!: Add with_data_change to transaction #1281

feat!: Add with_data_change to transaction #1281

Uh oh!

Conversation

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

Uh oh!

🥞 Stacked PR

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emkornfield commented Sep 11, 2025

Uh oh!

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

Uh oh!

Codecov Report

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!

Uh oh!

emkornfield commented Sep 27, 2025

Uh oh!

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!

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!

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!

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!

nicklan left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

emkornfield commented Sep 10, 2025 •

edited

Loading

codecov bot commented Sep 12, 2025 •

edited

Loading