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

emkornfield · 2025-10-09T21:20:47Z

🥞 Stacked PR

Use this link to review incremental changes.

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

An optional stats field is part of the remove schema. This PR adds it here. While utility of stats can be limited it is important to propagate for remove_actions in transactions to appropriately record number of records removed on a remove action. It is likely also important to not drop this field when doing things like log_compaction.

BREAKING CHANGE: Adds a field in the middle of a the remove_file schema.

codecov · 2025-10-09T21:43:32Z

Codecov Report

❌ Patch coverage is 63.26531% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.85%. Comparing base (2931ebb) to head (155be2f).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/actions/visitors.rs	62.50%	9 Missing and 9 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1390      +/-   ##
==========================================
- Coverage   84.88%   84.85%   -0.03%     
==========================================
  Files         119      119              
  Lines       30987    31031      +44     
  Branches    30987    31031      +44     
==========================================
+ Hits        26303    26332      +29     
- Misses       3415     3424       +9     
- Partials     1269     1275       +6

☔ 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.

scovich

Looks good.

Please also run+fix the examples (inspect-table is already broken from previous schema changes that weren't incorporated, #1372)

emkornfield · 2025-10-15T18:40:08Z

Please also run+fix the examples (inspect-table is already broken from previous schema changes that weren't incorporated, #1372)

Hopefully this should be covered by CI now. We will see how it does

## 🥞 Stacked PR Use this [link](https://github.com/delta-io/delta-kernel-rs/pull/1281/files) to review incremental changes. - [**stack/with_data_change**](#1281) [[Files changed](https://github.com/delta-io/delta-kernel-rs/pull/1281/files)] - [stack/add_stats_to_remove](#1390) [[Files changed](https://github.com/delta-io/delta-kernel-rs/pull/1390/files/a9159f03f6260388d7bbf9f4fd2ca06f8db21f8f..7637fb90686733d81815f4fae5f9e47880f55a4f)] - [stack/remove_files](#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.

## 🥞 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.

nicklan

looks fine, but I think there's a small bug

nicklan · 2025-10-23T23:23:19Z

kernel/src/actions/visitors.rs

        let base_row_id: Option<i64> = getters[12].get_opt(row_index, "remove.baseRowId")?;
        let default_row_commit_version: Option<i64> =
            getters[13].get_opt(row_index, "remove.defaultRowCommitVersion")?;


These have to bump up too right?

Suggested change

let base_row_id: Option<i64> = getters[13].get_opt(row_index, "remove.baseRowId")?;

let default_row_commit_version: Option<i64> =

getters[14].get_opt(row_index, "remove.defaultRowCommitVersion")?;

Probably indicates we have a lack of test coverage for these

fixed and added full test that should catch this in the future.

nicklan

thanks for adding the test. lgtm!

emkornfield · 2025-10-29T22:06:01Z

@scovich at least with current CI coverage on examples this PR passes. Was there a particular command invocation that you are concerned about? If not would you mind stamping?

scovich · 2025-11-03T22:33:37Z

Please also run+fix the examples (inspect-table is already broken from previous schema changes that weren't incorporated, #1372)

Hopefully this should be covered by CI now. We will see how it does

There are no removes in the table CI ran over, so indexing errors in the remove visitor and any actions checked after that will not be exercised. But the example was already broken before this change, so we should prob just fix it separately.

scovich

LGTM

emkornfield · 2025-11-03T23:02:56Z

Please also run+fix the examples (inspect-table is already broken from previous schema changes that weren't incorporated, #1372)

Hopefully this should be covered by CI now. We will see how it does

There are no removes in the table CI ran over, so indexing errors in the remove visitor and any actions checked after that will not be exercised. But the example was already broken before this change, so we should prob just fix it separately.

#1451 to track this.

emkornfield · 2025-11-03T23:19:03Z

Thanks for the reviews merging.

emkornfield mentioned this pull request Oct 9, 2025

feat!: Add with_data_change to transaction #1281

Merged

github-actions bot assigned emkornfield Oct 9, 2025

emkornfield mentioned this pull request Oct 9, 2025

feat!: add remove_files API #1353

Open

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

emkornfield requested review from OussamaSaoudi and zachschuermann October 9, 2025 21:27

emkornfield force-pushed the stack/add_stats_to_remove branch 2 times, most recently from 08fdb5c to d959169 Compare October 9, 2025 21:41

scovich reviewed Oct 9, 2025

View reviewed changes

emkornfield marked this pull request as ready for review October 9, 2025 23:29

nicklan requested review from nicklan and removed request for OussamaSaoudi and zachschuermann October 14, 2025 23:17

emkornfield force-pushed the stack/add_stats_to_remove branch from d959169 to 05176bf Compare October 15, 2025 18:38

emkornfield requested a review from scovich October 15, 2025 18:56

emkornfield force-pushed the stack/add_stats_to_remove branch from 05176bf to 7637fb9 Compare October 15, 2025 19:57

emkornfield force-pushed the stack/add_stats_to_remove branch from 7637fb9 to 9317082 Compare October 15, 2025 20:08

emkornfield mentioned this pull request Oct 16, 2025

feat: Add write_parquet_file to ParquetHandler #1392

Open

emkornfield changed the title ~~[feat!] Add optional stats field to remove action~~ feat!: Add optional stats field to remove action Oct 21, 2025

nicklan requested changes Oct 23, 2025

View reviewed changes

emkornfield added 3 commits October 28, 2025 04:55

[feat!] Add optional stats field to remove action

83eccc4

clipp

5b6b945

fix indexing add test

c89bfcd

emkornfield force-pushed the stack/add_stats_to_remove branch from 9317082 to c89bfcd Compare October 28, 2025 05:32

emkornfield mentioned this pull request Oct 28, 2025

add deletion vector APIs to transaction #1430

Open

fix clippy

d542199

fmt

ac8252b

emkornfield requested a review from nicklan October 28, 2025 17:59

nicklan approved these changes Oct 29, 2025

View reviewed changes

scovich approved these changes Nov 3, 2025

View reviewed changes

Merge branch 'main' into stack/add_stats_to_remove

155be2f

emkornfield mentioned this pull request Nov 3, 2025

Add sample table that has all possible actions to properly exercise inspect-table code #1451

Open

emkornfield merged commit 81a8b7d into delta-io:main Nov 3, 2025
20 of 22 checks passed

+let base_row_id: Option<i64> = getters[13].get_opt(row_index, "remove.baseRowId")?;
+let default_row_commit_version: Option<i64> =
+getters[14].get_opt(row_index, "remove.defaultRowCommitVersion")?;

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

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

Uh oh!

Conversation

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

Uh oh!

🥞 Stacked PR

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

scovich left a comment

Choose a reason for hiding this comment

Uh oh!

emkornfield commented Oct 15, 2025

Uh oh!

nicklan left a comment

Choose a reason for hiding this comment

Uh oh!

nicklan Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

emkornfield Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

nicklan left a comment

Choose a reason for hiding this comment

Uh oh!

emkornfield commented Oct 29, 2025

Uh oh!

scovich commented Nov 3, 2025

Uh oh!

scovich left a comment

Choose a reason for hiding this comment

Uh oh!

emkornfield commented Nov 3, 2025

Uh oh!

emkornfield commented Nov 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

emkornfield commented Oct 9, 2025 •

edited

Loading

codecov bot commented Oct 9, 2025 •

edited

Loading