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

EstherBear · 2024-06-17T04:06:21Z

Which Delta project/connector is this regarding?

Description

Add non-monotonic inCommitTimestamp with related table properties and table features to prepare for adding monotonic inCommitTimestamp in laters PRs.

How was this patch tested?

Add unit tests to verify the inCommitTimestamp and related table properties and table features when enabling the inCommitTimestamp enablement property.

Does this PR introduce any user-facing changes?

Yes, user can enable non-monotonic inCommitTimestamp by enabling its property.

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

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

dhruvarya-db · 2024-06-18T00:34:55Z

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

+    public Map<String, String> filterOutUnchangedProperties(Map<String, String> newProperties) {
+        Map<String, String> oldProperties = getConfiguration();
+        return newProperties.entrySet().stream()
+                .filter(entry -> !oldProperties.containsKey(entry.getKey()) ||


TableConfig validation is case insensitive so shouldn't this also be case insensitive?

The input of this function is the validated properties returned by the validation function whose key is same as the one in TableConfig. And the property name in Metadata is also stored in the case from TableConfig. So I think it should be fine?

kernel/kernel-api/src/main/java/io/delta/kernel/internal/util/InCommitTimestampUtils.java

dhruvarya-db · 2024-06-18T00:40:57Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/util/InCommitTimestampUtils.java

+        // (see [[ConflictChecker.checkNoMetadataUpdates]]), so we know that
+        // all winning transactions' ICT enablement status must match the read snapshot.
+        //
+        // WARNING: The Metadata() of InitialSnapshot can enable ICT by default. To ensure that


Is this actually true in Kernel?

In Kernel, the Metadata() of InitialSnapshot will not enable ICT by default. But the ICT can be enabled during the first commit using the withTableProperties API. I will update the comments.

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

dhruvarya-db · 2024-06-18T00:45:45Z

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

+      val ver1Snapshot = table.getLatestSnapshot(engine).asInstanceOf[SnapshotImpl]
+      assert(ver1Snapshot.getProtocol.getWriterFeatures.contains("inCommitTimestamp-preview"))
+      assert(
+        getInCommitTimestamp(engine, table, 1).get >= beforeCommitAttemptStartTime)


It would be good to eventually have a version of transaction builder that takes in a Clock so that this can be tested more thoroughly.

Yes. I will add it in the next PR. Or do you think I should add the Clock in this PR?

I just saw that you added it in this PR: #3282. No need to shift it to this one.

…rties and table features

dhruvarya-db · 2024-06-18T23:06:27Z

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

@@ -137,8 +176,125 @@ trait DeltaTableWriteSuiteBase extends AnyFunSuite with TestUtils {
    Optional.empty()
  }

+  def generateData(


Has this been copied as is?

Yes. I just moved it from DeltaTableWriteSuite to DeltaTableWriteSuiteBase so that both DeltaTableWriteSuite and InCommitTimestampSuite can use it.

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

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

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

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

kernel/kernel-api/src/main/java/io/delta/kernel/internal/util/InCommitTimestampUtils.java

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

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

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

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

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

vkorukanti · 2024-06-20T21:14:11Z

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

+      setTablePropAndVerify(
+        engine, tablePath, isNewTable = false, IN_COMMIT_TIMESTAMPS_ENABLED, "false", false)


How is this working? Shouldn't we throw error if the ICT is disabled?

@dhruvarya-db how does Delta-Spark handles when the ICT is disabled?

Disabling ICT by setting the enablement property to false is possible even in Delta-Spark. Do you see any problems with this? Disabling ICT this way does not remove it from the protocol though.

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

vkorukanti

Looks good!

One pending comment is: how are you verifying the tables with ICT enabled are readable (i.e we are not writing an unreadable table)?

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

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

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

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

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

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

vkorukanti · 2024-06-25T06:32:36Z

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

@@ -53,7 +55,7 @@ public static Protocol fromColumnVector(ColumnVector vector, int rowId) {
    private final int minReaderVersion;
    private final int minWriterVersion;
    private final List<String> readerFeatures;
-    private final List<String> writerFeatures;
+    private List<String> writerFeatures;


vkorukanti · 2024-06-25T06:35:58Z

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

+
+      val ver0Snapshot = table.getLatestSnapshot(engine).asInstanceOf[SnapshotImpl]
+      assert(getInCommitTimestamp(engine, table, 0).get >= beforeCommitAttemptStartTime)
+      assert(ver0Snapshot.getProtocol.getWriterFeatures.contains("inCommitTimestamp-preview"))


assertHasWriterFeature(ver0Snapshot, "inCommitTimestamp-preview"

also another utility method assertHasNoWriterFeature

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

EstherBear · 2024-06-26T01:40:04Z

next #3282

vkorukanti

Thank you for the PR!

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

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

EstherBear · 2024-06-25T20:12:43Z

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

@@ -120,11 +125,22 @@ public TransactionCommitResult commit(Engine engine, CloseableIterable<Row> data
                    "Transaction is already attempted to commit. Create a new transaction.");

            long commitAsVersion = readSnapshot.getVersion(engine) + 1;
+            CommitInfo attemptCommitInfo = generateCommitAction(engine);


The conflict resolution happens here and I think I've added a TODO comment.

EstherBear · 2024-07-30T19:29:33Z

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

@@ -147,17 +157,43 @@ public TransactionCommitResult commit(Engine engine, CloseableIterable<Row> data
        throw new ConcurrentWriteException();
    }

+    private void updateMetadata(Metadata metadata) {


Update Metadata

EstherBear · 2024-07-30T19:32:59Z

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

@@ -123,6 +134,93 @@ public static void validateWriteSupportedTable(
        }
    }

+    /**


Update Protocol

EstherBear force-pushed the add-non-monotonic-ict branch 4 times, most recently from 77b6870 to bd001c9 Compare June 17, 2024 21:54

dhruvarya-db reviewed Jun 18, 2024

View reviewed changes

EstherBear added 2 commits June 18, 2024 10:04

[Kernel] Add non-monotonic inCommitTimestamp with related table prope…

983b090

…rties and table features

[Kernel] Resolve first batch of comments of adding non-monotonic ICT

c6a6f8c

EstherBear force-pushed the add-non-monotonic-ict branch from 4aeffed to c6a6f8c Compare June 18, 2024 17:27

EstherBear mentioned this pull request Jun 18, 2024

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

Merged

5 tasks

dhruvarya-db approved these changes Jun 18, 2024

View reviewed changes

vkorukanti reviewed Jun 20, 2024

View reviewed changes

[Kernel] Resolve second batch of comments of adding non-monotonic ICT

5db0080

EstherBear force-pushed the add-non-monotonic-ict branch from 12b5538 to 5db0080 Compare June 20, 2024 20:35

vkorukanti requested changes Jun 20, 2024

View reviewed changes

EstherBear force-pushed the add-non-monotonic-ict branch from 3cac10d to 0b10947 Compare June 21, 2024 03:22

[Kernel] Resolve third batch of comments of adding non-monotonic ICT

cd0621e

EstherBear force-pushed the add-non-monotonic-ict branch from 0b10947 to cd0621e Compare June 21, 2024 14:54

vkorukanti requested changes Jun 25, 2024

View reviewed changes

EstherBear added 2 commits June 25, 2024 18:17

[Kernel] Resolve fourth batch of comments of adding non-monotonic ICT

2cfacfd

Merge branch 'master' into add-non-monotonic-ict

e706f80

vkorukanti approved these changes Jun 26, 2024

View reviewed changes

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

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

[Kernel] Resolve fifth batch of comments of adding non-monotonic ICT

71eaed9

vkorukanti approved these changes Jun 27, 2024

View reviewed changes

vkorukanti merged commit c395a63 into delta-io:master Jun 27, 2024
10 checks passed

EstherBear mentioned this pull request Jul 22, 2024

[Feature Request] Support Coordinated Commits in Delta Kernel #3406

Open

8 tasks

EstherBear commented Jul 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

EstherBear commented Jun 17, 2024

dhruvarya-db Jun 18, 2024

EstherBear Jun 18, 2024 •

edited

Loading

dhruvarya-db Jun 18, 2024

EstherBear Jun 18, 2024 •

edited

Loading

dhruvarya-db Jun 18, 2024

EstherBear Jun 18, 2024

dhruvarya-db Jun 18, 2024

dhruvarya-db Jun 18, 2024

EstherBear Jun 19, 2024 •

edited

Loading

vkorukanti Jun 20, 2024

vkorukanti Jun 20, 2024

dhruvarya-db Jun 20, 2024

vkorukanti left a comment

vkorukanti Jun 25, 2024

vkorukanti Jun 25, 2024

EstherBear commented Jun 26, 2024

vkorukanti left a comment

EstherBear Jun 25, 2024

EstherBear Jul 30, 2024

EstherBear Jul 30, 2024

		setTablePropAndVerify(
		engine, tablePath, isNewTable = false, IN_COMMIT_TIMESTAMPS_ENABLED, "false", false)

@@ @@ -123,6 +134,93 @@ public static void validateWriteSupportedTable( @@
                       }
                   }
+                  /**

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

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

Conversation

EstherBear commented Jun 17, 2024

Which Delta project/connector is this regarding?

Description

How was this patch tested?

Does this PR introduce any user-facing changes?

Choose a reason for hiding this comment

EstherBear Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EstherBear Jun 18, 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

Choose a reason for hiding this comment

EstherBear Jun 19, 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

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

EstherBear commented Jun 26, 2024

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

EstherBear Jun 18, 2024 •

edited

Loading

EstherBear Jun 18, 2024 •

edited

Loading

EstherBear Jun 19, 2024 •

edited

Loading