feat: introduce CDC write-side support for the Update operations #2486
Conversation
|
I think it's better to disable it until all operations (delete and merge) are supported, otherwise we cannot push any python releases until those are added |
| // Add the AddCDCFile actions that exist to the commit | ||
| actions.extend(writer.close().await?.into_iter().map(|add| { | ||
| Action::Cdc(AddCDCFile { | ||
| // This is a gnarly hack, but the action needs the nested path, not the |
There was a problem hiding this comment.
Can't we pass the write_action_type to the writer_config and then use some generic return types, so that based on the writer_config we either return Action::Cdc or Action::Add?
Then you don't have to add this hack into every operation
There was a problem hiding this comment.
writer_config is already a mess, I do not want to add more of anything there. Since I started this pull request we added another field there 🙃
I wouldn't call this hack a hack though! That makes it sounds so ugly 😆 The concern I have with making the DeltaWriter generic with regards to the Action generated is that the Add has some more expansive requirements with file stats.
One option I considered was adding an From<Add> on AddCDCFile to at least relegate this code into a better location. I'm not happy with our writer structure at the moment anyways, and it's due for some surgery but I don't want to do that surgery here 😦 `
crates/core/src/operations/update.rs
Outdated
| let input_schema = snapshot.input_schema()?; | ||
| // Wrap the scan with a CDCObserver if CDC has been abled so that the tracker can | ||
| // later be used to produce the CDC files | ||
| let scan: Arc<dyn ExecutionPlan> = match snapshot.table_config().enable_change_data_feed() { |
There was a problem hiding this comment.
If the table is at v7 writer, then we should only look at the writer_feature whether it exists or not.
In spark it's possible to disable change data feed through the config, even though the writer feature exists.
There was a problem hiding this comment.
I'm confused by this comment. Are you stating that our enable_change_data_feed() function is incorrect? I understand what portion of the protocol you're referring to here, but I'm not clear what change is being suggested
There was a problem hiding this comment.
That function only grabs it from the config, which is valid for writer versions 1..6, but for writer version >=7 we need to look at the writer's features in the protocol if ChangeDateFeed is enabled there.
How would you disable this? It doesn't make sense to me to include short-term configuration or feature flags to me. The protocol states that when the enable change data feed table-feature is enabled, that writers can optionally produce CDC files. Our writers just optionally will only create them on updates for now 😆 |
| #[cfg(feature = "cdf")] | ||
| { | ||
| writer_features.insert(WriterFeatures::ChangeDataFeed); | ||
| writer_features.insert(WriterFeatures::GeneratedColumns); |
There was a problem hiding this comment.
Is this needed? I don't remember generated columns being required
There was a problem hiding this comment.
@ion-elgreco writer versions 4-6 require this 😦 [see here](If the table is on a Writer Version starting from 4 up to 6, Generated Columns are always supported.)
Writer versions before 7 are annoying , they're annoying after 7 too 🤣
There was a problem hiding this comment.
But we are not adding the support here, right?
Protocol states this: "The value of delta.generationExpression SHOULD be parsed as a SQL expression.
Writers MUST enforce that any data writing to the table satisfy the condition ( <=> ) IS TRUE. <=> is the NULL-safe equal operator which performs an equality comparison like the = operator but returns TRUE rather than NULL if both operands are NULL"
We might only support v7 with CDF at this stage
This change introduces a `CDCTracker` which helps collect changes during merges and update. This is admittedly rather inefficient, but my hope is that this provides a place to start iterating and improving upon the writer code There is still additional work which needs to be done to handle table features properly for other code paths (see the middleware discussion we have had in Slack) but this produces CDC files for Update operations Fixes delta-io#604 Fixes delta-io#2095
This change introduces a
CDCTrackerwhich helps collect changes during merges and update. This is admittedly rather inefficient, but my hope is that this provides a place to start iterating and improving upon the writer codeThere is still additional work which needs to be done to handle table features properly for other code paths (see the middleware discussion we have had in Slack) but this produces CDC files for Update operations
Fixes #604
Fixes #2095