refactor: unify Reader/WriterFeature into a single TableFeature #1345

ding-young · 2025-09-24T09:17:05Z

What changes are proposed in this pull request?

As suggested in above issue, this pr

adds validation check for given (reader_features, writer_features) on Protocol::try_new
refactors ReaderFeature WriterFeature into single TableFeature

so that we can check only writer feature like

// If protocol is created via Protocol::try_new(), then it is valid
if !protocol.has_write_feature(&TableFeature::VariantType)

Note that there are three feature types, and the Unknown variant is for handling TableFeature::Unknown which is either Writer or ReaderWriter based on its actual usage.

pub(crate) enum FeatureType {
    Writer,
    ReaderWriter,
    Unknown,
}

How was this change tested?

Yes, added new test

kernel/src/actions/mod.rs

kernel/src/table_features/mod.rs

kernel/src/actions/mod.rs

codecov · 2025-09-25T20:47:23Z

Codecov Report

❌ Patch coverage is 98.67110% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.84%. Comparing base (d3c7323) to head (f70b355).
⚠️ Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/actions/mod.rs	98.70%	0 Missing and 2 partials ⚠️
kernel/src/table_configuration.rs	93.75%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1345      +/-   ##
==========================================
+ Coverage   84.81%   84.84%   +0.03%     
==========================================
  Files         113      113              
  Lines       28647    28721      +74     
  Branches    28647    28721      +74     
==========================================
+ Hits        24297    24369      +72     
  Misses       3196     3196              
- Partials     1154     1156       +2

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

OussamaSaoudi · 2025-10-02T23:48:23Z

kernel/src/actions/mod.rs

+                    matches!(
+                        feature.feature_type(),
+                        FeatureType::Writer | FeatureType::Unknown
+                    ) || reader_features.contains(feature)


There's never a case where we allow a reader feature and not a writer featurer.

Suggested change

) || reader_features.contains(feature)

)

kernel/src/actions/mod.rs

OussamaSaoudi · 2025-10-02T23:52:45Z

kernel/src/actions/mod.rs

