[Kernel] Assign base row ID and default row commit version to AddFile #3894

qiyuandong-db · 2024-11-21T14:53:39Z

Which Delta project/connector is this regarding?

Description

This PR implements the row tracking support requirements in Delta Kernel, according to the Delta Protocol. Specifically, it includes:

assigning baseRowId and defaultRowCommitVersion to AddFile actions prior to committing them
maintaining the rowIdHighWaterMark of the delta.rowTracking metadata domain during the base row ID assignment, which is the highest assigned fresh row id for the table

How was this patch tested?

Added tests in RowTrackingSuite.scala. This includes unit tests and integration tests with Delta-Spark.

Does this PR introduce any user-facing changes?

No.

johanl-db

There are a few things missing from the row tracking spec, see my comment

kernel/kernel-api/src/main/java/io/delta/kernel/internal/rowtracking/RowTracking.java

johanl-db · 2024-11-21T17:42:29Z

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

+    return new KernelException(
+        "Cannot assign baseRowId to add action. "
+            + "The number of records in this data file is missing.");
+  }


As we discussed, this can be an issue: if connectors don't populate numRecords stats in the addFile action that are committed, the commit will fail if row tracking is supported (note that this is still better than today where we always fail in that case since we don't support row tracking.

Question more for kernel folks: do we some guarantee or requirement that connectors populate numRecords? Are connectors that implement writes today (if any) populating numRecords?

In any case, I would word the exception so that it puts the burden more on the connector, for example:
"All add actions must have statistics that include the number of records when writing to a Delta table with the RowTracking table feature enabled."

Question more for kernel folks: do we some guarantee or requirement that connectors populate numRecords? Are connectors that implement writes today (if any) populating numRecords?

cc @vkorukanti thoughts?

We expect the connector to populate the numRecords and other stats as it is a heavy operation and we don't want to do that in Kernel. If some protocol feature (e.g., icebergCompatV2) requires stats and they are missing in the DataFileStatus, we throw errors.

So it seems like we should update the Row Tracking protocol to indicate that the AddFile statistics must include numRecords ?

+1 on making the error message more explicit about (1) what is unsupported (cannot write to row tracking table)(2) why it is unsupported (requires numRecords to be populated in stats) (3) who is responsible/what needs to be updated for support (not supported by this kernel integration/engine)

Thanks for the suggestion! I’ve updated the error message to better explain the situation and our expectations.

So it seems like we should update the Row Tracking protocol to indicate that the AddFile statistics must include numRecords ?

I'm not sure if this should go into protocol. The numRecords is used for assigning baseRowId, but we might have alternative ways to obtain this information (e.g. passing it directly from the connector, or potentially computing it within the Kernel ourselves). It seems more like an expectation for connectors to provide this data.

qiyuandong-db · 2024-12-09T08:40:04Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/rowtracking/RowTracking.java

