[Kernel] Implement basic CRC writer #4073

huan233usc · 2025-01-18T01:22:48Z

Which Delta project/connector is this regarding?

Description

This PR adds the utility class for writing CRC files for delta table after commit. Integration will be done in the following PR.

How was this patch tested?

Added a new test suite

Does this PR introduce any user-facing changes?

No

scottsand-db

Looks great! Left some comments.

scottsand-db · 2025-01-21T22:39:01Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/checksum/ChecksumWriter.java

+public class ChecksumWriter {
+
+  private static final Logger log = LoggerFactory.getLogger(ChecksumWriter.class);
+  public static StructType CRC_FILE_SCHEMA =


Shouldn't this CRC_FILE_SCHEMA be stored in https://github.com/delta-io/delta/pull/4077/files#diff-897131313222a4b4add04e6c677dc523ce53481fb5e784d7058e9d550a12159a VersionStats

(although I'd like VersionStats to be renamed)

you can have a FULL_SCHEMA (that we use for writing) and a READ_SCHEMA that we use for reading

I create a ChecksumUtils.java for holding shared code path like FULL_SCHEMA. In the end we may also want to make read schema also the same as FULL_SCHEMA. I will do a refactor later.

kernel/kernel-api/src/main/java/io/delta/kernel/internal/checksum/ChecksumWriter.java

kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotHint.java

kernel/kernel-api/src/main/java/io/delta/kernel/internal/checksum/ChecksumWriter.java

scottsand-db · 2025-01-21T22:42:05Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/checksum/ChecksumWriter.java

+   *
+   * @return true if checksum file is successfully written, false otherwise.
+   */
+  public boolean maybeWriteCheckSum(SnapshotHint postCommitSnapshot, String tnxId) {


add some logs stmts?

kernel/kernel-api/src/test/scala/io/delta/kernel/internal/checksum/ChecksumReadWriteSuite.scala

scottsand-db · 2025-01-21T22:46:38Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/checksum/ChecksumWriter.java

+   *
+   * @return true if checksum file is successfully written, false otherwise.
+   */
+  public boolean maybeWriteCheckSum(SnapshotHint postCommitSnapshot, String tnxId) {


nit: txnId not tnxId

scottsand-db · 2025-01-21T22:47:29Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/checksum/ChecksumWriter.java

+   *
+   * @return true if checksum file is successfully written, false otherwise.
+   */
+  public boolean maybeWriteCheckSum(SnapshotHint postCommitSnapshot, String tnxId) {


what is the transaction id? do all transactions have one?

yeah pretty sure this is optional (I think this is setTxnId?...)

https://github.com/delta-io/delta/blob/master/PROTOCOL.md#version-checksum-file-schema

Yeah it is optional, changed the signature.

scottsand-db · 2025-01-21T22:48:26Z

kernel/kernel-api/src/test/scala/io/delta/kernel/internal/checksum/ChecksumReadWriteSuite.scala

+class MockCheckSumFileJsonWriter extends BaseMockJsonHandler {
+  var capturedCrcRow: Row = new GenericRow(new StructType(), new util.HashMap[Integer, AnyRef]);
+
+  override def writeJsonFileAtomically(filePath: String,


nit: weird indentation. Please see https://github.com/databricks/scala-style-guide

fixed scala format plugin locally and re-run

scottsand-db · 2025-01-21T22:49:54Z

kernel/kernel-api/src/test/scala/io/delta/kernel/internal/checksum/ChecksumReadWriteSuite.scala

+
+  def createTestProtocol(): Protocol = {
+    new Protocol(
+      /* minReaderVersion= */ 0,


let's use version 1, 2 not 0, 1.

we may assert in the future that these values are proper, in the protocol constructor; let's get ahead of that now

allisonport-db

Left some comments. For the tests (especially in kernel-api when we're testing inner utils try to make sure we cover all the cases based on the code added/written).

allisonport-db · 2025-01-22T20:03:04Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/checksum/ChecksumWriter.java

+public class ChecksumWriter {
+
+  private static final Logger log = LoggerFactory.getLogger(ChecksumWriter.class);
+  public static StructType CRC_FILE_SCHEMA =


you can have a FULL_SCHEMA (that we use for writing) and a READ_SCHEMA that we use for reading

allisonport-db · 2025-01-22T20:04:51Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/checksum/ChecksumWriter.java

+                .getJsonHandler()
+                .writeJsonFileAtomically(
+                    newChecksumPath.toString(),
+                    toCloseableIterator(


I think you should be able to use Utils.singletonCloseableIterator here instead (cleaner since that's exactly what we want)

allisonport-db · 2025-01-22T20:09:10Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotHint.java

+    this.protocol = protocol;
+    this.metadata = metadata;
+    this.tableSizeBytes = tableSizeBytes;
+    this.numFiles = numFiles;


Is metadata and protocol required non-null here? let's add requireNonNull to all these non-primitive args

otherwise we need null checks up in ChecksumWriter. buildCheckSumRow

allisonport-db · 2025-01-22T20:12:40Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/checksum/ChecksumWriter.java

+   *
+   * @return true if checksum file is successfully written, false otherwise.
+   */
+  public boolean maybeWriteCheckSum(SnapshotHint postCommitSnapshot, String tnxId) {


yeah pretty sure this is optional (I think this is setTxnId?...)

https://github.com/delta-io/delta/blob/master/PROTOCOL.md#version-checksum-file-schema

allisonport-db · 2025-01-22T20:15:37Z

kernel/kernel-api/src/test/scala/io/delta/kernel/internal/checksum/ChecksumReadWriteSuite.scala

+      OptionalLong.of(100),
+      OptionalLong.of(1))
+    checksumWriter.maybeWriteCheckSum(snapshotHint, "tnx")
+


validate the schema first

allisonport-db · 2025-01-22T20:16:34Z

kernel/kernel-api/src/test/scala/io/delta/kernel/internal/checksum/ChecksumReadWriteSuite.scala

+      OptionalLong.of(100),
+      OptionalLong.of(1))
+    checksumWriter.maybeWriteCheckSum(snapshotHint, "tnx")
+


validate the checksum file path that it was written to

allisonport-db · 2025-01-22T20:17:54Z

kernel/kernel-api/src/test/scala/io/delta/kernel/internal/checksum/ChecksumReadWriteSuite.scala

+      OptionalLong.of(100),
+      OptionalLong.of(1))
+    checksumWriter.maybeWriteCheckSum(snapshotHint, "tnx")
+


validate that no additional rows were written to the file (only 1 row was written and it has the schema we expect)

allisonport-db · 2025-01-22T20:18:57Z

kernel/kernel-api/src/test/scala/io/delta/kernel/internal/checksum/ChecksumReadWriteSuite.scala

+import java.util
+import java.util.{Collections, HashMap, Optional, OptionalLong}
+
+class ChecksumReadWriteSuite extends AnyFunSuite with MockEngineUtils {


add tests for numFiles or getTableSizeBytes aren't present ==> no checksum file is written

we also probably want to test that the entire write does not fail if the checksum write fails, but I think we can probably do this in the PR with the e2e integration

also tests for when txnId is not present (let's make it optional as in the protocol)

Done for numFiles or getTableSizeBytes aren't present ==> no checksum file is written and when txnId is not present.

for the second one I will make sure to add it in e2e tests.

allisonport-db · 2025-01-22T20:20:45Z

kernel/kernel-api/src/test/scala/io/delta/kernel/internal/checksum/ChecksumReadWriteSuite.scala

+      new ArrayValue() { // partitionColumns
+        override def getSize = 1
+
+        override def getElements: ColumnVector = singletonStringColumnVector("c3")
+      },


You can use VectorUtils.stringArrayValue here

allisonport-db · 2025-01-22T20:21:22Z

kernel/kernel-api/src/test/scala/io/delta/kernel/internal/checksum/ChecksumReadWriteSuite.scala

+      new MapValue() { // conf
+        override def getSize = 1
+
+        override def getKeys: ColumnVector = singletonStringColumnVector("delta.appendOnly")
+
+        override def getValues: ColumnVector =
+          singletonStringColumnVector("true")
+      }


Same thing here with VectorUtils.stringStringMapValue

huan233usc added 6 commits January 17, 2025 12:56

initial impl of crc writer

f268125

add tests

1e11245

remove empty lines

4379dc7

formatting

6bef23b

add comments

b906527

remove outdated todos

e73f143

scottsand-db requested review from vkorukanti, scottsand-db and allisonport-db and removed request for vkorukanti January 21, 2025 19:05

scottsand-db assigned huan233usc Jan 21, 2025

scottsand-db requested changes Jan 21, 2025

View reviewed changes

allisonport-db requested changes Jan 22, 2025

View reviewed changes

huan233usc added 16 commits January 22, 2025 14:44

fix scala format, change txnid optional

7058da7

saving progress

b45223f

add tests

edc82e2

tnx -> txn

24816c9

introduce crc utils

6398915

save point

e1e98c3

update tests

6f0119c

Merge branch 'master' into crc-writer

cd7aec8

update doc

a3e41c3

move method within utils

81d33e9

make version hint inline

a6c6106

check write sccess

7d594d6

nullness check on m&p

3ce4f75

add logging

9a0e01c

validate only 1 row

7ac14b5

validate path, schema

0b388c7

remove unused import

83341bf

huan233usc requested review from scottsand-db and allisonport-db January 24, 2025 01:37

check crc exists if write is successful

e9acf68

huan233usc marked this pull request as draft January 29, 2025 20:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Kernel] Implement basic CRC writer #4073

[Kernel] Implement basic CRC writer #4073

huan233usc commented Jan 18, 2025

scottsand-db left a comment

scottsand-db Jan 21, 2025

allisonport-db Jan 22, 2025

huan233usc Jan 24, 2025

scottsand-db Jan 21, 2025

scottsand-db Jan 21, 2025

scottsand-db Jan 21, 2025

allisonport-db Jan 22, 2025

huan233usc Jan 24, 2025

scottsand-db Jan 21, 2025

huan233usc Jan 24, 2025

scottsand-db Jan 21, 2025

allisonport-db left a comment

allisonport-db Jan 22, 2025

allisonport-db Jan 22, 2025

allisonport-db Jan 22, 2025

allisonport-db Jan 22, 2025

allisonport-db Jan 22, 2025

allisonport-db Jan 22, 2025

allisonport-db Jan 22, 2025

allisonport-db Jan 22, 2025

allisonport-db Jan 22, 2025

allisonport-db Jan 22, 2025

allisonport-db Jan 22, 2025

huan233usc Jan 24, 2025

allisonport-db Jan 22, 2025

allisonport-db Jan 22, 2025

[Kernel] Implement basic CRC writer #4073

Are you sure you want to change the base?

[Kernel] Implement basic CRC writer #4073

Conversation

huan233usc commented Jan 18, 2025

Which Delta project/connector is this regarding?

Description

How was this patch tested?

Does this PR introduce any user-facing changes?

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

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

allisonport-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

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