Implement Builder for Scans on `TableChanges` by OussamaSaoudi-db · Pull Request #521 · delta-io/delta-kernel-rs

OussamaSaoudi-db · 2024-11-21T22:57:25Z

What changes are proposed in this pull request?

This PR introduces the TableChangesScanBuilder which constructs a TableChangesScan given a TableChanges, and optionally, a predicate and schema.

This introduces the following structs:

TableChangesScan
TableChangesScanBuilder

I also introduce methods to TableChanges to get a builder:

into_scan_builder
scan_builder

How was this change tested?

I ensure that schema projection works for CDF's generated columns, predicates are correctly processed, and that the ColumnTypes are correctly created for the TableChangesScan

codecov · 2024-11-21T23:01:21Z

Codecov Report

Attention: Patch coverage is 93.22034% with 8 lines in your changes missing coverage. Please review.

Project coverage is 80.58%. Comparing base (4a0fad2) to head (53732c4).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/table_changes/scan.rs	95.53%	2 Missing and 3 partials ⚠️
kernel/src/table_changes/mod.rs	50.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #521      +/-   ##
==========================================
+ Coverage   80.43%   80.58%   +0.15%     
==========================================
  Files          62       63       +1     
  Lines       13645    13762     +117     
  Branches    13645    13762     +117     
==========================================
+ Hits        10975    11090     +115     
+ Misses       2114     2113       -1     
- Partials      556      559       +3

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

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

zachschuermann

quick pass with some early comments

scovich · 2024-11-21T23:13:13Z

        Option::<Protocol>::get_struct_field(PROTOCOL_NAME),
        Option::<SetTransaction>::get_struct_field(SET_TRANSACTION_NAME),
        Option::<CommitInfo>::get_struct_field(COMMIT_INFO_NAME),
+        Option::<Cdc>::get_struct_field(CDC_NAME),


Does this PR need to rebase? I could have sworn Cdc infra already merged?

Yep, needs a rebase

scovich

LGTM, few nits

scovich · 2024-11-21T23:35:32Z

+        match schema_opt {
+            Some(schema) => self.with_schema(schema),
+            None => self,
+        }


does this also work?

Suggested change

match schema_opt {

Some(schema) => self.with_schema(schema),

None => self,

}

schema_opt.map_or(self, |schema| self.with_schema(schema))

Ah it doesn't because it sees self is moving ownership to map_or's default and also to the closure. Not allowed 😔

map_or_else might delay the move enough for it to work?

schema_opt.map_or_else(|| self, |schema| self.with_schema(schema))

scovich · 2024-11-21T23:38:52Z

+                    // Add to read schema, store field so we can build a `Column` expression later
+                    // if needed (i.e. if we have partition columns)
+                    let physical_field =
+                        logical_field.make_physical(*self.table_changes.column_mapping_mode())?;


FYI this changes a lot to support column mapping and/or nested columns: #512

Hmm how do you think we should handle this?

This is an area where CDF is not different from a normal scan. We just need to leverage the same approach the other PR introduces. IMO we should NOT solve it here -- just let the other PR pick it up.

scovich · 2024-11-21T23:42:14Z

+                ColumnType::Selected("_change_type".to_string()),
+                ColumnType::Selected("_commit_version".to_string()),
+                ColumnType::Selected("_commit_timestamp".to_string()),


I guess these generated columns are not file-constant values, so we have to treat them like normal columns even tho they don't come from the parquet?

Correct. I considered introducing a ColumnType::Generated. I feel like this is the "right" way, but may take some more discussion and changes to kernel/scan/mod.rs. If you think ColumnType::Generated is not controversial, we can go with that.

The current plan is to treat them as selected, and later check when transforming physical to logical:

