Skip to content

Conversation

@DrakeLin
Copy link
Collaborator

@DrakeLin DrakeLin commented Nov 22, 2025

What changes are proposed in this pull request?

Refactored custom is_x_supported() and is_x_enabled() functions in table_configuration.rs to use the generic is_feature_enabled(&TableFeature) and is_feature_supported(&TableFeature) functions

How was this change tested?

Several test tables had invalid protocols where delta.enableChangeDataFeed=true was set but the protocol (3,7 with table features) didn't include changeDataFeed in writerFeatures. This is invalid because table features protocol requires explicit feature listing. Fixed:

  • cdf-column-mapping-name-mode-3-7.tar.zst - added changeDataFeed to writerFeatures
  • cdf-table-with-dv.tar.zst - added changeDataFeed to writerFeatures, removed macOS AppleDouble cruft
  • table-with-cdf/ - downgraded from protocol 3,7 (empty features) to legacy protocol 1,4 which implicitly supports CDF
  • The FFI tests used minWriterVersion: 2 but CDF requires minWriterVersion: 4

@codecov
Copy link

codecov bot commented Nov 22, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.76%. Comparing base (1259c8f) to head (4994e6e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1519      +/-   ##
==========================================
+ Coverage   84.72%   84.76%   +0.03%     
==========================================
  Files         126      126              
  Lines       35797    35677     -120     
  Branches    35797    35677     -120     
==========================================
- Hits        30330    30242      -88     
+ Misses       4006     3975      -31     
+ Partials     1461     1460       -1     

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

@github-actions github-actions bot added the breaking-change Change that require a major version bump label Nov 22, 2025
@DrakeLin DrakeLin force-pushed the drake-lin_data/refactor-x-enabled-and-supported branch 3 times, most recently from 262bf8c to ab09aea Compare December 2, 2025 22:19
Copy link
Member

@nicklan nicklan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly looks great, love the number of removed vs added lines :)

I just wonder, should we be adding back in the removed tests with valid p and ms?

@DrakeLin
Copy link
Collaborator Author

DrakeLin commented Dec 4, 2025

mostly looks great, love the number of removed vs added lines :)

I just wonder, should we be adding back in the removed tests with valid p and ms?

Which tests are you referring to? I might've lost track.

Copy link
Member

@nicklan nicklan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@DrakeLin DrakeLin force-pushed the drake-lin_data/refactor-x-enabled-and-supported branch from 42cac70 to 961326b Compare December 5, 2025 22:04
@DrakeLin DrakeLin removed the breaking-change Change that require a major version bump label Dec 5, 2025
@github-actions github-actions bot added the breaking-change Change that require a major version bump label Dec 5, 2025
@DrakeLin DrakeLin merged commit 3379cc2 into delta-io:main Dec 5, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change Change that require a major version bump

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants