feat: Support writing to not only 3/7 protocol #693

nicklan · 2025-02-12T19:21:36Z

What changes are proposed in this pull request?

Our previous check was too strict. Now we just ensure that the protocol makes sense given what features are present/specified.

How was this change tested?

Made all existing write.rs tests also write to a protocol 1/1 table, and they all work.

nicklan · 2025-02-12T19:23:11Z

kernel/tests/write.rs

@@ -139,136 +149,174 @@ fn new_commit_info() -> DeltaResult<Box<ArrowEngineData>> {
    Ok(Box::new(ArrowEngineData::new(commit_info_batch)))
 }



It looks like there are a lot of changes in this file, but mainly it's just:

Allowingsetup to create a 3/7 or 1/1 table

adding a new function to generate both types of tables and associated store/engine/location

making each test iterate over those tables instead of just one

nicklan · 2025-02-12T19:24:03Z

kernel/tests/write.rs

-            "operationParameters": {},
-            "engineCommitInfo": {
-                "engineInfo": "default engine"
+


for basically every test I just:

removed the line like: let table = create_table(store.clone(), table_location, schema, &[]).await?; and replaced it with for (table, engine, store, table_name) in setup_tables(schema, &[]).await? {

updated any hard coded paths to rather use table_name

It looks like more changes, but if you hide whitespace you'll see it's minimal.

codecov · 2025-02-12T19:28:06Z

Codecov Report

Attention: Patch coverage is 45.45455% with 6 lines in your changes missing coverage. Please review.

Project coverage is 84.54%. Comparing base (484eb33) to head (5b3cfbc).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/actions/mod.rs	45.45%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #693      +/-   ##
==========================================
- Coverage   84.56%   84.54%   -0.02%     
==========================================
  Files          75       75              
  Lines       17550    17553       +3     
  Branches    17550    17553       +3     
==========================================
  Hits        14841    14841              
- Misses       2000     2003       +3     
  Partials      709      709

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

scovich · 2025-02-12T19:33:30Z

kernel/src/actions/mod.rs

+                    "Tables with min writer version != 7 should not have table features.",
+                ))
+            }
+            None => {


I think we still need the default case (but with a different error message).

Every writer feature > 1 introduces new capabilities kernel must support (cumulative), with no way to know whether the table actually uses those features. See the full list here.

Versions 2, 3, and 4 are especially problematic because they respectively require column invariants, check constraints, and generated cols -- none of which we could implement because they are underspecified and their implementation is highly spark-specific.

Writer version 6 is also problematic due to identity cols. We maybe could implement support for those, but it would be an annoying time sink -- even blind appends are affected.

So basically, we can only support writer versions 1 and 7.

Meanwhile, we can actually support all three reader versions, because 2 just requires column mapping which we already support.

We might be able to partly work around this because some features have an obvious effect on the table's Metadata when they're active. We can probably get away with allowing the protocol version, as long as we can prove that the table doesn't use any unsupported features?

So this does cover all cases. Note it's checking what writerFeatures is set to. So in the top two cases there is a writerFeatures in the protocol. If we're not on version 7, that's an error, if we are on 7, we validate the features as before.

The None case is then interesting. We could actually be on any version:

7 is invalid because then there should be writer features, but there aren't.

1 is valid, and we can write to it

all the rest are currently invalid.

I've updated the code to enforce that, but I got the sense DBR might go to (1,2) sometimes, so perhaps we should check and if it's 2 we can look in the metadata configuration and make sure it doesn't contain delta.invariants before continuing.

thoughts?

Agree the cases are covered correctly now.

Re (1, 2) -- agree we need probably need to do that at some point. However, in theory the table only has (1, 2) because it is using invariants; otherwise why did it the table need to upgrade in the first place?

Good candidate for a follow-on PR tho.

scovich · 2025-02-12T19:38:34Z

kernel/src/actions/mod.rs

+                require!(
+                    self.min_writer_version != 7,


Doesn't the first match arm guarantee this assertion can never fail?

No. This is the case where there was no writerFeatures specified in the protocol. So I've switched this to be a check that we're on version 1 only (but maybe we need 2, see other comment)

scovich · 2025-02-12T19:40:14Z

kernel/tests/write.rs

+    let (store_37, engine_37, table_location_37) = setup("test_table_37", true);
+    let (store_11, engine_11, table_location_11) = setup("test_table_11", true);


Why do the different tables need different engines, out of curiosity?

We pass the storage to the engine, so because we want disjoint storage, we need to have two engines

zachschuermann

LGTM one nit!

zachschuermann · 2025-02-18T16:19:22Z

kernel/src/actions/mod.rs

+                require!(
+                    self.min_writer_version == 1,
+                    Error::unsupported(
+                        "Currently delta-kernel-rs can only write to tables with min_writer_version = 1 or 7"


nit: just to match spec (or maybe do delta.minWriterVersion to match the user-facing table property?)

Suggested change

"Currently delta-kernel-rs can only write to tables with min_writer_version = 1 or 7"

"Currently delta-kernel-rs can only write to tables with protocol.minWriterVersion = 1 or 7"

Support writing to not only 3/7 protocol

edf591a

nicklan requested review from scovich and zachschuermann and removed request for scovich February 12, 2025 19:21

github-actions bot assigned nicklan Feb 12, 2025

nicklan requested review from scovich and OussamaSaoudi February 12, 2025 19:21

nicklan commented Feb 12, 2025

View reviewed changes

scovich reviewed Feb 12, 2025

View reviewed changes

only support versions 1 and 7 on write

907d418

nicklan requested a review from scovich February 13, 2025 01:42

scovich approved these changes Feb 13, 2025

View reviewed changes

roeap approved these changes Feb 15, 2025

View reviewed changes

zachschuermann approved these changes Feb 18, 2025

View reviewed changes

Merge branch 'main' into support-earlier-protocols-for-write

2a6ad83

nicklan mentioned this pull request Feb 20, 2025

Support writer version 2 #706

Open

nicklan added 2 commits February 20, 2025 15:47

Merge branch 'main' into support-earlier-protocols-for-write

7b47813

minor cleanup

5b3cfbc

nicklan changed the title ~~Support writing to not only 3/7 protocol~~ feat: Support writing to not only 3/7 protocol Feb 20, 2025

github-actions bot added the breaking-change Change that will require a version bump label Feb 20, 2025

nicklan merged commit e2bcd0b into delta-io:main Feb 20, 2025
19 of 21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support writing to not only 3/7 protocol #693

feat: Support writing to not only 3/7 protocol #693

nicklan commented Feb 12, 2025

nicklan Feb 12, 2025

nicklan Feb 12, 2025 •

edited

Loading

codecov bot commented Feb 12, 2025 •

edited

Loading

scovich Feb 12, 2025

scovich Feb 12, 2025

nicklan Feb 13, 2025

scovich Feb 13, 2025 •

edited

Loading

scovich Feb 12, 2025

nicklan Feb 13, 2025 •

edited

Loading

scovich Feb 12, 2025

nicklan Feb 13, 2025

zachschuermann left a comment

zachschuermann Feb 18, 2025

		@@ -139,136 +149,174 @@ fn new_commit_info() -> DeltaResult<Box<ArrowEngineData>> {
		Ok(Box::new(ArrowEngineData::new(commit_info_batch)))
		}

		let (store_37, engine_37, table_location_37) = setup("test_table_37", true);
		let (store_11, engine_11, table_location_11) = setup("test_table_11", true);

	"Currently delta-kernel-rs can only write to tables with min_writer_version = 1 or 7"
	"Currently delta-kernel-rs can only write to tables with protocol.minWriterVersion = 1 or 7"

feat: Support writing to not only 3/7 protocol #693

feat: Support writing to not only 3/7 protocol #693

Conversation

nicklan commented Feb 12, 2025

What changes are proposed in this pull request?

How was this change tested?

Choose a reason for hiding this comment

nicklan Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Feb 12, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scovich Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicklan Feb 13, 2025 • edited Loading

Choose a reason for hiding this 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

Choose a reason for hiding this comment

nicklan Feb 12, 2025 •

edited

Loading

codecov bot commented Feb 12, 2025 •

edited

Loading

scovich Feb 13, 2025 •

edited

Loading

nicklan Feb 13, 2025 •

edited

Loading