match (column_type) { ColumnType::Selected(col) => { if CDF_FIELDS.contains(col) { // Treat as CDF generated column } else { // Usual path for ColumnType::Selected } } ... }

@nicklan @zachschuermann If you feel strongly that we should do Generated or keep Selected, let me know!

Let's keep it as-is for now. Once we add support for row tracking there will be more generated columns to worry about and we can revisit with more context available.

Yeah, and I think the expression stuff will change this as well. Rather than a "selected" field, we'll just generate an expression to say, add a column with this value.

sure SGTM :)

nicklan · 2024-11-22T22:21:45Z

+                ColumnType::Selected("_change_type".to_string()),
+                ColumnType::Selected("_commit_version".to_string()),
+                ColumnType::Selected("_commit_timestamp".to_string()),


Yeah, and I think the expression stuff will change this as well. Rather than a "selected" field, we'll just generate an expression to say, add a column with this value.

zachschuermann

LGTM just a couple nits!

zachschuermann · 2024-11-22T23:10:02Z

+                ColumnType::Selected("_change_type".to_string()),
+                ColumnType::Selected("_commit_version".to_string()),
+                ColumnType::Selected("_commit_timestamp".to_string()),


sure SGTM :)

zachschuermann · 2024-11-22T23:16:20Z

+
+    #[test]
+    fn simple_table_changes_scan_builder() {
+        let path = "./tests/data/table-with-cdf";


nit: little bit confusing there's a column named 'part' but there are no partition columns. maybe just add a comment that this is a non-partitioned table?

github-actions Bot assigned OussamaSaoudi-db Nov 21, 2024

github-actions Bot added the breaking-change Public API change that could cause downstream compilation failures. Requires a major version bump. label Nov 21, 2024

zachschuermann reviewed Nov 21, 2024

View reviewed changes

scovich reviewed Nov 21, 2024

View reviewed changes

OussamaSaoudi-db changed the title ~~Implement Builder for Scans on TableChangs~~ Implement Builder for Scans on TableChanges Nov 21, 2024

OussamaSaoudi-db force-pushed the scan_builder branch 2 times, most recently from 9ac7550 to 2e1fd66 Compare November 21, 2024 23:19

scovich approved these changes Nov 21, 2024

View reviewed changes

OussamaSaoudi-db force-pushed the scan_builder branch from 2e1fd66 to a4f36d2 Compare November 22, 2024 02:51

OussamaSaoudi-db requested review from nicklan and zachschuermann November 22, 2024 04:52

OussamaSaoudi-db added 12 commits November 22, 2024 13:51

Add table changes constructor

f35d1f4

Fix docs

3566220

Add test for schema, asserting it is equal

63aba2f

Add table changes schema

c5c79e6

add schema to error message

fe8cea5

Add tablechangesscan

ef9ffa0

table_changes

58e7cfd

Remove derives

da8e8b0

Add docs

030c64a

Add comments for all_fileds

3a38232

Address pr comments

113b267

Remove duplicate line from rebase

2c53852

OussamaSaoudi-db force-pushed the scan_builder branch from 2e20fab to 2c53852 Compare November 22, 2024 21:55

remove duplicate test

f7c00a8

nicklan approved these changes Nov 22, 2024

View reviewed changes

zachschuermann approved these changes Nov 22, 2024

View reviewed changes

zachschuermann removed the breaking-change Public API change that could cause downstream compilation failures. Requires a major version bump. label Nov 22, 2024

add clarifying comment to test

53732c4

OussamaSaoudi-db merged commit db29db0 into delta-io:main Nov 23, 2024

OussamaSaoudi-db mentioned this pull request Dec 14, 2024

[tracking issue] Change Data Feed read path #440

Closed

21 tasks

Conversation

OussamaSaoudi-db commented Nov 21, 2024 • edited 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 Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zachschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scovich Nov 22, 2024 • 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!

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

OussamaSaoudi-db commented Nov 21, 2024 •

edited

Loading

codecov Bot commented Nov 21, 2024 •

edited

Loading

scovich Nov 22, 2024 •

edited

Loading