refactor: Make CDF use TableConfiguration and refactor log replay by OussamaSaoudi · Pull Request #645 · delta-io/delta-kernel-rs

OussamaSaoudi · 2025-01-15T00:09:29Z

What changes are proposed in this pull request?

This PR replaces the old CDF protocol and metadata checks to use TableConfiguration. This paves the way for checking deletion vector enablement, in-commit timestamp enablement, and verifying column mapping support in the future.

This PR also refactors LogReplay to clarify naming.

How was this change tested?

This is a refactor. All existing tests pass.

codecov · 2025-01-15T00:13:35Z

Codecov Report

❌ Patch coverage is 90.96045% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.83%. Comparing base (1f4e4c0) to head (ecfbaef).
⚠️ Report is 657 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/table_changes/log_replay.rs	87.50%	1 Missing and 7 partials ⚠️
kernel/src/actions/visitors.rs	41.66%	0 Missing and 7 partials ⚠️
kernel/src/table_changes/scan.rs	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #645      +/-   ##
==========================================
+ Coverage   84.78%   84.83%   +0.04%     
==========================================
  Files          88       88              
  Lines       22605    22679      +74     
  Branches    22605    22679      +74     
==========================================
+ Hits        19166    19239      +73     
+ Misses       2459     2458       -1     
- Partials      980      982       +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.

scovich · 2025-01-15T16:05:28Z

