[Kernel] Load the protocol and metadata from the CRC files when available #4077

huan233usc · 2025-01-21T18:28:30Z

Which Delta project/connector is this regarding?

Description

CP crc loading code to master branch from "kernel-20250115-crc-optimization"
PR created using git cherry-pick 7d32f66d9efd1dca41ed2a45cf259525cbdd8952

How was this patch tested?

Does this PR introduce any user-facing changes?

No

… in DeltaLog Initial read current version CRC w test to verify wip end-2-end working + tests Co-authored-by: Allison Portis <[email protected]> Co-authored-by: Venki Korukanti <[email protected]>

scottsand-db

Looks great! Left some comments.

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

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

scottsand-db · 2025-01-21T19:18:44Z

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

+        ChecksumReader.getVersionStats(
+            engine, logSegment.logPath, snapshotVersion, crcSearchLowerBound);
+    if (versionStatsOpt.isPresent()) {
+      // We found the protocol and metadata for the version we are looking for


can we add a check that versionStateOpt.get.version is > (or >=, whatever is corect) than the snapshot hint?

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

...el/kernel-defaults/src/test/scala/io/delta/kernel/defaults/LogReplayEngineMetricsSuite.scala

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

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

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

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

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

allisonport-db · 2025-01-22T19:01:07Z

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

+    try (CloseableIterator<FileStatus> crcFiles =
+        engine.getFileSystemClient().listFrom(lowerBoundFilePath.toString())) {
+      List<FileStatus> crcFilesList = new ArrayList<>();
+      crcFiles
+          .filter(file -> isChecksumFile(new Path(file.getPath())))
+          .forEachRemaining(crcFilesList::add);


@scottsand-db I wonder if this can be refactored to use any of the log listing methods in your PR?

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

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

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

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

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

scottsand-db · 2025-01-23T21:26:47Z

...el/kernel-defaults/src/test/scala/io/delta/kernel/defaults/LogReplayEngineMetricsSuite.scala

+    }
+  }
+
+  def buildTableWithCrc(path: String): Unit = {


what are the assumptions of what delta talbe state this method produces? 0.json to 11.json, 0.crc to 11.crc, 10.checkpoint.parquet?

Can we (a) state these assumptions, (b) assert that they are true, and (c) inline in the test succinctly what his has yieled e.g. buildTableWithCrc(path) // 0 to 11 .json, 0 to 11.crc, 10.checkpoint.parquet

This will make understanding the tests much easier.

0.json to 11.json, 0.crc to 11.crc, 10.checkpoint.parquet
Yes correct.
Added comments

Please verbatim include this comment in the buildTableWithCrc(path) usages above

buildTableWithCrc(path) // 0 to 11 .json, 0 to 11.crc, 10.checkpoint.parquet

scottsand-db · 2025-01-23T21:27:21Z

...el/kernel-defaults/src/test/scala/io/delta/kernel/defaults/LogReplayEngineMetricsSuite.scala

+    withTempDirAndMetricsEngine { (path, engine) =>
+      buildTableWithCrc(path)
+      (3 to 6).foreach { version =>
+        val crcFile = new File(path, f"_delta_log/$version%020d.crc")


when creating crc file names, can we please use FileNames. checksumFile

scottsand-db · 2025-01-23T23:22:21Z

...el/kernel-defaults/src/test/scala/io/delta/kernel/defaults/LogReplayEngineMetricsSuite.scala

+    }
+  }
+
+  test(


test("checksum not found at read version and checkpoint exists at read version => use checkpoint") {

...el/kernel-defaults/src/test/scala/io/delta/kernel/defaults/LogReplayEngineMetricsSuite.scala

scottsand-db

Looking good! A few more minor changes to make the code a bit clearer

scottsand-db · 2025-01-24T17:53:12Z

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

+      snapshotHint = Optional.empty();
+    }
+
+    // Finds the exclusive lower bound for CRC search.


inclusive lower bound

scottsand-db · 2025-01-24T17:56:53Z

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

+    checkArgument(targetedVersion == 0 || targetedVersion >= lowerBound);
+    logger.info("Loading CRC file for version {} with lower bound {}", targetedVersion, lowerBound);
+    // First try to load the CRC at given version. If not found or failed to read then try to
+    // find the latest CRC file that is created after the lower bound version or within the last 100


that is created at or after the lower bound version

if no lower bound is provided. --> lowerbound is provided. it is not optional. we can remove this comment.

scottsand-db · 2025-01-24T17:58:37Z

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

+  public static Optional<CRCInfo> getCRCInfo(
+      Engine engine, Path logPath, long targetedVersion, long lowerBound) {
+    // If targetedVersion is 0, lower bound will be ignored as snapshot 0 is the first snapshot.
+    checkArgument(targetedVersion == 0 || targetedVersion >= lowerBound);


because of this or we have to consider cases in the code below where lowerBound is greater than targedVersion. IMO this is confusing and adds extra congnitive load to understand the code.

why not just eliminate this case? checkArgument(targetVersion >= lowerBound)

There is an edge case caused by the caller's call which makes lowerBound at least 1

Math.max(
checkpointVersionFromLogSegment,
snapshotVersion - 100,
snapshotHint.orElse(0) + 1,
0
)
targetVersion=0 needs to be handle separately.

why not just eliminate this case? checkArgument(targetVersion >= lowerBound)
Done
I moved the check to checkArgument(targetVersion >= lowerBound) to line 64 before lower bound is actually used.

There is an edge case caused by the caller's call which makes lowerBound at least 1

Another option would be to, in getCrcInfo, just set lowerBound to min(lowerBound, targetVersion), right?

getCrcInfo, just set lowerBound to min(lowerBound, targetVersion), right?
let me do that

scottsand-db · 2025-01-24T17:59:51Z

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

+      return crcInfoOpt;
+    }
+    logger.info(
+        "CRC file for version {} not found, attempt to loading version up to {}",


"CRC file for version {} not found, listing CRC files from version {}"

scottsand-db · 2025-01-24T18:01:35Z

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

+
+      long crcVersion = FileNames.checksumVersion(filePath);
+
+      return fromColumnarBatch(engine, crcVersion, batch, 0 /* rowId */, filePath.toString());


CRCInfo.fromColumnarBatch.

fromColumnarBatch is used in lots of places in the code --> lets include the classname so it's clear to the reader which class and method you are calling

scottsand-db

LGTM!

allisonport-db · 2025-01-24T22:20:02Z

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

+  public static <T> List<T> toFilteredList(
+      CloseableIterator<T> iterator, Function<T, Boolean> filter) {
+    List<T> result = new ArrayList<>();
+    iterator.filter(filter).forEachRemaining(result::add);
+    return result;
+  }


Sorry nit, I think it's easier if this is just toList and we can do the iterator filtering in the outer call. This way we can re-use this even when we don't want to filter

i.e. toList(iterator.filter(filter))

[Kernel] Support for loading protocol and metadata from checksum file…

dd9fe7b

… in DeltaLog Initial read current version CRC w test to verify wip end-2-end working + tests Co-authored-by: Allison Portis <[email protected]> Co-authored-by: Venki Korukanti <[email protected]>

huan233usc mentioned this pull request Jan 21, 2025

[Kernel] Load the protocol and metadata from the CRC files when available #4071

Closed

5 tasks

scottsand-db requested review from scottsand-db, vkorukanti 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

huan233usc added 4 commits January 21, 2025 15:31

refactor according to the comments

df12241

add logging and fix comments

a404846

move comments

1e410c5

update comments

1994b2e

huan233usc force-pushed the cp-crc branch from 6c44a28 to 1994b2e Compare January 22, 2025 01:12

huan233usc requested a review from scottsand-db January 22, 2025 17:46

update comments

ddc0815

scottsand-db requested changes Jan 22, 2025

View reviewed changes

replace static test table with test generated

02067f3

allisonport-db reviewed Jan 22, 2025

View reviewed changes

huan233usc added 7 commits January 22, 2025 12:05

use optional in building crc and use rfii in crc test

dec4938

resolve comments

6eff126

update comments

69c29df

fix test

fec680e

updated param name

60981a6

update doc

97bc44f

fix idention

2f5b1a2

scottsand-db reviewed Jan 22, 2025

View reviewed changes

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

scottsand-db reviewed Jan 22, 2025

View reviewed changes

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

huan233usc added 2 commits January 22, 2025 13:25

update the doc to reflect nullness

c7bf343

fix javafmt

dbf4490

huan233usc requested a review from scottsand-db January 22, 2025 22:18

huan233usc added 2 commits January 23, 2025 12:51

handle version is 0

9993886

merge from latest version

70829a0

scottsand-db reviewed Jan 23, 2025

View reviewed changes

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

huan233usc added 2 commits January 23, 2025 13:01

fix java format

733eaed

rever accident deleted changes in conflict resolve

4202b31

scottsand-db self-requested a review January 23, 2025 21:21

scottsand-db reviewed Jan 23, 2025

View reviewed changes

huan233usc added 3 commits January 23, 2025 14:06

refactor

37f4251

fix test attempt 1

7494fd3

fix test attempt 2

3f0e05f

scottsand-db reviewed Jan 23, 2025

View reviewed changes

...el/kernel-defaults/src/test/scala/io/delta/kernel/defaults/LogReplayEngineMetricsSuite.scala Show resolved Hide resolved

huan233usc added 4 commits January 23, 2025 16:09

add tests

4bb7bad

fix indent

e42b70e

fix test

18ecc74

add docs

7383397

huan233usc requested a review from scottsand-db January 24, 2025 00:28

huan233usc added 2 commits January 23, 2025 16:30

fix comment

afd5db1

fix comment

ec3a24b

scottsand-db requested changes Jan 24, 2025

View reviewed changes

huan233usc added 2 commits January 24, 2025 10:13

resolve comments

412e0fb

format java

e3d86a9

huan233usc requested a review from scottsand-db January 24, 2025 18:19

huan233usc added 2 commits January 24, 2025 10:26

update check

0dbea3f

format java

588b464

scottsand-db approved these changes Jan 24, 2025

View reviewed changes

allisonport-db reviewed Jan 24, 2025

View reviewed changes

update internal utils to move filter away

155cf2b

huan233usc requested a review from allisonport-db January 24, 2025 22:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Kernel] Load the protocol and metadata from the CRC files when available #4077

[Kernel] Load the protocol and metadata from the CRC files when available #4077

huan233usc commented Jan 21, 2025

scottsand-db left a comment

scottsand-db Jan 21, 2025

allisonport-db Jan 22, 2025

scottsand-db Jan 23, 2025

huan233usc Jan 23, 2025

scottsand-db Jan 23, 2025

scottsand-db Jan 23, 2025

scottsand-db Jan 23, 2025

scottsand-db left a comment

scottsand-db Jan 24, 2025

scottsand-db Jan 24, 2025

scottsand-db Jan 24, 2025

scottsand-db Jan 24, 2025

huan233usc Jan 24, 2025 •

edited

Loading

scottsand-db Jan 24, 2025 •

edited

Loading

huan233usc Jan 24, 2025 •

edited

Loading

scottsand-db Jan 24, 2025

scottsand-db Jan 24, 2025

scottsand-db left a comment

allisonport-db Jan 24, 2025

+                  }
+                }
+                test(


		long crcVersion = FileNames.checksumVersion(filePath);

		return fromColumnarBatch(engine, crcVersion, batch, 0 /* rowId */, filePath.toString());

[Kernel] Load the protocol and metadata from the CRC files when available #4077

Are you sure you want to change the base?

[Kernel] Load the protocol and metadata from the CRC files when available #4077

Conversation

huan233usc commented Jan 21, 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

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

huan233usc Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

scottsand-db Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

huan233usc Jan 24, 2025 • edited Loading

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

huan233usc Jan 24, 2025 •

edited

Loading

scottsand-db Jan 24, 2025 •

edited

Loading

huan233usc Jan 24, 2025 •

edited

Loading