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

EstherBear · 2024-06-18T20:03:14Z

Which Delta project/connector is this regarding?

Description

Update ConflictChecker to perform conflict resolution of inCommitTimestamp and complete the inCommitTimestamp support in Kernel.

How was this patch tested?

Add unit tests to verify the conflict resolution of timestamps and enablement version.

Does this PR introduce any user-facing changes?

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

EstherBear · 2024-06-18T20:03:39Z

This PR is based on #3282 merge it first.

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

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

kernel/kernel-api/src/main/java/io/delta/kernel/internal/replay/ConflictChecker.java

vkorukanti · 2024-06-28T06:45:36Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/replay/ConflictChecker.java

+ if (i == actionBatchList.size() - 1) {
+ CommitInfo commitInfo =
+ getCommitInfo(batch.getColumnVector(COMMITINFO_ORDINAL));
+ winningCommitInfoOpt = Optional.ofNullable(commitInfo);
+ }


If the goal is to get the last commit info, why not always look for the commit info, store in a variable, and the last value of the variable is the one we need?

Also, we can't assume the commit info to be present in the last action batch. A commit file could be read could generate multiple batches and the first batch in the list contains the commit info.

We couldn't always store commit info in a variable and use the last value of the variable because this scenario mentioned by @dhruvarya-db. But you are right, it's not necessarily in the last action batch.

Does it make sense to check if a batch is generated from the last commit file by checking its version from ActionWrapper?

I also add a test for this scenario.

Pushed the solution by checking version from ActionWrapper.

vkorukanti · 2024-06-28T06:48:51Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/replay/ConflictChecker.java

