feat: Add the in-commit timestamp field to CommitInfo #581

OussamaSaoudi-db · 2024-12-09T23:46:59Z

What changes are proposed in this pull request?

This PR adds the inCommitTimestamp filed to CommitInfo. This lays the groundwork for supporting ICT for table properties and for writes.

We update transaction write tests to ignore the in-commit timestamp field since the write path does not currently support in-commit timestamps.

How was this change tested?

All existing tests pass.

codecov · 2024-12-09T23:50:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.70%. Comparing base (606db20) to head (d37a191).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #581      +/-   ##
==========================================
+ Coverage   83.68%   83.70%   +0.02%     
==========================================
  Files          75       75              
  Lines       16951    16955       +4     
  Branches    16951    16955       +4     
==========================================
+ Hits        14185    14193       +8     
+ Misses       2100     2098       -2     
+ Partials      666      664       -2

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

scovich

Approach looks good, but we're using the wrong column name.

scovich · 2024-12-10T15:05:04Z

kernel/src/table_changes/log_replay.rs

+            } else if let Some(timestamp) = getters[16].get_long(i, "commitInfo.timestamp")? {
+                *self.timestamp = timestamp;


According to the Delta spec:

The commitInfo action must be the first action in the commit... [and] must include a field named inCommitTimestamp, of type long

The current CommitInfo in kernel doesn't even define that field; the timestamp field is just an example from the spec (that predates ICT) of the sorts of information a commit might include, and I believe it's something delta-spark was already emitting for years before ICT came along. I don't know why the ICT spec couldn't just "take over" that existing timestamp field, nor whether two fields necessarily have the same value.

Also, either now or as a follow-up, we should probably enforce that the commit info does indeed come first? But we'd have to verify that ICT is actually enabled first, since the presence, position, and content of the commit info action is otherwise unconstrained.

Finally, the spec doesn't specifically say what we should do with an ICT that we find in a non-ICT table? Presumably we should ignore it because it's not the timestamp that time travel would use?

Wow this is really annoying. The test tables from delta-rs seem to just pickup the timestamp field in commitInfo. Meanwhile delta-spark uses some external service to get non-ict timestamps.

The issue is that without a commitInfo-based timestamp, we can't get reliable E2E testing for timestamps. Git doesn't preserve timestamps. Perhaps for tests, I should just project the timestamp column out?

Regarding ICT enablement, this is the behaviour of delta-spark:

// If the commit has an In-Commit Timestamp, we should use that as the commit timestamp. // Note that it is technically possible for a commit range to begin with ICT commits // followed by non-ICT commits, and end with ICT commits again. Ideally, for these commits // we should use the file modification time for the first two ranges. However, this // scenario is an edge case not worth optimizing for. val ts = commitInfo .flatMap(_.inCommitTimestamp) .map(ict => new Timestamp(ict)) .getOrElse(nonICTTimestampsByVersion.get(v).orNull)

Okay so spoke to @zachschuermann and the plan is to fix the delta-rs tests by changing their field from "timestamp" to "inCommitTimestamp"

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

kernel/src/transaction.rs

+
+    // Since writing in-commit timestamps is not supported, we remove the field so it is not
+    // written to the log
+    commit_info_data_type


@zachschuermann just flagging this comment and approach to writing ICT.

seems fine :) can we add an issue link/todo so we are tracking this?

OussamaSaoudi-db · 2024-12-30T22:43:22Z

@scovich This PR has been updated to only add the ICT field to CommitInfo.

scovich

LGTM!

zachschuermann

LGTM thanks! just a couple quick things

zachschuermann · 2025-01-09T15:28:43Z

kernel/src/actions/mod.rs

+    /// The time this logical file was created, as milliseconds since the epoch. Unlike
+    /// `timestamp`, this field is guaranteed to be monotonically increase with each commit.
+    /// If in-commit timestamps are enabled, this is always required.
+    pub(crate) in_commit_timestamp: Option<i64>,


maybe also note that (when enabled) it requires that commitInfo be the first action in the commit?

While that's technically what the protocol says, delta-spark doesn't check that this is true. It simply extracts the CommitInfo if present.

I think the error handling would be more code and let us capture fewer tables than delta spark, and commitInfo being the first action doesn't seem to impact us much in this scenario.

Delta-spark doesn't check, but it also does stop looking after the first line of json. So if the commit info is not first it will appear to not exist at all. Sometimes that causes an error downstream, and sometimes it would cause a silent wrong behavior.

I would prefer to check strictly unless/until we see a need to do otherwise.

@scovich Do you recommend that we error, or just ignore the ICT

zachschuermann · 2025-01-09T15:32:27Z

kernel/src/transaction.rs