+            (Some(reader_features), None) => {
+                if *reader_features == vec![TableFeature::ColumnMapping] {
+                    Ok(())
+                } else {
+                    Err(Error::invalid_protocol(
+                        "Reader features should be present in writer features",
+                    ))
+                }


We won't allow a case where there are reader features but no writer features. So we can keep this simple.

Suggested change

(Some(reader_features), None) => {

if *reader_features == vec![TableFeature::ColumnMapping] {

Ok(())

} else {

Err(Error::invalid_protocol(

"Reader features should be present in writer features",

))

}

(Some(reader_features), None) => {

Err(Error::invalid_protocol(

"Reader features should be present in writer features",

))

There were existing test cases where we allowed reader feature (ColumnMapping) without writer feature because of #1124.

delta-kernel-rs/kernel/tests/write.rs

Lines 905 to 917 in 82fa82e

// TODO: (#1124) we don't actually support column mapping writes yet, but have some

// tests that do column mapping on writes. For now omit the writer feature to let tests

// run, but after actual support this should be enabled.

let table_url = create_table(

store.clone(),

table_location,

table_schema.clone(),

&[],

true,

vec!["variantType", "variantShredding-preview", "columnMapping"],

vec!["variantType", "variantShredding-preview"],

)

.await?;

Also, previous feature check logic for ColumnMapping relied on reader feature instead of writer feature.

delta-kernel-rs/kernel/src/table_features/column_mapping.rs

Lines 37 to 42 in 82fa82e

// NOTE: The table property is optional even when the feature is supported, and is allowed

// (but should be ignored) even when the feature is not supported. For details see

// https://github.com/delta-io/delta/blob/master/PROTOCOL.md#column-mapping

(Some(mode), 2) => mode,

(Some(mode), 3) if protocol.has_reader_feature(&ReaderFeature::ColumnMapping) => mode,

_ => ColumnMappingMode::None,

Should we update this logic and remove the exception for ColumnMapping as well?

@zachschuermann what's the context of #1124? I don't know of any reason for there to be a reader feature present but not the equivalent writer feature.

there isn't. it's an old bug that we temporarily benefitted from since it let us do column mapping read tests without actually having write support. the ideal path forward here IMO is we change to (1) "support" column mapping as a table feature (RW) but then (2) if users attempt writes to a column mapped table we block it in ensure_write_supported

Thank you for clarification. I'll make the changes.

kernel/src/actions/mod.rs

DrakeLin · 2025-10-16T18:00:12Z

@ding-young Did you get a chance to look at this PR again?

ding-young · 2025-10-20T03:57:06Z

@ding-young Did you get a chance to look at this PR again?

@DrakeLin I was waiting for answer to #1345 (comment) before proceeding. I would appreciate it if you could provide clarification, as it will determine how I should update the code logic and the existing unit tests.

DrakeLin · 2025-10-22T17:18:35Z

@ding-young Sounds great, I think zach answered your question. Otherwise, once we address Oussama's comments, I think the PR looks good to me :D

ding-young · 2025-10-24T17:17:30Z

kernel/src/actions/mod.rs

+                Some(vec![TableFeature::VariantType]),
+                Some(vec![
+                    TableFeature::VariantType,
+                    TableFeature::DeletionVectors,
+                ]),
+                "Writer features must be Writer-only or also listed in reader features",


Answer to https://github.com/delta-io/delta-kernel-rs/pull/1345/files#r2400410008

I added that logic to catch cases where an RW feature is listed in the writer features but not in the reader features. Without this, the corresponding test case would fail. Do you mean we don't need to handle this case as well?

Yeah, since we are consolidating features into TableFeatures, there can only be 3 case for RW features:

Exists in Reader + Writer features

Existing in Writer Features only

Neither

For WriterOnly features:

Exists in Writer Features only

Neither

I think the idea in this test case is for TableFeatures that are WriterFeatures, they shouldn't exist in the ReaderFeatures?

ding-young · 2025-10-24T17:20:22Z

kernel/tests/write.rs

+#[ignore]
 #[tokio::test]
 async fn test_append_variant() -> Result<(), Box<dyn std::error::Error>> {


Now, if we include column mapping write in the writer features, it triggers an unsupported writer feature error, so this test doesn’t pass. I considered a few ways to bypass it, but for now, I just ignored the test. I’d be glad to hear any other suggestions.

Let's add to writer features then block it when we start a transaction

or - do we even need column mapping here? can we just remove it?

github-actions bot assigned ding-young Sep 24, 2025

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

ding-young commented Sep 24, 2025

View reviewed changes

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

zachschuermann requested review from DrakeLin, OussamaSaoudi, nicklan, scovich and zachschuermann and removed request for nicklan and scovich September 24, 2025 23:21

OussamaSaoudi reviewed Sep 25, 2025

View reviewed changes

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

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

ding-young added 3 commits October 1, 2025 20:29

refactor: Unify Reader/WriterFeature into a single TableFeature

50c1a5b

refactor: add separate validation function for TableFeature

136e32d

feat: validate table feature on protocol creation

f70b355

ding-young force-pushed the refactor-table-feature branch from a1ed296 to f70b355 Compare October 1, 2025 15:08

OussamaSaoudi requested changes Oct 3, 2025

View reviewed changes

OussamaSaoudi removed the request for review from zachschuermann October 7, 2025 21:24

ding-young added 2 commits October 25, 2025 01:54

remove exception for ColumnMapping

8ac58e9

Merge remote-tracking branch 'upstream/main' into refactor-table-feature

3d35911

ding-young commented Oct 24, 2025

View reviewed changes

	// TODO: (#1124) we don't actually support column mapping writes yet, but have some
	// tests that do column mapping on writes. For now omit the writer feature to let tests
	// run, but after actual support this should be enabled.
	let table_url = create_table(
	store.clone(),
	table_location,
	table_schema.clone(),
	&[],
	true,
	vec!["variantType", "variantShredding-preview", "columnMapping"],
	vec!["variantType", "variantShredding-preview"],
	)
	.await?;

	// NOTE: The table property is optional even when the feature is supported, and is allowed
	// (but should be ignored) even when the feature is not supported. For details see
	// https://github.com/delta-io/delta/blob/master/PROTOCOL.md#column-mapping
	(Some(mode), 2) => mode,
	(Some(mode), 3) if protocol.has_reader_feature(&ReaderFeature::ColumnMapping) => mode,
	_ => ColumnMappingMode::None,

Uh oh!

refactor: unify Reader/WriterFeature into a single TableFeature #1345

Are you sure you want to change the base?

refactor: unify Reader/WriterFeature into a single TableFeature #1345

Uh oh!

Conversation

ding-young commented Sep 24, 2025 • 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!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

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!

Uh oh!

Uh oh!

DrakeLin commented Oct 16, 2025

Uh oh!

ding-young commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrakeLin commented Oct 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrakeLin Oct 24, 2025 • 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!

ding-young Oct 24, 2025 • 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!

DrakeLin Oct 24, 2025 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ding-young commented Sep 24, 2025 •

edited

Loading

codecov bot commented Sep 25, 2025 •

edited

Loading

ding-young commented Oct 20, 2025 •

edited

Loading

DrakeLin Oct 24, 2025 •

edited

Loading

ding-young Oct 24, 2025 •

edited

Loading

DrakeLin Oct 24, 2025 •

edited

Loading