feat: Add in-commit timestamp support for change data feed #617

OussamaSaoudi-db · 2024-12-30T22:51:42Z

What changes are proposed in this pull request?

This adds support for in-commit timestamps when performing change data feed. Now when a commit contains commitInfo with inCommitTimestamp, that timestamp will be the one used for all changed rows in the commit.

Depends on #581

Please only review these commits.

How was this change tested?

Add tests to check that the timestamp extracted from commits containing in-commit-timestamps are the ICT instead of file modification time.

codecov · 2024-12-30T22:56:52Z

Codecov Report

Attention: Patch coverage is 96.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.15%. Comparing base (06d8dbb) to head (d295ffc).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/table_changes/log_replay.rs	86.66%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #617   +/-   ##
=======================================
  Coverage   84.14%   84.15%           
=======================================
  Files          77       77           
  Lines       17710    17801   +91     
  Branches    17710    17801   +91     
=======================================
+ Hits        14902    14980   +78     
- Misses       2096     2106   +10     
- Partials      712      715    +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

scovich

LGTM, one nit

kernel/src/table_changes/log_replay.rs

zachschuermann

few things looks good tho!

zachschuermann · 2025-01-09T16:29:23Z

kernel/src/table_changes/log_replay.rs

        for actions in action_iter {
            let actions = actions?;

            let mut visitor = PreparePhaseVisitor {
                add_paths: &mut add_paths,
                remove_dvs: &mut remove_dvs,
                has_cdc_action: &mut has_cdc_action,
+                commit_timestamp: &mut timestamp,


would this be clearer?

Suggested change

commit_timestamp: &mut timestamp,

in_commit_timestamp: &mut timestamp,

We initialize this field with the file modification timestamp, so it would be inaccurate to call it that. I do like the update you made below tho when we actually read ICT from a commitinfo.

zachschuermann · 2025-01-09T16:32:21Z

kernel/src/table_changes/log_replay.rs

@@ -136,15 +137,14 @@ impl LogReplayScanner {
    /// 2. Construct a map from path to deletion vector of remove actions that share the same path
    ///    as an add action.
    /// 3. Perform validation on each protocol and metadata action in the commit.
+    /// 4. Extract the in-commit timestamp from [`CommitInfo`] if it is present.


I can't comment on L130 above but I think we need to do some comment updates?

Good catch! I went through every mention of ICT and I think I got them all.

kernel/src/table_changes/log_replay.rs

zachschuermann · 2025-01-09T16:39:35Z

kernel/src/table_changes/log_replay/tests.rs

+            Action::CommitInfo(CommitInfo {
+                in_commit_timestamp: Some(timestamp),
+                ..Default::default()
+            }),


what happens if commit info isn't first? do we still read it? I know the protocol says it must be first with ICT enabled but I wonder what the expected behavior is when it isn't first? do we do the right thing?

(but probably don't solve here)

Discussed a little here:
#581 (comment)

I'm still quite certain that delta-spark doesn't care about the ordering because it goes through the all actions in the commit looking for CommitInfo

var commitInfo: Option[CommitInfo] = None actions.foreach { case c: AddCDCFile => cdcActions.append(c) totalFiles += 1L totalBytes += c.size case a: AddFile => totalFiles += 1L totalBytes += a.size case r: RemoveFile => totalFiles += 1L totalBytes += r.size.getOrElse(0L) case i: CommitInfo => commitInfo = Some(i) case _ => // do nothing }

I've added a check that only puts in the ICT if it is the first action in the log, but there comes a question: should we fail if it isn't the first action?

I can also revert the check that CommitInfo is first and revisit that in a future PR.

zachschuermann · 2025-01-09T16:42:13Z

kernel/src/table_changes/log_replay.rs

        Arc::new(StructType::new(vec![
            Option::<Add>::get_struct_field(ADD_NAME),
            Option::<Remove>::get_struct_field(REMOVE_NAME),
            Option::<Cdc>::get_struct_field(CDC_NAME),
            Option::<Metadata>::get_struct_field(METADATA_NAME),
            Option::<Protocol>::get_struct_field(PROTOCOL_NAME),
+            StructField::new("commitInfo", StructType::new([ict_type]), true),


though i wonder if we can do something similar to above like Option<CommitInfo>::get_struct_field(COMMIT_INFO_NAME) and get struct field inCommitTimestamp of that?

but for now at least can use COMMIT_INFO_NAME?

Suggested change

StructField::new("commitInfo", StructType::new([ict_type]), true),

StructField::new(COMMIT_INFO_NAME, StructType::new([ict_type]), true),

i wonder if we can do something similar to above like Option::get_struct_field(COMMIT_INFO_NAME) and get struct field inCommitTimestamp of that?

We would get a StructField of type CommitInfo, which we'd have to 1) get datatype, 2) cast to a struct 3) get the ICT field. So I'll stick with your suggested change 👍

zachschuermann · 2025-01-09T16:44:08Z

kernel/src/table_changes/scan_file.rs

+                Action::Cdc(cdc.clone()),
+                Action::CommitInfo(commit_info.clone()),


are these ordered? should commit info be first?

swapped ordering

Update docs for ICT Assert selection vector wip ict impl add tmp ict fixup for writes remove non_ict schema fix commit Remove cdf changes for ict remove unused imports Add clarifying comment for inCommitTimestamp Add documentation for ICT Revert "Remove cdf changes for ict" This reverts commit e2e38cb. Fix ict reading Address nits make ICT only work if it is the first row in a commit Rename and patch comments Fix naming referring to CommitInfo Patch up docs

github-actions bot assigned OussamaSaoudi-db Dec 30, 2024

OussamaSaoudi-db requested review from scovich, nicklan and zachschuermann December 30, 2024 22:55

OussamaSaoudi-db marked this pull request as ready for review December 30, 2024 22:55

OussamaSaoudi-db changed the title ~~feat: Add in-commit timestamp support for change data fede~~ feat: Add in-commit timestamp support for change data feed Jan 2, 2025

scovich approved these changes Jan 6, 2025

View reviewed changes

kernel/src/table_changes/log_replay.rs Outdated Show resolved Hide resolved

kernel/src/table_changes/log_replay.rs Outdated Show resolved Hide resolved

kernel/src/table_changes/log_replay.rs Outdated Show resolved Hide resolved

zachschuermann approved these changes Jan 9, 2025

View reviewed changes

OussamaSaoudi-db force-pushed the cdf_ict_impl branch 2 times, most recently from 7242904 to f50c202 Compare January 10, 2025 00:13

OussamaSaoudi added the merge hold Don't allow the PR to merge label Jan 10, 2025

OussamaSaoudi force-pushed the cdf_ict_impl branch from 2831885 to d295ffc Compare February 4, 2025 01:18

github-actions bot added the breaking-change Change that will require a version bump label Feb 4, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add in-commit timestamp support for change data feed #617

feat: Add in-commit timestamp support for change data feed #617

OussamaSaoudi-db commented Dec 30, 2024 •

edited

Loading

codecov bot commented Dec 30, 2024 •

edited

Loading

scovich left a comment

zachschuermann left a comment

zachschuermann Jan 9, 2025

OussamaSaoudi-db Jan 10, 2025

zachschuermann Jan 9, 2025

OussamaSaoudi-db Jan 10, 2025

zachschuermann Jan 9, 2025

zachschuermann Jan 9, 2025

OussamaSaoudi-db Jan 10, 2025

OussamaSaoudi-db Jan 10, 2025

zachschuermann Jan 9, 2025

OussamaSaoudi-db Jan 10, 2025

zachschuermann Jan 9, 2025

OussamaSaoudi-db Jan 10, 2025

	commit_timestamp: &mut timestamp,
	in_commit_timestamp: &mut timestamp,

	StructField::new("commitInfo", StructType::new([ict_type]), true),
	StructField::new(COMMIT_INFO_NAME, StructType::new([ict_type]), true),

		Action::Cdc(cdc.clone()),
		Action::CommitInfo(commit_info.clone()),

feat: Add in-commit timestamp support for change data feed #617

Are you sure you want to change the base?

feat: Add in-commit timestamp support for change data feed #617

Conversation

OussamaSaoudi-db commented Dec 30, 2024 • edited Loading

What changes are proposed in this pull request?

How was this change tested?

codecov bot commented Dec 30, 2024 • edited Loading

Codecov Report

scovich left a comment

Choose a reason for hiding this comment

zachschuermann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OussamaSaoudi-db commented Dec 30, 2024 •

edited

Loading

codecov bot commented Dec 30, 2024 •

edited

Loading