+
+    // Since writing in-commit timestamps is not supported, we remove the field so it is not
+    // written to the log
+    commit_info_data_type


seems fine :) can we add an issue link/todo so we are tracking this?

OussamaSaoudi-db · 2025-01-09T23:43:32Z

@zachschuermann Added issue here #634

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

OussamaSaoudi-db mentioned this pull request Dec 9, 2024

TableChangesScan::execute and end to end testing for CDF #580

Merged

OussamaSaoudi-db requested review from nicklan, zachschuermann and scovich and removed request for nicklan and zachschuermann December 10, 2024 00:21

scovich requested changes Dec 10, 2024

View reviewed changes

OussamaSaoudi-db marked this pull request as draft December 11, 2024 00:08

github-actions bot added the breaking-change Change that will require a version bump label Dec 11, 2024

OussamaSaoudi-db mentioned this pull request Dec 14, 2024

[tracking issue] Change Data Feed read path #440

Open

21 tasks

OussamaSaoudi-db force-pushed the cdf_ict branch from 0163c63 to 99f1321 Compare December 30, 2024 21:21

OussamaSaoudi-db changed the title ~~Support in-commit timestamps in Change Data Feed~~ Add the in-commit timestamp field to CommitInfo Dec 30, 2024

OussamaSaoudi-db commented Dec 30, 2024

View reviewed changes

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

OussamaSaoudi-db changed the title ~~Add the in-commit timestamp field to CommitInfo~~ feat: Add the in-commit timestamp field to CommitInfo Dec 30, 2024

OussamaSaoudi-db requested a review from scovich December 30, 2024 22:48

OussamaSaoudi-db mentioned this pull request Dec 30, 2024

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

Open

scovich approved these changes Jan 6, 2025

View reviewed changes

OussamaSaoudi-db removed the breaking-change Change that will require a version bump label Jan 8, 2025

zachschuermann approved these changes Jan 9, 2025

View reviewed changes

OussamaSaoudi-db added 4 commits January 9, 2025 15:45

Add ict and tests

a513a09

Update docs for ICT

bc3ff0c

Assert selection vector

db5e301

wip ict impl

c57c645

OussamaSaoudi-db added 6 commits January 9, 2025 15:45

add tmp ict fixup for writes

64efb75

remove non_ict schema

e2f44b9

fix commit

acac33c

Remove cdf changes for ict

f52b5fc

remove unused imports

ae2b7d1

Add clarifying comment for inCommitTimestamp

f8909ba

OussamaSaoudi-db force-pushed the cdf_ict branch from 55144b2 to f8909ba Compare January 9, 2025 23:45

OussamaSaoudi-db and others added 3 commits January 9, 2025 16:05

Add documentation for ICT

fb1ff2f

Merge branch 'main' into cdf_ict

7258611

Merge branch 'main' into cdf_ict

d37a191

OussamaSaoudi merged commit 616e9ac into delta-io:main Jan 15, 2025
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add the in-commit timestamp field to CommitInfo #581

feat: Add the in-commit timestamp field to CommitInfo #581

OussamaSaoudi-db commented Dec 9, 2024 •

edited

Loading

codecov bot commented Dec 9, 2024 •

edited

Loading

scovich left a comment

scovich Dec 10, 2024

OussamaSaoudi-db Dec 10, 2024 •

edited

Loading

OussamaSaoudi-db Dec 10, 2024

OussamaSaoudi-db Dec 30, 2024

zachschuermann Jan 9, 2025

OussamaSaoudi-db commented Dec 30, 2024

scovich left a comment

zachschuermann left a comment

zachschuermann Jan 9, 2025

OussamaSaoudi-db Jan 9, 2025

scovich Jan 9, 2025 •

edited

Loading

OussamaSaoudi-db Jan 9, 2025

zachschuermann Jan 9, 2025

OussamaSaoudi-db commented Jan 9, 2025

		} else if let Some(timestamp) = getters[16].get_long(i, "commitInfo.timestamp")? {
		*self.timestamp = timestamp;

feat: Add the in-commit timestamp field to CommitInfo #581

feat: Add the in-commit timestamp field to CommitInfo #581

Conversation

OussamaSaoudi-db commented Dec 9, 2024 • edited Loading

What changes are proposed in this pull request?

How was this change tested?

codecov bot commented Dec 9, 2024 • edited Loading

Codecov Report

scovich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OussamaSaoudi-db Dec 10, 2024 • edited Loading

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

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

scovich Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OussamaSaoudi-db commented Jan 9, 2025

OussamaSaoudi-db commented Dec 9, 2024 •

edited

Loading

codecov bot commented Dec 9, 2024 •

edited

Loading

OussamaSaoudi-db Dec 10, 2024 •

edited

Loading

scovich Jan 9, 2025 •

edited

Loading