[Kernel] Add monotonic inCommitTimestamp and read support for inCommitTimestamp #3282

EstherBear · 2024-06-18T18:03:07Z

Which Delta project/connector is this regarding?

Description

Add read support for inCommitTimestamp and ensure the increasing monotonicity of inCommitTimestamp assuming that there are no conflicts to prepare for the complete inCommitTimestamp support with conflict resolution in Kernel.

How was this patch tested?

Add unit tests to verify that the read of inCommitTimestamp is correct and inCommitTimestamp is monotonic.

Does this PR introduce any user-facing changes?

Yes, user can enable monotonic inCommitTimestamp assuming that there are no conflicts by enabling its property.

EstherBear · 2024-06-18T18:06:06Z

This PR is based on #3276 merge it first.

...l/kernel-api/src/main/java/io/delta/kernel/exceptions/MissingInCommitTimestampException.java

dhruvarya-db · 2024-06-18T23:17:44Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/SnapshotImpl.java

@@ -121,4 +125,22 @@ public Path getLogPath() {
    public Path getDataPath() {
        return dataPath;
    }
+
+    public long getTimestamp(Engine engine) {


Please add a docstring.

Also, it looks like the Commit file be read every time getTimestamp is invoked. This is inefficient.

We should read it once and store it in a variable.

As a future optimization, this should be read as part of LogReplay.loadTableProtocolAndMetadata and then passed by SnapshotManager to Snapshot

I think it will be only read once? inCommitTimestampOpt will be present for the following reads except the first read.

You are right, thanks for pointing that out.

kernel/kernel-api/src/main/java/io/delta/kernel/internal/TransactionImpl.java

kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/InCommitTimestampSuite.scala

kernel/kernel-api/src/main/java/io/delta/kernel/internal/DeltaHistoryManager.java

kernel/kernel-api/src/main/java/io/delta/kernel/internal/TransactionImpl.java

kernel/kernel-api/src/main/java/io/delta/kernel/internal/SnapshotImpl.java

kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/InCommitTimestampSuite.scala

vkorukanti

Looks nice! Few comments!

kernel/kernel-api/src/main/java/io/delta/kernel/exceptions/MissingCommitInfoException.java

...l/kernel-api/src/main/java/io/delta/kernel/exceptions/MissingInCommitTimestampException.java

kernel/kernel-api/src/main/java/io/delta/kernel/internal/DeltaHistoryManager.java

vkorukanti · 2024-06-26T04:27:41Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/SnapshotImpl.java

@@ -41,6 +43,7 @@ public class SnapshotImpl implements Snapshot {
    private final Protocol protocol;
    private final Metadata metadata;
    private final LogSegment logSegment;
+    private Optional<Long> inCommitTimestampOpt;


Use Lazy instance to load it atmost once. See the examples of Lazy in Metadata.java

vkorukanti · 2024-06-26T04:28:56Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/SnapshotImpl.java

@@ -121,4 +125,33 @@ public Path getLogPath() {
    public Path getDataPath() {
        return dataPath;
    }
+
+    /**
+     * Returns the timestamp of the latest commit of this snapshot.


Should it say: Returns the timestamp of when this table snapshot version is created?

It can also be the file modification time as a fallback? So it's not necessarily of the time when this table snapshot version is created?

vkorukanti · 2024-06-26T04:29:28Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/SnapshotImpl.java

+            }
+            return inCommitTimestampOpt.get();
+        } else {
+            return logSegment.lastCommitTimestamp;


Is the LogSegment always initialized with the correct lastCommitTimestamp?

It's the modification time of the file. When we couldn't get ICT, it's a fallback.

kernel/kernel-api/src/main/java/io/delta/kernel/internal/TableImpl.java

kernel/kernel-api/src/main/java/io/delta/kernel/internal/actions/CommitInfo.java

…tTimestamp

EstherBear · 2024-06-27T18:13:37Z

next #3283

dhruvarya-db · 2024-06-27T21:26:40Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/SnapshotImpl.java

+        if (TableConfig.IN_COMMIT_TIMESTAMPS_ENABLED.fromMetadata(metadata)) {
+            if (!inCommitTimestampOpt.isPresent()) {
+                try {
+                    Optional<CommitInfo> commitInfoOpt = CommitInfo.getCommitInfoOpt(


Ideally, the ICT should be read part of the protocol and metadata query in LogReplay.java. Can you create an issue to track this improvement?

@EstherBear please create an issue for this.

Created #3321

vkorukanti · 2024-06-28T06:34:50Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/SnapshotImpl.java

+        if (TableConfig.IN_COMMIT_TIMESTAMPS_ENABLED.fromMetadata(metadata)) {
+            if (!inCommitTimestampOpt.isPresent()) {
+                try {
+                    Optional<CommitInfo> commitInfoOpt = CommitInfo.getCommitInfoOpt(


@EstherBear please create an issue for this.

vkorukanti · 2024-06-28T06:38:22Z

kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/InCommitTimestampSuite.scala

+
+      val ver2Snapshot = table.getLatestSnapshot(engine).asInstanceOf[SnapshotImpl]
+      val ver2Timestamp = ver2Snapshot.getTimestamp(engine)
+      assert(ver2Timestamp == ver1Timestamp + 1)


We need to verify that the tables are readable. I will merge this, but could you please address it in the next PR?

EstherBear mentioned this pull request Jun 18, 2024

[Kernel] Update ConflictChecker to perform conflict resolution of ICT #3283

Open

5 tasks

EstherBear changed the title ~~Add monotonic inCommitTimestamp and read support for inCommitTimestamp~~ [Kernel] Add monotonic inCommitTimestamp and read support for inCommitTimestamp Jun 18, 2024

dhruvarya-db reviewed Jun 18, 2024

View reviewed changes

dhruvarya-db mentioned this pull request Jun 18, 2024

[Kernel] Add non-monotonic inCommitTimestamp with related table properties and table features #3276

Merged

5 tasks

EstherBear force-pushed the add-monotonic-ict branch 8 times, most recently from a1d1196 to 1f69550 Compare June 26, 2024 01:39

dhruvarya-db reviewed Jun 26, 2024

View reviewed changes

kernel/kernel-api/src/main/java/io/delta/kernel/internal/DeltaHistoryManager.java Outdated Show resolved Hide resolved

dhruvarya-db reviewed Jun 26, 2024

View reviewed changes

kernel/kernel-api/src/main/java/io/delta/kernel/internal/DeltaHistoryManager.java Outdated Show resolved Hide resolved

dhruvarya-db reviewed Jun 26, 2024

View reviewed changes

kernel/kernel-api/src/main/java/io/delta/kernel/internal/DeltaHistoryManager.java Outdated Show resolved Hide resolved

dhruvarya-db reviewed Jun 26, 2024

View reviewed changes

kernel/kernel-api/src/main/java/io/delta/kernel/internal/DeltaHistoryManager.java Outdated Show resolved Hide resolved

dhruvarya-db reviewed Jun 26, 2024

View reviewed changes

kernel/kernel-api/src/main/java/io/delta/kernel/internal/TransactionImpl.java Outdated Show resolved Hide resolved

dhruvarya-db reviewed Jun 26, 2024

View reviewed changes

kernel/kernel-api/src/main/java/io/delta/kernel/internal/SnapshotImpl.java Show resolved Hide resolved

dhruvarya-db reviewed Jun 26, 2024

View reviewed changes

kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/InCommitTimestampSuite.scala Outdated Show resolved Hide resolved

vkorukanti requested changes Jun 26, 2024

View reviewed changes

EstherBear force-pushed the add-monotonic-ict branch from 1f69550 to ea40a0f Compare June 26, 2024 20:12

[Kernel] Add monotonic inCommitTimestamp and read support for inCommi…

7aba5e9

…tTimestamp

EstherBear force-pushed the add-monotonic-ict branch from 0e8b844 to 7aba5e9 Compare June 27, 2024 18:01

dhruvarya-db reviewed Jun 27, 2024

View reviewed changes

dhruvarya-db approved these changes Jun 27, 2024

View reviewed changes

vkorukanti approved these changes Jun 28, 2024

View reviewed changes

vkorukanti merged commit 87f0685 into delta-io:master Jun 28, 2024
10 checks passed

EstherBear mentioned this pull request Jun 28, 2024

[Kernel][Feature Request] Improvement of ICT Read #3321

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Kernel] Add monotonic inCommitTimestamp and read support for inCommitTimestamp #3282

[Kernel] Add monotonic inCommitTimestamp and read support for inCommitTimestamp #3282

EstherBear commented Jun 18, 2024

EstherBear commented Jun 18, 2024

dhruvarya-db Jun 18, 2024

dhruvarya-db Jun 18, 2024

EstherBear Jun 19, 2024

dhruvarya-db Jun 19, 2024

vkorukanti left a comment

vkorukanti Jun 26, 2024

vkorukanti Jun 26, 2024

EstherBear Jun 26, 2024

vkorukanti Jun 26, 2024

EstherBear Jun 26, 2024

EstherBear commented Jun 27, 2024

dhruvarya-db Jun 27, 2024

vkorukanti Jun 28, 2024

EstherBear Jun 28, 2024

vkorukanti Jun 28, 2024

vkorukanti Jun 28, 2024

[Kernel] Add monotonic inCommitTimestamp and read support for inCommitTimestamp #3282

[Kernel] Add monotonic inCommitTimestamp and read support for inCommitTimestamp #3282

Conversation

EstherBear commented Jun 18, 2024

Which Delta project/connector is this regarding?

Description

How was this patch tested?

Does this PR introduce any user-facing changes?

EstherBear commented Jun 18, 2024

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

vkorukanti 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

EstherBear commented Jun 27, 2024

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