-
Notifications
You must be signed in to change notification settings - Fork 119
feat: enable writes to CDF enabled tables only if append only is supported #1449
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1449 +/- ##
==========================================
+ Coverage 84.84% 84.92% +0.07%
==========================================
Files 119 119
Lines 31031 31152 +121
Branches 31031 31152 +121
==========================================
+ Hits 26329 26455 +126
+ Misses 3425 3421 -4
+ Partials 1277 1276 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kernel/src/table_configuration.rs
Outdated
| &["delta.enableChangeDataFeed", "delta.appendOnly"], | ||
| &[ChangeDataFeed, ColumnMapping, AppendOnly], | ||
| ), | ||
| Err(Error::unsupported(r#"Unknown TableFeatures: "columnMapping". Supported TableFeatures: "changeDataFeed", "appendOnly", "deletionVectors", "domainMetadata", "inCommitTimestamp", "invariants", "rowTracking", "timestampNtz", "variantType", "variantType-preview", "variantShredding-preview""#)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a confusing error message. Can we either update it to be more clear about what's wrong, or if that's a bigger change, file a follow-up to clean it up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed it to: "Found unsupported TableFeatures"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, can we open a follow-up for considering expansion to non-append only? perhaps if we track the notion of a blind append transaction?
|
Followup here: #1453 |
What changes are proposed in this pull request?
This PR enables writes to tables that have CDF enabled only if append only is also enabled.
Change Data Feed writes require cdc files to be written to the delta table for DML operations. However, delta-kernel-rs does not support writing cdc files. However, Change Data Feed appends do not require writing cdc files. Thus we enable CDF for tables where append only is enabled. Such tables are safe to write to because there will never be a DML operation that requires a cdc write.
How was this change tested?
Verify that table configuration fails on incorrect/invalid configurations.