+   * @return an {@link CloseableIterable} of data actions with base row IDs and default row commit
+   *     versions assigned
+   */
+  public static CloseableIterable<Row> assignBaseRowIdAndDefaultRowCommitVersion(


Another possible approach is to assign baseRowId and defaultRowCommitVersion when creating AddFile actions.

However, this would duplicate logic across all places where AddFile actions are created (currently just Transaction.generateAppendActions, but potentially more in the future I guess). Plus, baseRowId assignment is stateful, making it tricky if AddFile actions are created in multiple steps/places.

I chose to handle it here for now to keep things centralized, even though it requires converting AddFile rows back to actions, updating them, and converting back to rows.

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

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

kernel/kernel-api/src/main/java/io/delta/kernel/internal/rowtracking/RowTracking.java

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

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

scottsand-db

Looks great! Thanks for making this. Left some comments.

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

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

kernel/kernel-api/src/main/java/io/delta/kernel/internal/rowtracking/RowTracking.java

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

kernel/kernel-api/src/main/java/io/delta/kernel/internal/rowtracking/RowTracking.java

scottsand-db · 2024-12-16T17:40:19Z

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

+   * @param protocol the protocol to check
+   * @return true if the protocol supports row tracking, false otherwise
+   */
+  public static boolean isRowTrackingSupported(Protocol protocol) {


Question: where (probably in future PRs?) will you check that RowTracking is enabled (which is strictly stronger than supported?

https://github.com/delta-io/delta/blob/master/PROTOCOL.md#row-tracking

We will not be checking whether row tracking is enabled in this PR or in planned future ones. The current goal is to add a minimal implementation to ensure row tracking is supported in Delta Kernel. Addressing the enabled requirement is outside the current scope.

I imagine that in the future, we may need to check it is enabled when 1) reconstructing stable row ID / row commit version during reads, and 2) preserving stable row ID / row commit version during writes (e.g., for handling UPDATE and DELETE operations).

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

scottsand-db

Looks great! Just asked for a few minor changes. Thanks!

Super clean btw; glad we did that refactor in another PR!

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

kernel/kernel-api/src/main/java/io/delta/kernel/internal/rowtracking/RowTracking.java

scottsand-db · 2025-01-06T17:11:28Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/rowtracking/RowTracking.java

+   *     versions assigned
+   */
+  public static CloseableIterable<Row> assignBaseRowIdAndDefaultRowCommitVersion(
+      SnapshotImpl snapshot, long commitVersion, CloseableIterable<Row> dataActions) {


It seems like snapshot is only being used for getProtocol() now, right?

Can we just pass in the protocol? So we aren't coding to a specific implementation of snapshot (SnapshotImpl)

snapshot is actually also used to read the row ID high watermark from the domain metadata.

final AtomicLong currRowIdHighWatermark = new AtomicLong(readRowIdHighWaterMark(snapshot));

And this uses the SnapshotImpl API getDomainMetadataMap.

scottsand-db · 2025-01-06T17:12:21Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/rowtracking/RowTracking.java

+   * plus 1. The high watermark is then incremented by the number of records in the file. For
+   * actions missing a default row commit version, assigns the specified commit version.
+   *
+   * @param snapshot the current snapshot of the table


This is a static function, right? So what does "current" mean? Can you give some temporal context of when this method should be called duration a transaction.

Further, it is only called during transactions (writes). Would it ever be called during reads?

By "current snapshot" I mean the snapshot that this transaction is reading at, i.e. the TransactionImpl.readSnapshot.

Further, it is only called during transactions (writes). Would it ever be called during reads?

Yes, it is only called during transactions (writes), and won't be called for reads.

I've updated the doc to make it clearer and provided more context. Thanks!

kernel/kernel-api/src/main/java/io/delta/kernel/internal/rowtracking/RowTracking.java

johanl-db

Looks good, we need to ensure writing to a table with row tracking supported works on tables that are on writer version 3 (includes invariants) but don't contain any actual invariant

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

johanl-db · 2025-01-08T09:30:48Z

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

+  }
+
+  test("Integration test - Write table with Spark then write with Kernel") {
+    // TODO: Implement this test. Creating and writing a table using Spark with row tracking also


Looking at https://github.com/delta-io/delta/blame/d86ae203a596bedb46b5751f15ca3847613fe0a2/kernel/kernel-api/src/main/java/io/delta/kernel/internal/TableFeatures.java#L262, this should only fail if the table actually contains an invariant, so in most cases it would work, e.p. we should be able to write a test here

Will address this in a follow up PR. Thanks!

…delta-io#3894)  #### Which Delta project/connector is this regarding?  - [ ] Spark - [ ] Standalone - [ ] Flink - [x] Kernel - [ ] Other (fill in here) ## Description  This PR implements the row tracking _support_ requirements in Delta Kernel, according to the [Delta Protocol](https://github.com/delta-io/delta/blob/master/PROTOCOL.md#row-tracking). Specifically, it includes: - assigning `baseRowId` and `defaultRowCommitVersion` to `AddFile` actions prior to committing them - maintaining the `rowIdHighWaterMark` of the `delta.rowTracking` metadata domain during the base row ID assignment, which is the highest assigned fresh row id for the table ## How was this patch tested?  Added tests in `RowTrackingSuite.scala`. This includes unit tests and integration tests with Delta-Spark. ## Does this PR introduce _any_ user-facing changes?  No.

qiyuandong-db force-pushed the delta-kernel-row-tracking branch from 54b77cc to 75c0005 Compare November 21, 2024 15:09

johanl-db reviewed Nov 21, 2024

View reviewed changes

qiyuandong-db force-pushed the delta-kernel-row-tracking branch from 75c0005 to c584707 Compare December 7, 2024 21:46

qiyuandong-db changed the title ~~[Kernel] Assign base row ID to AddFile actions~~ [Kernel] Assign base row ID and default row commit version to AddFile Dec 7, 2024

qiyuandong-db force-pushed the delta-kernel-row-tracking branch from c584707 to e94da89 Compare December 9, 2024 08:28

qiyuandong-db commented Dec 9, 2024

View reviewed changes

johanl-db reviewed Dec 9, 2024

View reviewed changes

qiyuandong-db force-pushed the delta-kernel-row-tracking branch from 954641b to 2816342 Compare December 9, 2024 15:14

scottsand-db requested changes Dec 11, 2024

View reviewed changes

qiyuandong-db force-pushed the delta-kernel-row-tracking branch from 700a46f to 895e0ae Compare December 12, 2024 15:06

scottsand-db requested review from vkorukanti and allisonport-db December 16, 2024 17:30

scottsand-db reviewed Dec 16, 2024

View reviewed changes

zachschuermann requested review from zachschuermann and nicklan December 17, 2024 17:44

qiyuandong-db closed this Jan 6, 2025

qiyuandong-db force-pushed the delta-kernel-row-tracking branch from 0fd9d1b to 8a4cfd4 Compare January 6, 2025 14:58

qiyuandong-db reopened this Jan 6, 2025

Assign baseRowID and defaultRowCommitVersion to AddFile actions

5def107

qiyuandong-db force-pushed the delta-kernel-row-tracking branch from 3e106d8 to 5def107 Compare January 6, 2025 15:21

scottsand-db requested changes Jan 6, 2025

View reviewed changes

qiyuandong-db added 3 commits January 6, 2025 22:56

Address comments

1b72020

AddFile POJO can handle row schema with or without stats field

d803298

Minor doc fix

d86ae20

qiyuandong-db requested a review from scottsand-db January 7, 2025 19:57

scottsand-db approved these changes Jan 7, 2025

View reviewed changes

johanl-db approved these changes Jan 8, 2025

View reviewed changes

Minor fix for error message

dfbf700

qiyuandong-db force-pushed the delta-kernel-row-tracking branch from 3b616b8 to dfbf700 Compare January 8, 2025 10:50

scottsand-db merged commit 837cf22 into delta-io:master Jan 8, 2025
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Kernel] Assign base row ID and default row commit version to AddFile #3894

[Kernel] Assign base row ID and default row commit version to AddFile #3894

qiyuandong-db commented Nov 21, 2024 •

edited

Loading

johanl-db left a comment

johanl-db Nov 21, 2024

scottsand-db Dec 16, 2024

vkorukanti Dec 16, 2024

scottsand-db Dec 16, 2024

allisonport-db Dec 16, 2024

qiyuandong-db Dec 17, 2024

qiyuandong-db Dec 9, 2024

scottsand-db left a comment

scottsand-db Dec 16, 2024

qiyuandong-db Dec 17, 2024

scottsand-db left a comment

scottsand-db Jan 6, 2025

qiyuandong-db Jan 6, 2025

scottsand-db Jan 6, 2025

qiyuandong-db Jan 6, 2025

johanl-db left a comment

johanl-db Jan 8, 2025

qiyuandong-db Jan 8, 2025

[Kernel] Assign base row ID and default row commit version to AddFile #3894

[Kernel] Assign base row ID and default row commit version to AddFile #3894

Conversation

qiyuandong-db commented Nov 21, 2024 • edited Loading

Which Delta project/connector is this regarding?

Description

How was this patch tested?

Does this PR introduce any user-facing changes?

johanl-db 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

scottsand-db left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottsand-db 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

johanl-db left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiyuandong-db commented Nov 21, 2024 •

edited

Loading