+ long lastWinningVersion = getLastWinningTxnVersion(winningCommits);
+ return new TransactionRebaseState(
+ lastWinningVersion,
+ getLastCommitTimestamp(


why do we need to fetch the ICT again? aren't we reading already above?

We just read the CommitInfo above but not the ICT? We still need to extract the ICT in CommitInfo?

@vkorukanti From what I understand, this will not perform an additional IO. This simply extracts the timestamp from the CommitInfo action (or the file modification timestamp if ICT is not enabled.)

Got it. Thanks for clarifying.

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

kernel/kernel-api/src/main/java/io/delta/kernel/internal/replay/ConflictChecker.java

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

vkorukanti · 2024-07-15T17:02:19Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/replay/ConflictChecker.java

+ return new TransactionRebaseState(
+ lastWinningVersion,
+ getLastCommitTimestamp(
+ engine, lastWinningVersion, winningCommits, winningCommitInfoOpt.get()));


Do you want to check that winningCommitInfoOpt is not empty?

This get is for the atomic reference but not for the optional.

Still, how do verify it is actually set? do you want to start with null and then verify it is not null?

It starts with optional.empty. And in the getLastCommitTimestamp function it will check if it's ict enabled. And if it's ict enabled and the winningCommitInfoOpt is empty, the CommitInfo.getRequiredInCommitTimestamp will raise an error. It has the same logic with Delta Spark.

vkorukanti · 2024-07-15T17:03:52Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/replay/ConflictChecker.java

@@ -88,10 +92,13 @@ public static TransactionRebaseState resolveConflicts(

 public TransactionRebaseState resolveConflicts(Engine engine) throws ConcurrentWriteException {
 List<FileStatus> winningCommits = getWinningCommitFiles(engine);
+ AtomicReference<Optional<CommitInfo>> winningCommitInfoOpt =


why is this a atmoic reference? Why can't we just use the Optional?

Because of this error:
#3283 (comment)

kernel/kernel-api/src/main/java/io/delta/kernel/internal/replay/ConflictChecker.java

vkorukanti · 2024-07-15T17:07:45Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/replay/ConflictChecker.java

+ winningCommitTimestamp = CommitInfo.getRequiredInCommitTimestamp(
+ winningCommitInfoOpt,
+ String.valueOf(lastWinningVersion),
+ snapshot.getDataPath());


why do you need the datapath here?

Just for the error message print which is consistent with Delta-Spark.

vkorukanti · 2024-07-15T17:08:07Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/replay/ConflictChecker.java

+ } else {
+ winningCommitTimestamp = CommitInfo.getRequiredInCommitTimestamp(
+ winningCommitInfoOpt,
+ String.valueOf(lastWinningVersion),


why convert to string?

Just for the error message print which is consistent with Delta-Spark.

Rather than passing these as string, may be just prepare a string which contains the lastWinningVersion and tablePath and pass it as a context to getRequiredInCommitTimestamp?

CommitInfo.getRequiredInCommitTimestamp(winningCommitInfoOpt, String.format("error...", ...))

The getRequiredInCommitTimestamp has some logic to check if winningCommitInfoOpt is empty and if it contains ict and raises different errors accordingly. So I think it's better to leave this function handle with the error messages?

vkorukanti

LGTM. Thanks for working this.

Last few comments. Once addressed, this PR is good to go.

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

vkorukanti · 2024-07-16T15:47:01Z

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

+ IN_COMMIT_TIMESTAMP_ENABLEMENT_VERSION.fromMetadata(ver2Snapshot.getMetadata)
+ assert(observedEnablementTimestamp.get == ver1Snapshot.getTimestamp(engine) + 1)
+ assert(
+ observedEnablementTimestamp.get == getInCommitTimestamp(engine, table, version = 2).get)


nit: you can remove .get from both sides.

They are different actually. Left is Optional[Long] and right is Option[Long].

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

vkorukanti · 2024-07-16T15:52:25Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/replay/ConflictChecker.java

+ } else {
+ winningCommitTimestamp = CommitInfo.getRequiredInCommitTimestamp(
+ winningCommitInfoOpt,
+ String.valueOf(lastWinningVersion),


Rather than passing these as string, may be just prepare a string which contains the lastWinningVersion and tablePath and pass it as a context to getRequiredInCommitTimestamp?

CommitInfo.getRequiredInCommitTimestamp(winningCommitInfoOpt, String.format("error...", ...))

vkorukanti · 2024-07-16T15:55:37Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/replay/ConflictChecker.java

+ return new TransactionRebaseState(
+ lastWinningVersion,
+ getLastCommitTimestamp(
+ engine, lastWinningVersion, winningCommits, winningCommitInfoOpt.get()));


Still, how do verify it is actually set? do you want to start with null and then verify it is not null?

kernel/kernel-api/src/main/java/io/delta/kernel/internal/replay/ConflictChecker.java

vkorukanti · 2024-07-16T15:58:59Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/replay/ConflictChecker.java

+ long lastWinningVersion = getLastWinningTxnVersion(winningCommits);
+ return new TransactionRebaseState(
+ lastWinningVersion,
+ getLastCommitTimestamp(


Got it. Thanks for clarifying.

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

dhruvarya-db reviewed Jun 18, 2024

View reviewed changes

EstherBear force-pushed the resolve-ict-conflict branch 3 times, most recently from 675f778 to 6975234 Compare June 20, 2024 20:40

dhruvarya-db mentioned this pull request Jun 20, 2024

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

Merged

5 tasks

EstherBear force-pushed the resolve-ict-conflict branch 6 times, most recently from a26d3b0 to 2d30ab8 Compare June 27, 2024 18:13

EstherBear mentioned this pull request Jun 27, 2024

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

Merged

5 tasks

EstherBear force-pushed the resolve-ict-conflict branch from 2d30ab8 to efd5afb Compare June 28, 2024 05:36

vkorukanti requested changes Jun 28, 2024

View reviewed changes

EstherBear added 2 commits June 28, 2024 10:02

[Kernel] Update ConflictChecker to perform conflict resolution of ICT

f836a51

[Kernel] Resolve first batch of comments of resolving ict conflict

2748b63

EstherBear force-pushed the resolve-ict-conflict branch from efd5afb to 2748b63 Compare June 28, 2024 17:30

dhruvarya-db reviewed Jun 28, 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 28, 2024

View reviewed changes

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

dhruvarya-db reviewed Jun 28, 2024

View reviewed changes

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

[Kernel] Resolve second batch of comments of resolving ict conflict

2b02d8f

dhruvarya-db reviewed Jun 28, 2024

View reviewed changes

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

dhruvarya-db approved these changes Jun 28, 2024

View reviewed changes

[Kernel] Fix indentation

ad8d1cb

vkorukanti requested changes Jul 15, 2024

View reviewed changes

[Kernel] Resolve third batch of comments of resolving ict conflict

de01558

vkorukanti requested changes Jul 16, 2024

View reviewed changes

EstherBear added 2 commits July 16, 2024 13:51

[Kernel] Resolve fourth batch of comments of resolving ict conflict

95a4917

[Kernel] Resolve fifth batch of comments of resolving ict conflict

38edfd4

vkorukanti approved these changes Jul 16, 2024

View reviewed changes

vkorukanti merged commit 4430dc1 into delta-io:master Jul 16, 2024
10 checks passed

EstherBear mentioned this pull request Jul 22, 2024

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

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

EstherBear commented Jun 18, 2024

EstherBear commented Jun 18, 2024

vkorukanti Jun 28, 2024

EstherBear Jun 28, 2024

EstherBear Jun 28, 2024

EstherBear Jun 28, 2024

EstherBear Jun 28, 2024

vkorukanti Jun 28, 2024

EstherBear Jun 28, 2024

dhruvarya-db Jun 28, 2024

vkorukanti Jul 16, 2024

vkorukanti Jul 15, 2024

EstherBear Jul 15, 2024

vkorukanti Jul 16, 2024

EstherBear Jul 16, 2024 •

edited

Loading

vkorukanti Jul 15, 2024

EstherBear Jul 15, 2024

vkorukanti Jul 15, 2024

EstherBear Jul 15, 2024 •

edited

Loading

vkorukanti Jul 15, 2024

EstherBear Jul 15, 2024 •

edited

Loading

vkorukanti Jul 16, 2024

EstherBear Jul 16, 2024

vkorukanti left a comment

vkorukanti Jul 16, 2024

EstherBear Jul 16, 2024

vkorukanti Jul 16, 2024

vkorukanti Jul 16, 2024

vkorukanti Jul 16, 2024

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

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

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

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

EstherBear Jul 16, 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

EstherBear Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EstherBear Jul 15, 2024 • edited Loading

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 Jul 16, 2024 •

edited

Loading

EstherBear Jul 15, 2024 •

edited

Loading

EstherBear Jul 15, 2024 •

edited

Loading