feat: read `partition_values` in `RemoveVisitor` and remove `break` in `RowVisitor` for `RemoveVisitor` #633

sebastiantia · 2025-01-09T20:52:14Z

What changes are proposed in this pull request?

This PR updates RemoveVisitor to read partition_values
This PR removes the stray break in the implementation of RowVisitor for RemoveVisitor which prevented reading multiple Remove actions in a commit

How was this change tested?

Introduced test to parse the partition_values field from Remove actions

kernel/src/actions/visitors.rs

OussamaSaoudi-db · 2025-01-09T21:54:30Z

kernel/src/actions/visitors.rs

            if let Some(path) = getters[0].get_opt(i, "remove.path")? {
                self.removes.push(Self::visit_remove(i, path, getters)?);
-                break;


woah I didn't realize the break was there from the beginning

I mean, this has been dead code for a while, but what in the world? 🤦

good find lol!

kernel/src/actions/visitors.rs

OussamaSaoudi-db · 2025-01-09T22:03:02Z

kernel/src/table_changes/log_replay/tests.rs

@@ -515,6 +516,13 @@ async fn data_skipping_filter() {
                data_change: true,
                ..Default::default()
            }),
+            // Remove action with max value id = 5
+            Action::Remove(Remove {


I'd also like to see a case where the remove action is filtered out.

I'm not even 100% sure that data skipping works on removes. If it doesn't work and there's significant work involved in making it work, we can take that up in a followup PR.

Note that the action above with fake_path_2 would've been filtered anyway because it's paired with an add action. So the stats wasn't what caused it to be filtered out.

Right, looks like you're correct. After making the path unique for the remove action, the filter does not correctly filter the action. Investigating

Data skipping does not work reliably on removes. Even if it did work, there's technically a risk that we might filter out a remove that has stats, but fail to filter out an older add for the same file that lacks stats. For example, if the file was imported from a parquet or iceberg table, and we only added stats later.

👍 Thanks for your input, will note this and move on for now

@scovich For the purposes of CDF: Suppose data skipping failed to filter the add. The rows of the add data file should eventually be filtered by the predicate. So this should be fine right?

Looks like dataskipping looks exclusively at add actions, so this definitely is a separate PR. I do still wonder if data skipping could be leveraged for removes

I do still wonder if data skipping could be leveraged for removes

It's so tempting... but unfortunately that older add could have been removed by a replace-table operation, and have a completely incompatible schema vs. the table's current schema (on columns other than the predicate). If that happens, the read would fail at parquet read time, before we could apply the predicate to filter out the unwanted rows.

codecov · 2025-01-10T00:23:58Z

Codecov Report

Attention: Patch coverage is 92.30769% with 3 lines in your changes missing coverage. Please review.

Project coverage is 83.69%. Comparing base (9c43bf4) to head (bd932d6).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/actions/visitors.rs	92.30%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #633      +/-   ##
==========================================
+ Coverage   83.46%   83.69%   +0.23%     
==========================================
  Files          75       75              
  Lines       16918    16950      +32     
  Branches    16918    16950      +32     
==========================================
+ Hits        14120    14187      +67     
+ Misses       2143     2098      -45     
- Partials      655      665      +10

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

OussamaSaoudi-db

Just a small nit on that note. Besides that, LGTM!

kernel/src/table_changes/log_replay/tests.rs

zachschuermann

looking good! just a few comments!

kernel/src/actions/mod.rs

kernel/src/actions/visitors.rs

zachschuermann · 2025-01-10T17:16:03Z

kernel/src/actions/visitors.rs

            if let Some(path) = getters[0].get_opt(i, "remove.path")? {
                self.removes.push(Self::visit_remove(i, path, getters)?);
-                break;


good find lol!

kernel/src/actions/visitors.rs

zachschuermann

LGTM!

kernel/src/actions/visitors.rs

sebastiantia · 2025-01-14T01:01:31Z

In light of the fact that DataSkipping is not performed on Remove actions (prev discussion ref), we will be holding off from adding the stat field on Remove actions as we do not yet have a use-case it. The PR description and title should reflect this now.

This PR should now only:

handle partition values in Remove actions
remove the stray break in the implementation of RowVisitor forRemoveVisitor

nicklan

lgtm! thanks for iterating

github-actions bot assigned sebastiantia Jan 9, 2025

sebastiantia changed the title ~~feat: add stat to Removeaction~~ feat: add stat to Remove action Jan 9, 2025

sebastiantia requested a review from OussamaSaoudi-db January 9, 2025 21:44

OussamaSaoudi-db reviewed Jan 9, 2025

View reviewed changes

OussamaSaoudi-db approved these changes Jan 10, 2025

View reviewed changes

kernel/src/table_changes/log_replay/tests.rs Outdated Show resolved Hide resolved

zachschuermann reviewed Jan 10, 2025

View reviewed changes

zachschuermann requested a review from nicklan January 10, 2025 17:18

sebastiantia force-pushed the add_stat_to_remove_action branch from a59b383 to c1bf5e1 Compare January 10, 2025 18:43

sebastiantia requested review from zachschuermann and scovich January 10, 2025 18:55

zachschuermann approved these changes Jan 13, 2025

View reviewed changes

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

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

zachschuermann changed the title ~~feat: add stat to Remove action~~ feat: add stat to Remove action and parse partition values for Remove Jan 13, 2025

sebastiantia force-pushed the add_stat_to_remove_action branch from c1bf5e1 to 9823136 Compare January 13, 2025 18:15

sebastiantia changed the title ~~feat: add stat to Remove action and parse partition values for Remove~~ feat: handle partition_values in Remove actions and remove break in RemoveVisitor Jan 14, 2025

sebastiantia mentioned this pull request Jan 14, 2025

Remove action should have an optional stats field #567

Open

nicklan approved these changes Jan 14, 2025

View reviewed changes

sebastiantia force-pushed the add_stat_to_remove_action branch from 7d4643e to c9a110a Compare January 14, 2025 21:21

sebastiantia added 6 commits January 14, 2025 13:24

add to action

05b9b2d

cleanup & removal of DataFilterSkipping test

8a531c4

cleanup & split remove parsing test

6c92831

fix partition test

f668193

nits and rebase

7b43b35

remove stat field from remove actions

bd932d6

sebastiantia force-pushed the add_stat_to_remove_action branch from c9a110a to bd932d6 Compare January 14, 2025 21:24

sebastiantia changed the title ~~feat: handle partition_values in Remove actions and remove break in RemoveVisitor~~ feat: reading partition_values in RemoveVisitor and remove break in RowVisitor for RemoveVisitor Jan 14, 2025

sebastiantia changed the title ~~feat: reading partition_values in RemoveVisitor and remove break in RowVisitor for RemoveVisitor~~ feat: read partition_values in RemoveVisitor and remove break in RowVisitor for RemoveVisitor Jan 14, 2025

sebastiantia merged commit c1c1dbe into delta-io:main Jan 14, 2025
21 checks passed

sebastiantia deleted the add_stat_to_remove_action branch January 14, 2025 21:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: read `partition_values` in `RemoveVisitor` and remove `break` in `RowVisitor` for `RemoveVisitor` #633

feat: read `partition_values` in `RemoveVisitor` and remove `break` in `RowVisitor` for `RemoveVisitor` #633

sebastiantia commented Jan 9, 2025 •

edited

Loading

OussamaSaoudi-db Jan 9, 2025

scovich Jan 9, 2025

zachschuermann Jan 10, 2025

OussamaSaoudi-db Jan 9, 2025

OussamaSaoudi-db Jan 9, 2025

OussamaSaoudi-db Jan 9, 2025

sebastiantia Jan 9, 2025

scovich Jan 10, 2025

sebastiantia Jan 10, 2025

OussamaSaoudi-db Jan 10, 2025

OussamaSaoudi-db Jan 10, 2025

scovich Jan 13, 2025

codecov bot commented Jan 10, 2025 •

edited

Loading

OussamaSaoudi-db left a comment

zachschuermann left a comment

zachschuermann Jan 10, 2025

zachschuermann left a comment

sebastiantia commented Jan 14, 2025 •

edited

Loading

nicklan left a comment

feat: read partition_values in RemoveVisitor and remove break in RowVisitor for RemoveVisitor #633

feat: read partition_values in RemoveVisitor and remove break in RowVisitor for RemoveVisitor #633

Conversation

sebastiantia commented Jan 9, 2025 • edited Loading

What changes are proposed in this pull request?

How was this change tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 10, 2025 • edited Loading

Codecov Report

OussamaSaoudi-db left a comment

Choose a reason for hiding this comment

zachschuermann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachschuermann left a comment

Choose a reason for hiding this comment

sebastiantia commented Jan 14, 2025 • edited Loading

nicklan left a comment

Choose a reason for hiding this comment

feat: read `partition_values` in `RemoveVisitor` and remove `break` in `RowVisitor` for `RemoveVisitor` #633

feat: read `partition_values` in `RemoveVisitor` and remove `break` in `RowVisitor` for `RemoveVisitor` #633

sebastiantia commented Jan 9, 2025 •

edited

Loading

codecov bot commented Jan 10, 2025 •

edited

Loading

sebastiantia commented Jan 14, 2025 •

edited

Loading