+        configuration: HashMap::from([
+            ("delta.enableChangeDataFeed".to_string(), "true".to_string()),
+            (
+                "delta.enableDeletionVectors".to_string(),
+                "true".to_string(),
+            ),
+            ("delta.columnMapping.mode".to_string(), "none".to_string()),


aside: I wish rust had better (more transparent) handling of String vs. &str... code like this gets so ugly and unwieldy.

yep the to_string everywhere is kind of gross, but at least we're aware of where the allocations are happening

scovich · 2025-01-15T16:06:37Z


+fn table_config() -> TableConfiguration {
+    let schema_string = serde_json::to_string(&get_schema()).unwrap();
+    let metadata = Metadata {


Out of curiosity, why is Metadata infallible while Protocol is fallible? Seems like any number of things could go wrong with it, such as partition column names that aren't actually in the schema?

Part of me wonders if we should make a clean break between P&M as "plain old data" vs. table configuration as the one stop shop for validation of that raw data? So then, any time you see a P or M in isolation, you have to assume it's broken in arbitrary ways. It's only "for sure" self-consistent and valid if a TC successfully wrapped it.

I think Metadata just doesn't have a constructor for kernel/rust data that does these checks. All it has is pub fn try_new_from_data(data: &dyn EngineData)

Part of me wonders if we should make a clean break between P&M as "plain old data" vs. table configuration as the one stop shop for validation of that raw data?

Ohhh so proposing that we move the validation that we do in Protocol::try_new to TC? Then this Metadata validation could also live there.

Moreover if we think of protocol as just "raw, unchecked data", I'm starting to wonder if we should move ensure_read_supported, has_writer_feature and has_reader_feature to TC?

Yeah... TC becomes the logical heart of the table, with P&M as simple raw information underneath

Note for anyone reading this thread, I'm tracking table config stuff in #650

scovich · 2025-01-15T16:13:53Z

+            Some([ReaderFeatures::DeletionVectors]),
+            Some([ReaderFeatures::ColumnMapping]),


Should one of those be WriterFeatures? Also, the spec says:

Reader features should be listed in both readerFeatures and writerFeatures simultaneously, while writer features should be listed only in writerFeatures. It is not allowed to list a feature only in readerFeatures but not in writerFeatures.

This seems to be the case for all reader features. If that's the case, then ensure_read_supported should be checking that the reader features exist in both the reader and and the writer features list for a given protocol.

scovich · 2025-01-15T16:17:11Z

+    // Note: We do not perform data skipping yet because we need to visit all add and
+    // remove actions for deletion vector resolution to be correct.
+    //
+    // Consider a scenario with a pair of add/remove actions with the same path. The add


NOTE: Indentation change, best reviewed with whitespace changes hidden:

OussamaSaoudi · 2025-01-17T17:10:12Z

+            Some::<Vec<String>>(vec![]),
+            Some::<Vec<String>>(vec![]),


Note: Creating Protocol from None is actually pretty challenging since it doesn't know the type of T in Option::<T>::None. Also, the type try_new takes for reader/writer features is Option<impl IntoIterator<Item = impl Into<String>>> shouldn't we be using impl ToString?

Into<String> and ToString have different characteristics, especially wrt String:

Into<String> for String is a move

ToString for String makes a copy

More types impl ToString than impl Into<String>

Dunno which we "should" use here...

Hmm i guess we should prefer moves. Though this is affected by the proposed changes to move all logic to TableConfiguration, so I'll leave this for now and address it in #650

OussamaSaoudi · 2025-01-19T01:28:24Z

-            let string_list: DataType = ArrayType::new(STRING, false).into();
-            let string_string_map = MapType::new(STRING, STRING, false).into();
+            let str_list: DataType = ArrayType::new(STRING, false).into();
+            let str_str_map: DataType = MapType::new(STRING, STRING, false).into();


Note: Renaming so that the lines below don't get too long.

scovich

LGTM, except one possible latent bug to double-check

scovich · 2025-01-21T22:33:03Z

-            // The only (path, deletion_vector) pairs we must track are ones whose path is the
-            // same as an `add` action.
-            remove_dvs.retain(|rm_path, _| add_paths.contains(rm_path));
+        if has_metadata {


Maybe this makes it more self-describing?

Suggested change

if has_metadata {

if metadata_changed {

Would this be better as this?

require!( !metadata_changed || table_schema.as_ref() == table_configuration.schema(), Error::change_data_feed_incompatible_schema( table_schema, table_configuration.schema() ) );

Hmm this may encourage an expensive swap

table_schema.as_ref() == table_configuration.schema() || !metadata_changed

I'll keep it using the if for now.

scovich · 2025-01-21T22:35:26Z

+                getters[14].get_int(i, "protocol.min_reader_version")?
            {
                let protocol =
-                    ProtocolVisitor::visit_protocol(i, min_reader_version, &getters[12..=15])?;
+                    ProtocolVisitor::visit_protocol(i, min_reader_version, &getters[14..=17])?;


This looks wrong (existing bug)? Wouldn't we read [14] twice (or [12] previously)?
Or does it demand the full list even tho it won't use the first one?

visit_protocol expects all fields of protocol to be passed in, even though it only reads elements 1,2, 3 (ignores 0).

visit_protocol:

require!( getters.len() == 4, Error::InternalError(format!( "Wrong number of ProtocolVisitor getters: {}", getters.len() )) ); let min_writer_version: i32 = getters[1].get(row_index, "protocol.min_writer_version")?; let reader_features: Option<Vec<_>> = getters[2].get_opt(row_index, "protocol.reader_features")?; let writer_features: Option<Vec<_>> = getters[3].get_opt(row_index, "protocol.writer_features")?; Protocol::try_new( min_reader_version, min_writer_version, reader_features, writer_features, )

I could change this, but maybe that could be another PR?

No need -- I was just verifying that we didn't have a double-use of that field by mentioning it twice.

zachschuermann

few questions/comments otherwise LGTM!

zachschuermann · 2025-02-05T23:45:39Z

+/// - `has_cdc_action` which is `true` when there is a `cdc` action present in the commit. This is
+///   used in [`cdf_commit_to_scan_batches`] to correctly generate a selection vector over the actions
+///   in the commit.
+struct ProcessedCdfCommit {


organization nit: I think we could still have the 'constructor' abstraction and make process_cdf_commit a try_new and also make cdf_commit_to_scan_batches just into_scan_batches

that (to me) feels like a nice paradigm:

// do all the work to process a commit let processed_commit = ProcessedCdfCommit::try_new(...)?; // consume self and give back the scan batches let scan_data_iter = processed_commit.into_scan_batches(...)?;

EDIT: also now revisiting it looks like this is pretty much what we had before. any reason to make them static functions instead of those methods?

Pros for new approach

Here are reasons I had in mind:

I remember comments saying that the LogReplayScanner could use a refactor back when I was building it. Ryan mentioned that the internal structure was good for respecting rule of 30 and so on, but my impression is that it was a comment on the function size/responsibility rather than the api of LogReplayScanner.

Mapping 2 functions makes the 2 phases rly clear. Also because each function does a single thing, I think the documentation got a lot simpler.

Cons

I was going to add that

in the future if we wanted to test this more thoroughly, you can test each phase individually. That was not the case before.

But then I realized: cdf_commit_to_scan_batches doesn't work without a consistent and correct ProcessedCdfCommit created by process_cdf_commit. There's a dependency between these two functions 🤔

Aside

As an aside, I have a sense that structs come in 2 flavors:

"Just data". Ex: actions like Protocol, SocketAddr, Url

Structs that need to maintain some internal consistency with its state. Ex: TableConfiguration, BTreeMap, Snapshot.

maybe my assessment is wrong, but LogReplayScanner doesn't feel like it fully fits in 2. This might be the source of your initial apprehension in the comment above. It most definitely isn't in category 1.

In any case, I'm happy to revert back to the original design :) I'd also like to get your take on this pattern for struts that I mentioned.

zachschuermann · 2025-02-06T00:00:49Z

            id,
-            name,
-            description,
+            name: getters[1].get_opt(row_index, "metaData.name")?,


interesting i just noticed we had all lowercase before.. guess we are case-insensitive?

seems like it. idk what the column name restrictions are in delta, but we should probably keep those.

Delta can be case-sensitive, but those strings are only for error messages so it shouldn't matter.

zachschuermann · 2025-02-06T00:38:27Z

    commit_files: impl IntoIterator<Item = ParsedLogPath>,
    table_schema: SchemaRef,
    physical_predicate: Option<(ExpressionRef, SchemaRef)>,
+    mut table_configuration: TableConfiguration,


I suppose this is okay since we are taking ownership of the TableConfiguration and just updating it as we go.. Though I wonder if there would be a better (read: less mutability) way to do updates to configuration leveraging Cow and some ability to construct new TableConfiguration incrementally? I don't have a proposal here, just that we may want to revisit this

Cow prolly doesn't make sense in this scenario since I think that works better for pointer-based data (like graphs, linked lists, trees, etc). I think immutability is gonna take some funky iterator stuff, but I can revisit this in the future.

zachschuermann · 2025-02-06T00:40:14Z

+                *table_configuration = TableConfiguration::try_new(
+                    metadata.unwrap_or_else(|| table_configuration.metadata().clone()),
+                    protocol.unwrap_or_else(|| table_configuration.protocol().clone()),
+                    table_configuration.table_root().clone(),
+                    commit_file.version,
+                )?;


related to comment above - this feels like the need for an API that will allow us to do better here instead of cloning

zachschuermann · 2025-02-06T00:40:44Z

+                table_schema.as_ref() == table_configuration.schema(),
+                Error::change_data_feed_incompatible_schema(
+                    table_schema,
+                    table_configuration.schema()
+                )


and will this evolve to use the new schema resolution soon?

Yep schema evolution is another PR 👍

zachschuermann · 2025-02-06T00:45:03Z

            log_segment,
            start_version,
            schema,
+            start_table_config: start_snapshot.table_configuration().clone(),


can we avoid the clone? aren't we throwing away the start_snapshot? oh i guess if this returns ref we need to clone.. hm maybe there's a case for some into_config conversions etc.? (thinking out loud - not to solve here)

Yeah start_snapshot is Arc, so we probably can't take.

scovich · 2025-04-15T18:23:58Z

            id,
-            name,
-            description,
+            name: getters[1].get_opt(row_index, "metaData.name")?,


Delta can be case-sensitive, but those strings are only for error messages so it shouldn't matter.

scovich · 2025-04-15T18:36:03Z

+                self.metadata = Some(Metadata {
+                    id,
+                    schema_string: getters[11].get(i, "metaData.schemaString")?,
+                    partition_columns: getters[12].get(i, "metaData.partitionColumns")?,
+                    configuration: configuration_map_opt.unwrap_or_else(HashMap::new),


Why not use the MetadataVisitor here?

Hmm the idea was to project out all the other columns in Metadata that we don't care about for CDF. Do you reckon it's better to just visit the entire Metadata struct?

I see... didn't realize we were pruning columns here.

scovich · 2025-04-15T18:40:56Z


-/// Ensures that change data feed is enabled in `table_properties`. See the documentation
-/// of [`TableChanges`] for more details.
-fn check_cdf_table_properties(table_properties: &TableProperties) -> DeltaResult<()> {


Maybe I'm missing something, but what code replaces check_cdf_table_properties and ensure_cdf_read_supported? Were these duplicating existing TableConfiguration code?

Yep these were essentially duplicated when I first created TableConfiguration. The idea was to keep that PR small by doing the move in two steps: 1) implement in tableconfig, 2) swap out implementations in CDF

github-actions Bot assigned OussamaSaoudi Jan 15, 2025

OussamaSaoudi force-pushed the table_changes_refactor branch from 774fd50 to 23f4b6f Compare January 15, 2025 00:20

scovich reviewed Jan 15, 2025

View reviewed changes

OussamaSaoudi force-pushed the table_changes_refactor branch from 95cac9c to 5474769 Compare January 17, 2025 06:08

github-actions Bot added the breaking-change Public API change that could cause downstream compilation failures. Requires a major version bump. label Jan 17, 2025

OussamaSaoudi commented Jan 17, 2025

View reviewed changes

OussamaSaoudi force-pushed the table_changes_refactor branch from 5474769 to 436852b Compare January 19, 2025 01:17

OussamaSaoudi commented Jan 19, 2025

View reviewed changes

OussamaSaoudi marked this pull request as ready for review January 19, 2025 01:48

OussamaSaoudi requested review from nicklan, scovich and zachschuermann January 19, 2025 01:48

OussamaSaoudi changed the title ~~[wip] refactor: Make CDF use TableConfiguration and refactor log replay~~ refactor: Make CDF use TableConfiguration and refactor log replay Jan 19, 2025

OussamaSaoudi removed the breaking-change Public API change that could cause downstream compilation failures. Requires a major version bump. label Jan 19, 2025

scovich approved these changes Jan 21, 2025

View reviewed changes

OussamaSaoudi force-pushed the table_changes_refactor branch from fae5a21 to 0ece561 Compare January 22, 2025 01:03

github-actions Bot added the breaking-change Public API change that could cause downstream compilation failures. Requires a major version bump. label Jan 22, 2025

OussamaSaoudi force-pushed the table_changes_refactor branch 2 times, most recently from 552c5f0 to 7243c36 Compare January 29, 2025 02:03

OussamaSaoudi mentioned this pull request Jan 29, 2025

[WIP] Move protocol validation to TableConfiguration #665

Closed

OussamaSaoudi removed the breaking-change Public API change that could cause downstream compilation failures. Requires a major version bump. label Feb 3, 2025

zachschuermann approved these changes Feb 6, 2025

View reviewed changes

OussamaSaoudi force-pushed the table_changes_refactor branch from 0a7c859 to 812d445 Compare April 12, 2025 21:44

OussamaSaoudi requested a review from zachschuermann April 15, 2025 17:01

scovich reviewed Apr 15, 2025

View reviewed changes

Use table config in cdf

eec0cfc

OussamaSaoudi force-pushed the table_changes_refactor branch from 5380643 to eec0cfc Compare April 30, 2025 22:00

hntd187 added 2 commits May 6, 2025 09:07

Merge branch 'main' into table_changes_refactor

3244e67

Merge branch 'main' into table_changes_refactor

65c231e

github-actions Bot added the breaking-change Public API change that could cause downstream compilation failures. Requires a major version bump. label May 8, 2025

fix imports on merge conflict

ecfbaef

OussamaSaoudi mentioned this pull request Aug 18, 2025

[EPIC] Change Data Feed read path improvements #1177

Open

OussamaSaoudi marked this pull request as draft October 27, 2025 16:25

		Some([ReaderFeatures::DeletionVectors]),
		Some([ReaderFeatures::ColumnMapping]),

Conversation

OussamaSaoudi commented Jan 15, 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!

codecov Bot commented Jan 15, 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!

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!

Choose a reason for hiding this comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

scovich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

scovich Jan 22, 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!

zachschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OussamaSaoudi Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Pros for new approach

Cons

Aside

Uh oh!

OussamaSaoudi Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

OussamaSaoudi commented Jan 15, 2025 •

edited

Loading

codecov Bot commented Jan 15, 2025 •

edited

Loading

scovich Jan 22, 2025 •

edited

Loading

OussamaSaoudi Feb 6, 2025 •

edited

Loading

OussamaSaoudi Feb 6, 2025 •

edited

Loading